Re: [PATCH] i386/sev: Don't allow automatic fallback to legacy KVM_SEV*_INIT

[Paolo Bonzini]

On Thu, Jul 4, 2024 at 2:01 AM Michael Roth <michael.roth@amd.com> wrote:
> Currently if the 'legacy-vm-type' property of the sev-guest object is
> left unset, QEMU will attempt to use the newer KVM_SEV_INIT2 kernel
> interface in conjunction with the newer KVM_X86_SEV_VM and
> KVM_X86_SEV_ES_VM KVM VM types.
>
> This can lead to measurement changes if, for instance, an SEV guest was
> created on a host that originally had an older kernel that didn't
> support KVM_SEV_INIT2, but is booted on the same host later on after the
> host kernel was upgraded.

I think this is the right thing to do for SEV-ES. I agree that it's
bad to require a very new kernel (6.10 will be released only a month
before QEMU 9.1), on the other hand the KVM_SEV_ES_INIT API is broken
in several ways. As long as there is a way to go back to it, and it's
not changed by old machine types, not using it for SEV-ES is the
better choice for upstream.

On the other hand, I think it makes no difference for SEV?  Should we
always use KVM_SEV_INIT, or alternatively fall back as it was before
this patch?

Paolo

> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> cc: kvm@vger.kernel.org
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>  qapi/qom.json     | 11 ++++++-----
>  target/i386/sev.c | 30 ++++++++++++++++++++++++------
>  2 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 8bd299265e..a212c009aa 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -912,11 +912,12 @@
>  # @handle: SEV firmware handle (default: 0)
>  #
>  # @legacy-vm-type: Use legacy KVM_SEV_INIT KVM interface for creating the VM.
> -#                  The newer KVM_SEV_INIT2 interface syncs additional vCPU
> -#                  state when initializing the VMSA structures, which will
> -#                  result in a different guest measurement. Set this to
> -#                  maintain compatibility with older QEMU or kernel versions
> -#                  that rely on legacy KVM_SEV_INIT behavior.
> +#                  The newer KVM_SEV_INIT2 interface, from Linux >= 6.10, 
> syncs
> +#                  additional vCPU state when initializing the VMSA 
> structures,
> +#                  which will result in a different guest measurement. Set
> +#                  this to force compatibility with older QEMU or kernel
> +#                  versions that rely on legacy KVM_SEV_INIT behavior.
> +#                  Otherwise, QEMU will require KVM_SEV_INIT2 for SEV guests.
>  #                  (default: false) (since 9.1)
>  #
>  # Since: 2.12
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 3ab8b3c28b..8f56c0cf0c 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -1347,14 +1347,22 @@ static int sev_kvm_type(X86ConfidentialGuest *cg)
>          goto out;
>      }
>
> +    if (sev_guest->legacy_vm_type) {
> +        sev_common->kvm_type = KVM_X86_DEFAULT_VM;
> +        goto out;
> +    }
> +
>      kvm_type = (sev_guest->policy & SEV_POLICY_ES) ?
>                  KVM_X86_SEV_ES_VM : KVM_X86_SEV_VM;
> -    if (kvm_is_vm_type_supported(kvm_type) && !sev_guest->legacy_vm_type) {
> -        sev_common->kvm_type = kvm_type;
> -    } else {
> -        sev_common->kvm_type = KVM_X86_DEFAULT_VM;
> +    if (!kvm_is_vm_type_supported(kvm_type)) {
> +            error_report("SEV: host kernel does not support requested %s VM 
> type. To allow use of "
> +                         "legacy KVM_X86_DEFAULT_VM VM type, the 
> 'legacy-vm-type' argument must be "
> +                         "set to true for the sev-guest object.",
> +                         kvm_type == KVM_X86_SEV_VM ? "KVM_X86_SEV_VM" : 
> "KVM_X86_SEV_ES_VM");
> +            return -1;
>      }
>
> +    sev_common->kvm_type = kvm_type;
>  out:
>      return sev_common->kvm_type;
>  }
> @@ -1445,14 +1453,24 @@ static int 
> sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      }
>
>      trace_kvm_sev_init();
> -    if (x86_klass->kvm_type(X86_CONFIDENTIAL_GUEST(sev_common)) == 
> KVM_X86_DEFAULT_VM) {
> +    switch (x86_klass->kvm_type(X86_CONFIDENTIAL_GUEST(sev_common))) {
> +    case KVM_X86_DEFAULT_VM:
>          cmd = sev_es_enabled() ? KVM_SEV_ES_INIT : KVM_SEV_INIT;
>
>          ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, &fw_error);
> -    } else {
> +        break;
> +    case KVM_X86_SEV_VM:
> +    case KVM_X86_SEV_ES_VM:
> +    case KVM_X86_SNP_VM: {
>          struct kvm_sev_init args = { 0 };
>
>          ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_INIT2, &args, &fw_error);
> +        break;
> +    }
> +    default:
> +        error_setg(errp, "%s: host kernel does not support the requested SEV 
> configuration.",
> +                   __func__);
> +        return -1;
>      }
>
>      if (ret) {
> --
> 2.25.1
>