Re: [PATCH V2 1/6] Swiotlb: Add Swiotlb bounce buffer remap function for HV IVM

[Tianyu Lan <ltykernel@gmail.com>]

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?