Re: [PATCH v12 18/29] KVM: SEV: Use a VMSA physical address variable for populating VMCB

[Tom Lendacky]

On 4/16/24 06:53, Paolo Bonzini wrote:
> On Sat, Mar 30, 2024 at 10:01 PM Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> > On 3/29/24 23:58, Michael Roth wrote:
> > > From: Tom Lendacky<thomas.lendacky@xxxxxxx>
> > >
> > > In preparation to support SEV-SNP AP Creation, use a variable that holds
> > > the VMSA physical address rather than converting the virtual address.
> > > This will allow SEV-SNP AP Creation to set the new physical address that
> > > will be used should the vCPU reset path be taken.
> > >
> > > Signed-off-by: Tom Lendacky<thomas.lendacky@xxxxxxx>
> > > Signed-off-by: Ashish Kalra<ashish.kalra@xxxxxxx>
> > > Signed-off-by: Michael Roth<michael.roth@xxxxxxx>
> > > ---
> >
> > I'll get back to this one after Easter, but it looks like Sean had some
> > objections at https://lore.kernel.org/lkml/ZeCqnq7dLcJI41O9@xxxxxxxxxx/.

Note that AP create is called multiple times per vCPU under OVMF with 
and added call by the kernel when booting the APs.

> So IIUC the gist of the solution here would be to replace
>
>     /* Use the new VMSA */
>     svm->sev_es.vmsa_pa = pfn_to_hpa(pfn);
>     svm->vmcb->control.vmsa_pa = svm->sev_es.vmsa_pa;
>
> with something like
>
>     /* Use the new VMSA */
>     __free_page(virt_to_page(svm->sev_es.vmsa));

This should only be called for the page that KVM allocated during vCPU 
creation. After that, the VMSA page from an AP create is a guest page 
and shouldn't be freed by KVM.

>     svm->sev_es.vmsa = pfn_to_kaddr(pfn);
>     svm->vmcb->control.vmsa_pa = __pa(svm->sev_es.vmsa);
>
> and wrap the __free_page() in sev_free_vcpu() with "if
> (!svm->sev_es.snp_ap_create)".
>
> This should remove the need for svm->sev_es.vmsa_pa. It is always
> equal to svm->vmcb->control.vmsa_pa anyway.

Yeah, a little bit of multiple VMPL support worked its way in there 
where the VMSA per VMPL level is maintained.

But I believe that Sean wants a separate KVM object per VMPL level, so 
that would disappear anyway (Joerg and I want to get on the PUCK 
schedule to talk about multi-VMPL level support soon.)

> Also, it's possible to remove
>
>     /*
>      * gmem pages aren't currently migratable, but if this ever
>      * changes then care should be taken to ensure
>      * svm->sev_es.vmsa_pa is pinned through some other means.
>      */
>     kvm_release_pfn_clean(pfn);

Removing this here will cause any previous guest VMSA page(s) to remain 
pinned, that's the reason for unpinning here. OVMF re-uses the VMSA, but 
that isn't a requirement for a firmware, and the kernel will create a 
new VMSA page.

> if sev_free_vcpu() does
>
>     if (svm->sev_es.snp_ap_create) {
>       __free_page(virt_to_page(svm->sev_es.vmsa));
>     } else {
>       put_page(virt_to_page(svm->sev_es.vmsa));
>     }
>
> and while at it, please reverse the polarity of snp_ap_create and
> rename it to snp_ap_created.

The snp_ap_create flag gets cleared once the new VMSA is put in place, 
it doesn't remain. So the flag usage will have to be altered in order 
for this function to work properly.

Thanks,
Tom

> Paolo