On Mon, Feb 26, 2024 at 02:03:39PM -0500, Paolo Bonzini wrote:
> Some VM types have characteristics in common; in fact, the only use
> of VM types right now is kvm_arch_has_private_mem and it assumes that
> _all_ nonzero VM types have private memory.
>
> We will soon introduce a VM type for SEV and SEV-ES VMs, and at that
> point we will have two special characteristics of confidential VMs
> that depend on the VM type: not just if memory is private, but
> also whether guest state is protected. For the latter we have
> kvm->arch.guest_state_protected, which is only set on a fully initialized
> VM.
>
> For VM types with protected guest state, we can actually fix a problem in
> the SEV-ES implementation, where ioctls to set registers do not cause an
> error even if the VM has been initialized and the guest state encrypted.
> Make sure that when using VM types that will become an error.
>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Message-Id: <20240209183743.22030-7-pbonzini@xxxxxxxxxx>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
> arch/x86/include/asm/kvm_host.h | 7 ++-
> arch/x86/kvm/x86.c | 95 +++++++++++++++++++++++++++------
> 2 files changed, 84 insertions(+), 18 deletions(-)
>
> @@ -5552,9 +5561,13 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
> }
>
>
> -static void kvm_vcpu_ioctl_x86_get_xsave2(struct kvm_vcpu *vcpu,
> - u8 *state, unsigned int size)
> +static int kvm_vcpu_ioctl_x86_get_xsave2(struct kvm_vcpu *vcpu,
> + u8 *state, unsigned int size)
> {
> + if (vcpu->kvm->arch.has_protected_state &&
> + fpstate_is_confidential(&vcpu->arch.guest_fpu))
> + return -EINVAL;
> +
> /*
> * Only copy state for features that are enabled for the guest. The
> * state itself isn't problematic, but setting bits in the header for
> @@ -5571,22 +5584,27 @@ static void kvm_vcpu_ioctl_x86_get_xsave2(struct kvm_vcpu *vcpu,
> XFEATURE_MASK_FPSSE;
>
> if (fpstate_is_confidential(&vcpu->arch.guest_fpu))
> - return;
> + return 0;
>
> fpu_copy_guest_fpstate_to_uabi(&vcpu->arch.guest_fpu, state, size,
> supported_xcr0, vcpu->arch.pkru);
> + return 0;
> }
>
> -static void kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu,
> - struct kvm_xsave *guest_xsave)
> +static int kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu,
> + struct kvm_xsave *guest_xsave)
> {
> - kvm_vcpu_ioctl_x86_get_xsave2(vcpu, (void *)guest_xsave->region,
> - sizeof(guest_xsave->region));
> + return kvm_vcpu_ioctl_x86_get_xsave2(vcpu, (void *)guest_xsave->region,
> + sizeof(guest_xsave->region));
> }
>
> static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
> struct kvm_xsave *guest_xsave)
> {
> + if (vcpu->kvm->arch.has_protected_state &&
> + fpstate_is_confidential(&vcpu->arch.guest_fpu))
> + return -EINVAL;
> +
> if (fpstate_is_confidential(&vcpu->arch.guest_fpu))
> return 0;
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?
Also, do you happen to have a pointer to the WIP QEMU patches? Happy to
help with posting/testing those since we'll need similar for
SEV_INIT2-based SNP patches.
Thanks,
Mike
