[v2,2/6] x86/tdx: Support vmalloc() for tdx_enc_status_changed()

  When a TDX guest runs on Hyper-V, the hv_netvsc driver's netvsc_init_buf()
allocates buffers using vzalloc(), and needs to share the buffers with the
host OS by calling set_memory_decrypted(), which is not working for
vmalloc() yet. Add the support by handling the pages one by one.

Signed-off-by: Dexuan Cui <decui@microsoft.com>

---

Changes in v2:
  Changed tdx_enc_status_changed() in place.
  
Hi, Dave, I checked the huge vmalloc mapping code, but still don't know
how to get the underlying huge page info (if huge page is in use) and
try to use PG_LEVEL_2M/1G in try_accept_page() for vmalloc: I checked
is_vm_area_hugepages() and  __vfree() -> __vunmap(), and I think the
underlying page allocation info is internal to the mm code, and there
is no mm API to for me get the info in tdx_enc_status_changed().

Hi, Kirill, the load_unaligned_zeropad() issue is not addressed in
this patch. The issue looks like a generic issue that also happens to
AMD SNP vTOM mode and C-bit mode. Will need to figure out how to
address the issue. If we decide to adjust direct mapping to have the
shared bit set, it lools like we need to do the below for each
'start_va' vmalloc page:
  pa = slow_virt_to_phys(start_va);
  set_memory_decrypted(phys_to_virt(pa), 1); -- this line calls
tdx_enc_status_changed() the second time for the page, which is bad.
It looks like we need to find a way to reuse the cpa_flush() related
code in __set_memory_enc_pgtable() and make sure we call
tdx_enc_status_changed() only once for a vmalloc page?

  
 arch/x86/coco/tdx/tdx.c | 69 ++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 25 deletions(-)
  

On Tue,  6 Dec 2022 16:33:21 -0800
Dexuan Cui <decui@microsoft.com> wrote:

> When a TDX guest runs on Hyper-V, the hv_netvsc driver's
> netvsc_init_buf() allocates buffers using vzalloc(), and needs to share
> the buffers with the host OS by calling set_memory_decrypted(), which is
> not working for vmalloc() yet. Add the support by handling the pages one
> by one.
> 

It seems calling set_memory_decrypted() in netvsc_init_buf() is missing in
this patch series. I guess there should be another one extra patch to cover
that.

> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> 
> ---
> 
> Changes in v2:
>   Changed tdx_enc_status_changed() in place.
>   
> Hi, Dave, I checked the huge vmalloc mapping code, but still don't know
> how to get the underlying huge page info (if huge page is in use) and
> try to use PG_LEVEL_2M/1G in try_accept_page() for vmalloc: I checked
> is_vm_area_hugepages() and  __vfree() -> __vunmap(), and I think the
> underlying page allocation info is internal to the mm code, and there
> is no mm API to for me get the info in tdx_enc_status_changed().
> 
> Hi, Kirill, the load_unaligned_zeropad() issue is not addressed in
> this patch. The issue looks like a generic issue that also happens to
> AMD SNP vTOM mode and C-bit mode. Will need to figure out how to
> address the issue. If we decide to adjust direct mapping to have the
> shared bit set, it lools like we need to do the below for each
> 'start_va' vmalloc page:
>   pa = slow_virt_to_phys(start_va);
>   set_memory_decrypted(phys_to_virt(pa), 1); -- this line calls
> tdx_enc_status_changed() the second time for the page, which is bad.
> It looks like we need to find a way to reuse the cpa_flush() related
> code in __set_memory_enc_pgtable() and make sure we call
> tdx_enc_status_changed() only once for a vmalloc page?
> 
>   
>  arch/x86/coco/tdx/tdx.c | 69 ++++++++++++++++++++++++++---------------
>  1 file changed, 44 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index cdeda698d308..795ac56f06b8 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -5,6 +5,7 @@
>  #define pr_fmt(fmt)     "tdx: " fmt
>  
>  #include <linux/cpufeature.h>
> +#include <linux/mm.h>
>  #include <asm/coco.h>
>  #include <asm/tdx.h>
>  #include <asm/vmx.h>
> @@ -693,6 +694,34 @@ static bool try_accept_one(phys_addr_t *start,
> unsigned long len, return true;
>  }
>  
> +static bool try_accept_page(phys_addr_t start, phys_addr_t end)
> +{
> +	/*
> +	 * For shared->private conversion, accept the page using
> +	 * TDX_ACCEPT_PAGE TDX module call.
> +	 */
> +	while (start < end) {
> +		unsigned long len = end - start;
> +
> +		/*
> +		 * Try larger accepts first. It gives chance to VMM to
> keep
> +		 * 1G/2M SEPT entries where possible and speeds up
> process by
> +		 * cutting number of hypercalls (if successful).
> +		 */
> +
> +		if (try_accept_one(&start, len, PG_LEVEL_1G))
> +			continue;
> +
> +		if (try_accept_one(&start, len, PG_LEVEL_2M))
> +			continue;
> +
> +		if (!try_accept_one(&start, len, PG_LEVEL_4K))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  /*
>   * Notify the VMM about page mapping conversion. More info about ABI
>   * can be found in TDX Guest-Host-Communication Interface (GHCI),
> @@ -749,37 +778,27 @@ static bool tdx_map_gpa(phys_addr_t start,
> phys_addr_t end, bool enc) */
>  static bool tdx_enc_status_changed(unsigned long vaddr, int numpages,
> bool enc) {
> -	phys_addr_t start = __pa(vaddr);
> -	phys_addr_t end   = __pa(vaddr + numpages * PAGE_SIZE);
> +	bool is_vmalloc = is_vmalloc_addr((void *)vaddr);
> +	unsigned long len = numpages * PAGE_SIZE;
> +	void *start_va = (void *)vaddr, *end_va = start_va + len;
> +	phys_addr_t start_pa, end_pa;
>  
> -	if (!tdx_map_gpa(start, end, enc))
> +	if (offset_in_page(start_va) != 0)
>  		return false;
>  
> -	/* private->shared conversion  requires only MapGPA call */
> -	if (!enc)
> -		return true;
> -
> -	/*
> -	 * For shared->private conversion, accept the page using
> -	 * TDX_ACCEPT_PAGE TDX module call.
> -	 */
> -	while (start < end) {
> -		unsigned long len = end - start;
> -
> -		/*
> -		 * Try larger accepts first. It gives chance to VMM to
> keep
> -		 * 1G/2M SEPT entries where possible and speeds up
> process by
> -		 * cutting number of hypercalls (if successful).
> -		 */
> -
> -		if (try_accept_one(&start, len, PG_LEVEL_1G))
> -			continue;
> +	while (start_va < end_va) {
> +		start_pa = is_vmalloc ? slow_virt_to_phys(start_va) :
> +					__pa(start_va);
> +		end_pa = start_pa + (is_vmalloc ? PAGE_SIZE : len);
>  
> -		if (try_accept_one(&start, len, PG_LEVEL_2M))
> -			continue;
> +		if (!tdx_map_gpa(start_pa, end_pa, enc))
> +			return false;
>  
> -		if (!try_accept_one(&start, len, PG_LEVEL_4K))
> +		/* private->shared conversion requires only MapGPA call
> */
> +		if (enc && !try_accept_page(start_pa, end_pa))
>  			return false;
> +
> +		start_va += is_vmalloc ? PAGE_SIZE : len;
>  	}
>  
>  	return true;
  

> From: Zhi Wang <zhi.wang.linux@gmail.com>
> Sent: Thursday, January 5, 2023 1:45 AM
>  [...]
> On Tue,  6 Dec 2022 16:33:21 -0800
> Dexuan Cui <decui@microsoft.com> wrote:
> 
> > When a TDX guest runs on Hyper-V, the hv_netvsc driver's
> > netvsc_init_buf() allocates buffers using vzalloc(), and needs to share
> > the buffers with the host OS by calling set_memory_decrypted(), which is
> > not working for vmalloc() yet. Add the support by handling the pages one
> > by one.
> 
> It seems calling set_memory_decrypted() in netvsc_init_buf() is missing in
> this patch series. I guess there should be another one extra patch to cover
> that.

set_memory_decrypted() is not missing here. In netvsc_init_buf(), after
the line "net_device->recv_buf = vzalloc(buf_size);", we have 

vmbus_establish_gpadl(device->channel, net_device->recv_buf, ...), which

calls __vmbus_establish_gpadl(), which calls 

set_memory_decrypted((unsigned long)kbuffer, ...)
  

On Thu, 5 Jan 2023 17:33:16 +0000
Dexuan Cui <decui@microsoft.com> wrote:

> > From: Zhi Wang <zhi.wang.linux@gmail.com>
> > Sent: Thursday, January 5, 2023 1:45 AM
> >  [...]
> > On Tue,  6 Dec 2022 16:33:21 -0800
> > Dexuan Cui <decui@microsoft.com> wrote:
> > 
> > > When a TDX guest runs on Hyper-V, the hv_netvsc driver's
> > > netvsc_init_buf() allocates buffers using vzalloc(), and needs to
> > > share the buffers with the host OS by calling
> > > set_memory_decrypted(), which is not working for vmalloc() yet. Add
> > > the support by handling the pages one by one.
> > 
> > It seems calling set_memory_decrypted() in netvsc_init_buf() is
> > missing in this patch series. I guess there should be another one
> > extra patch to cover that.
> 
> set_memory_decrypted() is not missing here. In netvsc_init_buf(), after
> the line "net_device->recv_buf = vzalloc(buf_size);", we have 
> 
> vmbus_establish_gpadl(device->channel, net_device->recv_buf, ...), which
> 
> calls __vmbus_establish_gpadl(), which calls 
> 
> set_memory_decrypted((unsigned long)kbuffer, ...)
> 

I see. Then do we still need the hv_map_memory()in the following
code piece in netvsc.c after {set_memoery_encrypted, decrypted}()
supporting memory from vmalloc()?

	/* set_memory_decrypted() is called here. */

        ret = vmbus_establish_gpadl(device->channel, net_device->recv_buf,
                                    buf_size,
                                    &net_device->recv_buf_gpadl_handle);
        if (ret != 0) {
                netdev_err(ndev,
                        "unable to establish receive buffer's gpadl\n");
                goto cleanup;
        }
	
	/* Should we remove this? */
        if (hv_isolation_type_snp()) {
                vaddr = hv_map_memory(net_device->recv_buf, buf_size);
                if (!vaddr) {
                        ret = -ENOMEM;
                        goto cleanup;
                }

                net_device->recv_original_buf = net_device->recv_buf;
                net_device->recv_buf = vaddr;
        }

I assume that we need an VA mapped to a shared GPA here.

The VA(net_device->recv_buf) has been associated with a shared GPA in
set_memory_decrypted() by adjusting the kernel page table. hv_map_memory()
is with similar purpose but just a different way:

void *hv_map_memory(void *addr, unsigned long size)
{
        unsigned long *pfns = kcalloc(size / PAGE_SIZE,
                                      sizeof(unsigned long), GFP_KERNEL);
        void *vaddr;
        int i;

        if (!pfns)
                return NULL;

        for (i = 0; i < size / PAGE_SIZE; i++)
                pfns[i] = vmalloc_to_pfn(addr + i * PAGE_SIZE) +
                        (ms_hyperv.shared_gpa_boundary >> PAGE_SHIFT);

        vaddr = vmap_pfn(pfns, size / PAGE_SIZE, PAGE_KERNEL_IO);
        kfree(pfns);

        return vaddr;
}
  

> From: Zhi Wang <zhi.wang.linux@gmail.com>
> Sent: Thursday, January 5, 2023 10:10 AM
> [...]
> I see. Then do we still need the hv_map_memory()in the following
> code piece in netvsc.c after {set_memoery_encrypted, decrypted}()
> supporting memory from vmalloc()?

For SNP, set_memory_decrypted() is already able to support memory
from vmalloc().

For TDX, currently set_memory_decrypted()() is unable to support
memory from vmalloc().

>         /* set_memory_decrypted() is called here. */
>         ret = vmbus_establish_gpadl(device->channel,
> net_device->recv_buf, buf_size, 
> &net_device->recv_buf_gpadl_handle);
>         if (ret != 0) {
>                 netdev_err(ndev,
>                         "unable to establish receive buffer's gpadl\n");
>                 goto cleanup;
>         }
> 
>         /* Should we remove this? */

The below block of code is for SNP rather than TDX, so it has nothing to do
with the patch here. BTW, the code is ineeded removed in Michael's patchset,
which is for device assignment support for SNP guests on Hyper-V:
https://lwn.net/ml/linux-kernel/1669951831-4180-11-git-send-email-mikelley@microsoft.com/
and I'm happy with the removal of the code.

>         if (hv_isolation_type_snp()) {
>                 vaddr = hv_map_memory(net_device->recv_buf, buf_size);
>                 if (!vaddr) {
>                         ret = -ENOMEM;
>                         goto cleanup;
>                 }
> 
>                 net_device->recv_original_buf = net_device->recv_buf;
>                 net_device->recv_buf = vaddr;
>         }
> 
> I assume that we need an VA mapped to a shared GPA here.

Yes.

> The VA(net_device->recv_buf) has been associated with a shared GPA in
> set_memory_decrypted() by adjusting the kernel page table.

For a SNP guest with pavavisor on Hyper-V, this is not true in the current
mainline kernel: see set_memory_decrypted() -> __set_memory_enc_dec():

static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
{
		//Dexuan: For a SNP guest with paravisor on Hyper-V, currently we
        // only call hv_set_mem_host_visibility(), i.e. the page tabe is not
        // updated. This is being changed by Michael's patchset, e.g.,
https://lwn.net/ml/linux-kernel/1669951831-4180-7-git-send-email-mikelley@microsoft.com/
        
        if (hv_is_isolation_supported())
                return hv_set_mem_host_visibility(addr, numpages, !enc);

        if (cc_platform_has(CC_ATTR_MEM_ENCRYPT))
                return __set_memory_enc_pgtable(addr, numpages, enc);

        return 0;
}

> hv_map_memory()
> is with similar purpose but just a different way:
> 
> void *hv_map_memory(void *addr, unsigned long size)
> {
>         unsigned long *pfns = kcalloc(size / PAGE_SIZE,
>                                       sizeof(unsigned long),
> GFP_KERNEL);
>         void *vaddr;
>         int i;
> 
>         if (!pfns)
>                 return NULL;
> 
>         for (i = 0; i < size / PAGE_SIZE; i++)
>                 pfns[i] = vmalloc_to_pfn(addr + i * PAGE_SIZE) +
>                         (ms_hyperv.shared_gpa_boundary >>
> PAGE_SHIFT);
> 
>         vaddr = vmap_pfn(pfns, size / PAGE_SIZE, PAGE_KERNEL_IO);
>         kfree(pfns);
> 
>         return vaddr;
> }
  

On Thu, 5 Jan 2023 20:29:25 +0000
Dexuan Cui <decui@microsoft.com> wrote:

> > From: Zhi Wang <zhi.wang.linux@gmail.com>
> > Sent: Thursday, January 5, 2023 10:10 AM
> > [...]
> > I see. Then do we still need the hv_map_memory()in the following
> > code piece in netvsc.c after {set_memoery_encrypted, decrypted}()
> > supporting memory from vmalloc()?
> 
> For SNP, set_memory_decrypted() is already able to support memory
> from vmalloc().
> 
> For TDX, currently set_memory_decrypted()() is unable to support
> memory from vmalloc().
> 
I guess we both agree that memory conversion in HV should be done through
coco so the hv_map_memory can be removed (even the extra does not hurt
currently)

The memory conversion in current HV code is done by different approaches.
Some are going through the coco, some are not, which ends up
with if(hv_isolation_type_snp()) in memory allocation path. It can be
confusing. I suppose a reasonable purpose of hv_isolation_type_snp()
should cover the AMD SEV-SNP specific parts which haven't been (or are
not going to be) covered by coco. For example the GHCB stuff. 

Thanks,
Zhi.

> >         /* set_memory_decrypted() is called here. */
> >         ret = vmbus_establish_gpadl(device->channel,
> > net_device->recv_buf, buf_size, 
> > &net_device->recv_buf_gpadl_handle);
> >         if (ret != 0) {
> >                 netdev_err(ndev,
> >                         "unable to establish receive buffer's
> > gpadl\n"); goto cleanup;
> >         }
> > 
> >         /* Should we remove this? */
> 
> The below block of code is for SNP rather than TDX, so it has nothing to
> do with the patch here. BTW, the code is ineeded removed in Michael's
> patchset, which is for device assignment support for SNP guests on
> Hyper-V:
> https://lwn.net/ml/linux-kernel/1669951831-4180-11-git-send-email-mikelley@microsoft.com/

So happy to see this. :)

> and I'm happy with the removal of the code.
> 
> >         if (hv_isolation_type_snp()) {
> >                 vaddr = hv_map_memory(net_device->recv_buf, buf_size);
> >                 if (!vaddr) {
> >                         ret = -ENOMEM;
> >                         goto cleanup;
> >                 }
> > 
> >                 net_device->recv_original_buf = net_device->recv_buf;
> >                 net_device->recv_buf = vaddr;
> >         }
> > 
> > I assume that we need an VA mapped to a shared GPA here.
> 
> Yes.
> 
> > The VA(net_device->recv_buf) has been associated with a shared GPA in
> > set_memory_decrypted() by adjusting the kernel page table.
> 
> For a SNP guest with pavavisor on Hyper-V, this is not true in the
> current mainline kernel: see set_memory_decrypted() ->
> __set_memory_enc_dec():
> 
> static int __set_memory_enc_dec(unsigned long addr, int numpages, bool
> enc) {
> 		//Dexuan: For a SNP guest with paravisor on Hyper-V,
> currently we // only call hv_set_mem_host_visibility(), i.e. the page
> tabe is not // updated. This is being changed by Michael's patchset,
> e.g.,
> https://lwn.net/ml/linux-kernel/1669951831-4180-7-git-send-email-mikelley@microsoft.com/ 
>         if (hv_is_isolation_supported())
>                 return hv_set_mem_host_visibility(addr, numpages, !enc);
> 
>         if (cc_platform_has(CC_ATTR_MEM_ENCRYPT))
>                 return __set_memory_enc_pgtable(addr, numpages, enc);
> 
>         return 0;
> }
> 
> > hv_map_memory()
> > is with similar purpose but just a different way:
> > 
> > void *hv_map_memory(void *addr, unsigned long size)
> > {
> >         unsigned long *pfns = kcalloc(size / PAGE_SIZE,
> >                                       sizeof(unsigned long),
> > GFP_KERNEL);
> >         void *vaddr;
> >         int i;
> > 
> >         if (!pfns)
> >                 return NULL;
> > 
> >         for (i = 0; i < size / PAGE_SIZE; i++)
> >                 pfns[i] = vmalloc_to_pfn(addr + i * PAGE_SIZE) +
> >                         (ms_hyperv.shared_gpa_boundary >>
> > PAGE_SHIFT);
> > 
> >         vaddr = vmap_pfn(pfns, size / PAGE_SIZE, PAGE_KERNEL_IO);
> >         kfree(pfns);
> > 
> >         return vaddr;
> > }
  

> From: Zhi Wang <zhi.wang.linux@gmail.com>
> Sent: Friday, January 6, 2023 2:11 AM
> To: Dexuan Cui <decui@microsoft.com>
>  [...]
> I guess we both agree that memory conversion in HV should be done through
> coco so the hv_map_memory can be removed (even the extra does not hurt
> currently)

Correct. As I mentioned, Michael's pachset is doing that and hopefully it would
be merged into the upstream soon.

> The memory conversion in current HV code is done by different approaches.
> Some are going through the coco, some are not, which ends up
> with if(hv_isolation_type_snp()) in memory allocation path. It can be
> confusing. I suppose a reasonable purpose of hv_isolation_type_snp()
> should cover the AMD SEV-SNP specific parts which haven't been (or are
> not going to be) covered by coco. For example the GHCB stuff.
> 
> Thanks,
> Zhi.

Exactly.