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

[Zhi Wang]

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

> > From: Zhi Wang <zhi.wang.linux@xxxxxxxxx>
> > 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@xxxxxxxxxxxxx/

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@xxxxxxxxxxxxx/ 
>         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;
> > }