Re: [PATCH RFC v8 47/56] KVM: SVM: Support SEV-SNP AP Creation NAE event

[Michael Roth <michael.roth@amd.com>]

On Wed, Mar 01, 2023 at 10:14:19PM +0100, Alexander Graf wrote:
> 
> On 28.02.23 21:47, Zhi Wang wrote:
> > On Fri, 24 Feb 2023 13:37:48 +0100
> > Alexander Graf <graf@amazon.com> wrote:
> > 
> > > On 20.02.23 19:38, Michael Roth wrote:
> > > > From: Tom Lendacky <thomas.lendacky@amd.com>
> > > > 
> > > > Add support for the SEV-SNP AP Creation NAE event. This allows SEV-SNP
> > > > guests to alter the register state of the APs on their own. This allows
> > > > the guest a way of simulating INIT-SIPI.

<snip>

> > > > +                */
> > > > +               if (IS_ALIGNED(svm->vmcb->control.exit_info_2, PMD_SIZE)) {
> > > > +                       vcpu_unimpl(vcpu,
> > > > +                                   "vmgexit: AP VMSA address [%llx] from guest is unsafe as it is 2M aligned\n",
> > > > +                                   svm->vmcb->control.exit_info_2);
> > > > +                       ret = -EINVAL;
> > > > +                       goto out;
> > > > +               }
> > > > +
> > > > +               target_svm->sev_es.snp_vmsa_gpa = svm->vmcb->control.exit_info_2;
> > > > +               break;
> > > > +       case SVM_VMGEXIT_AP_DESTROY:
> > > 
> > > I don't understand the destroy path. Why does this case destroy anything?
> > > 
> > > 
> > > > +               break;
> > > > +       default:
> > > > +               vcpu_unimpl(vcpu, "vmgexit: invalid AP creation request [%#x] from guest\n",
> > > > +                           request);
> > > > +               ret = -EINVAL;
> > > > +               break;
> > > > +       }
> > > > +
> > > > +out:
> > > > +       if (kick) {
> > > > +               if (target_vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)
> > > > +                       target_vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> > > 
> > > What if the guest AP goes through a create -> destroy -> create cycle?
> > > Will it stay runnable while destroyed?
> > The code is not very straightforward.
> > 
> > 1) target_svm->sev_es.snp_vmsa_gpa is set as INVALID_PAGE in the beginning of this function.
> > 
> > 2) If a DESTROY is hit in this function, target_svm->sev_es.snp_vmsa_gpa will be
> > left as INVALID_PAGE.
> > 
> > 3) At the end of this function, it calls kvm_make_request(KVM_REQ_UPDATE_PROTECTED_GUEST_STATE).
> > 
> > 4) In the vcpu_enter_guest(), the kvm_vcpu_reset()->sev_snp_init_protected_guest_state()
> > ->__sev_snp_init_protected_guest_state() is called.
> > 
> > 5) The mp_state is set to KVM_MP_STATE_STOPPED by default and the runtime VMSA is
> > cleared. Then the it will be initialized according to the guest's
> > configuration.
> > 
> > 6) As the snp_vmsa_gpa is set as INVALID_PAGE in 1, the mp_state will be left as
> > KVM_MP_STATE_STOPPED.
> > 
> > 7) With this code piece:
> > 
> > +                       kvm_vcpu_reset(vcpu, true);
> > +                       if (vcpu->arch.mp_state != KVM_MP_STATE_RUNNABLE)
> > +                               goto out;
> > 
> > vcpu_enter_guest() bails out.
> 
> 
> Thanks a lot Zhi for the detailed explanation! I think this code flow wants
> to become slightly more obvious. For example, if we just said
> 
>   case SVM_VMGEXIT_AP_DESTROY:
>     /* This will tell __sev_snp_update_protected_guest_state to unmap the
> VMSA */
>     target_svm->sev_es.snp_vmsa_gpa = INVALID_PAGE;
>     break;
> 
> We'd get a big win in readability with little effort. It makes it
> immediately obvious where to look for the destroy operation.

The target->snp_vmsa_gpa value mainly serves to cache the GPA of the new
VMSA up until the point where KVM points the target vCPU's VMCB over to
using it as part of sev_snp_init_protected_guest_state(). We want to
make sure it is either INVALID_PAGE, or whatever GPA the guest want us
to set it to.

Once sev_snp_init_protected_guest_state() processes the update, it will
set it back to INVALID_PAGE. So if we initialized it once during VM
creation it wouldn't be necessary to keep setting it to INVALID_PAGE at
all, except for one case ...

There is a window where the guest could do an AP_CREATE with new GPA,
followed immediately by AP_DESTROY. If that AP_DESTROY got processed
before the vCPU re-enters the guest, it might instead restart the vCPU
(due to taking VALID_PAGE(snp_vmsa_gpa) branch in
__sev_snp_update_protected_guest_state()). So for that reason it makes
sense to always initialize it to INVALID_PAGE when an AP_CREATION
request comes in, to essentially clear any pending updates from a
previous pending AP_CREATION for the same vCPU. If we move it to the
AP_DESTROY case, but hit a failure elsewhere in sev_snp_ap_creation(),
then the calling vCPU will get a #GP, but the target vCPU might get
set up with the VMSA for the previous pending request. That might be
appropriate though, just because calling vCPU hosed itself doesn't mean
a previous valid AP_CREATION request shouldn't go through for target.
Hmm...

And having said all that... yes, I guess that basically boils down to
INVALID_PAGE being used as an indicator of whether or not to unmap
VMSA. I'll try to come up with some wording to better convey all this
a bit more clearly.

Thanks,

-Mike

> 
> 
> Alex
> 
> 
> 
> 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
> 
>