Re: [PATCH v3 10/15] KVM: x86: add fields to struct kvm_arch for CoCo features

[Paolo Bonzini]

On Fri, Mar 15, 2024 at 3:57 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Thu, Mar 14, 2024, Michael Roth wrote:
> > On Thu, Mar 14, 2024 at 03:56:27PM -0700, Sean Christopherson wrote:
> > > On Thu, Mar 14, 2024, Michael Roth wrote:
> > > > On Wed, Mar 13, 2024 at 09:49:52PM -0500, Michael Roth wrote:
> > > > > I've been trying to get SNP running on top of these patches and hit and
> > > > > issue with these due to fpstate_set_confidential() being done during
> > > > > svm_vcpu_create(), so when QEMU tries to sync FPU state prior to calling
> > > > > SNP_LAUNCH_FINISH it errors out. I think the same would happen with
> > > > > SEV-ES as well.
> > > > > Maybe fpstate_set_confidential() should be relocated to SEV_LAUNCH_FINISH
> > > > > site as part of these patches?
> > > >
> > > > Talked to Tom a bit about this and that might not make much sense unless
> > > > we actually want to add some code to sync that FPU state into the VMSA
>
> Is manually copying required for register state?  If so, manually copying everything
> seems like the way to go, otherwise we'll end up with a confusing ABI where a
> rather arbitrary set of bits are (not) configurable by userspace.

Yes, see sev_es_sync_vmsa. I'll add FPU as well.

> > SET_REGS/SREGS and the others only throw an error when
> > vcpu->arch.guest_state_protected gets set, which doesn't happen until
>
> Ah, I misread the diff and didn't see the existing check on fpstate_is_confidential().
>
> Side topic, I could have sworn KVM didn't allocate the guest fpstate for SEV-ES,
> but git blame says otherwise.  Avoiding that allocation would have been an argument
> for immediately marking the fpstate confidential.
>
> That said, any reason not to free the state when the fpstate is marked confidential?

No reason not to do it, except not wanting to add more cases to code
that's already pretty hairy.

> > sev_launch_update_vmsa(). So in those cases userspace is still able to sync
> > additional/non-reset state prior initial launch. It's just XSAVE/XSAVE2 that
> > are a bit more restrictive because they check fpstate_is_confidential()
> > instead, which gets set during vCPU creation.
> >
> > Somewhat related, but just noticed that KVM_SET_FPU also relies on
> > fpstate_is_confidential() but still silently returns 0 with this series.
> > Seems like it should be handled the same way as XSAVE/XSAVE2, whatever we
> > end up doing.
>
> +1
>
> Also, I think a less confusing and more robust way to deal with the new VM types
> would be to condition only the return code on whether or not the VM has protected
> state

Makes sense (I found KVM_GET/SET_FPU independently and will fix that
as well in the next submission).

Paolo