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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <887d57bc-8b1a-48ab-be72-17144791334a@gmail.com>
Date:   Wed, 24 Nov 2021 22:07:40 +0800
From:   Tianyu Lan <ltykernel@...il.com>
To:     "Michael Kelley (LINUX)" <mikelley@...rosoft.com>,
        Christoph Hellwig <hch@....de>
Cc:     "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        vkuznets <vkuznets@...hat.com>,
        "brijesh.singh@....com" <brijesh.singh@....com>,
        "konrad.wilk@...cle.com" <konrad.wilk@...cle.com>,
        "parri.andrea@...il.com" <parri.andrea@...il.com>,
        "dave.hansen@...el.com" <dave.hansen@...el.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
        "luto@...nel.org" <luto@...nel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "jgross@...e.com" <jgross@...e.com>,
        "sstabellini@...nel.org" <sstabellini@...nel.org>,
        "boris.ostrovsky@...cle.com" <boris.ostrovsky@...cle.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "joro@...tes.org" <joro@...tes.org>,
        "will@...nel.org" <will@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "hch@....de" <hch@....de>,
        "m.szyprowski@...sung.com" <m.szyprowski@...sung.com>,
        "robin.murphy@....com" <robin.murphy@....com>,
        Tianyu Lan <Tianyu.Lan@...rosoft.com>,
        "thomas.lendacky@....com" <thomas.lendacky@....com>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>
Subject: Re: [PATCH V2 1/6] Swiotlb: Add Swiotlb bounce buffer remap function
 for HV IVM

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?