Received-SPF: pass (google.com: domain of linux-kernel+bounces-106398-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240226190344.787149-1-pbonzini@redhat.com> <20240226190344.787149-11-pbonzini@redhat.com>
 <20240314024952.w6n6ol5hjzqayn2g@amd.com> <20240314220923.htmb4qix4ct5m5om@amd.com>
 <ZfOAm8HtAaazpc5O@google.com> <20240314234850.js4gvwv7wh43v3y5@amd.com> <ZfRhu0GVjWeAAJMB@google.com>
In-Reply-To: <ZfRhu0GVjWeAAJMB@google.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Mon, 18 Mar 2024 17:48:48 +0100
Message-ID: <CABgObfYNnDXvPU7OMDHzq-yjRYGHaS-M0E_tE2UB4ucv6E1x2Q@mail.gmail.com>
Subject: Re: [PATCH v3 10/15] KVM: x86: add fields to struct kvm_arch for CoCo features
To: Sean Christopherson <seanjc@google.com>
Cc: Michael Roth <michael.roth@amd.com>, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, 
	aik@amd.com, pankaj.gupta@amd.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Mar 15, 2024 at 3:57=E2=80=AFPM Sean Christopherson <seanjc@google.=
com> 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 h=
it and
> > > > > issue with these due to fpstate_set_confidential() being done dur=
ing
> > > > > 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 wi=
th
> > > > > SEV-ES as well.
> > > > > Maybe fpstate_set_confidential() should be relocated to SEV_LAUNC=
H_FINISH
> > > > > site as part of these patches?
> > > >
> > > > Talked to Tom a bit about this and that might not make much sense u=
nless
> > > > we actually want to add some code to sync that FPU state into the V=
MSA
>
> 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 whe=
re 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_co=
nfidential().
>
> 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 a=
n argument
> for immediately marking the fpstate confidential.
>
> That said, any reason not to free the state when the fpstate is marked co=
nfidential?

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 V=
M types
> would be to condition only the return code on whether or not the VM has p=
rotected
> state

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

Paolo