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?
