lkml.org 
[lkml]   [2021]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH V2 1/6] Swiotlb: Add Swiotlb bounce buffer remap function for HV IVM
From
Hi Michael:
Thanks for your review.

On 11/24/2021 1:15 AM, Michael Kelley (LINUX) wrote:
>> @@ -172,7 +200,14 @@ void __init swiotlb_update_mem_attributes(void)
>> vaddr = phys_to_virt(mem->start);
>> bytes = PAGE_ALIGN(mem->nslabs << IO_TLB_SHIFT);
>> set_memory_decrypted((unsigned long)vaddr, bytes >> PAGE_SHIFT);
>> - memset(vaddr, 0, bytes);
>> +
>> + mem->vaddr = swiotlb_mem_remap(mem, bytes);
>> + if (!mem->vaddr) {
>> + pr_err("Fail to remap swiotlb mem.\n");
>> + return;
>> + }
>> +
>> + memset(mem->vaddr, 0, bytes);
>> }


> In the error case, do you want to leave mem->vaddr as NULL? Or is it
> better to leave it as the virtual address of mem-start? Your code leaves it
> as NULL.
>
> The interaction between swiotlb_update_mem_attributes() and the helper
> function swiotlb_memo_remap() seems kind of clunky. phys_to_virt() gets called
> twice, for example, and two error messages are printed. The code would be
> more straightforward by just putting the helper function inline:
>
> mem->vaddr = phys_to_virt(mem->start);
> bytes = PAGE_ALIGN(mem->nslabs << IO_TLB_SHIFT);
> set_memory_decrypted((unsigned long)(mem->vaddr), bytes >> PAGE_SHIFT);
>
> if (swiotlb_unencrypted_base) {
> phys_addr_t paddr = mem->start + swiotlb_unencrypted_base;
>
> mem->vaddr = memremap(paddr, bytes, MEMREMAP_WB);
> if (!mem->vaddr) {
> pr_err("Failed to map the unencrypted memory %llx size %lx.\n",
> paddr, bytes);
> return;
> }
> }
>
> memset(mem->vaddr, 0, bytes);
>
> (This version also leaves mem->vaddr as NULL in the error case.)

From Christoph's previous suggestion, there should be a well-documented
wrapper to explain the remap option and so I split the code. leaving the
virtual address of mem-start is better.

https://lkml.org/lkml/2021/9/28/51

>
>> static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t start,
>> @@ -196,7 +231,18 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t start,
>> mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
>> mem->slots[i].alloc_size = 0;
>> }
>> +
>> + /*
>> + * If swiotlb_unencrypted_base is set, the bounce buffer memory will
>> + * be remapped and cleared in swiotlb_update_mem_attributes.
>> + */
>> + if (swiotlb_unencrypted_base)
>> + return;
>> +
>> + set_memory_decrypted((unsigned long)vaddr, bytes >> PAGE_SHIFT);
> Prior to this patch, and here in the new version as well, the return value from
> set_memory_decrypted() is ignored in several places in this file. As previously
> discussed, swiotlb_init_io_tlb_mem() is a void function, so there's no place to
> return an error. Is that OK?

Yes, the original code doesn't check the return value and so keep the
rule。

Christoph, Could you help to check which way do you prefer?


\
 
 \ /
  Last update: 2021-11-24 15:41    [W:0.036 / U:0.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site