| Message ID | 20240704000019.3928862-1-michael.roth@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | i386/sev: Don't allow automatic fallback to legacy KVM_SEV*_INIT | expand |
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 >
On Thu, Jul 04, 2024 at 08:51:05AM +0200, Paolo Bonzini wrote: > 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. Broken how ? I know there was the regression with the 'debug_swap' parameter, but was something that should just be fixed in the kernel, rather than breaking userspace. What else is a problem ? I don't think its reasonable for QEMU to require a brand new kernel for new machine types, given SEV & SEV-ES have been deployed for many years already. With regards, Daniel
On Thu, Jul 4, 2024 at 10:42 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Thu, Jul 04, 2024 at 08:51:05AM +0200, Paolo Bonzini wrote: > > 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. > > Broken how ? I know there was the regression with the 'debug_swap' > parameter, but was something that should just be fixed in the kernel, > rather than breaking userspace. What else is a problem ? The debug_swap parameter simply could not be enabled in the old API without breaking measurements. The new API *is the fix* to allow using it (though QEMU doesn't have the option plumbed in yet). There is no extensibility. Enabling debug_swap by default is also a thorny problem; it cannot be enabled by default because not all CPUs support it, and also we'd have the same problem that we cannot enable debug_swap on new machine types without requiring a new kernel. Tying the default to the -cpu model would work but it is confusing. But I guess we can add support for debug_swap, disabled by default and switch to the new API if debug_swap is enabled. > I don't think its reasonable for QEMU to require a brand new kernel > for new machine types, given SEV & SEV-ES have been deployed for > many years already. I think it's reasonable if the fix is displayed right into the error message. It's only needed for SEV-ES though, SEV can use the old and new APIs interchangeably. Paolo
On Thu, Jul 04, 2024 at 11:31:16AM +0200, Paolo Bonzini wrote: > On Thu, Jul 4, 2024 at 10:42 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Thu, Jul 04, 2024 at 08:51:05AM +0200, Paolo Bonzini wrote: > > > 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. > > > > Broken how ? I know there was the regression with the 'debug_swap' > > parameter, but was something that should just be fixed in the kernel, > > rather than breaking userspace. What else is a problem ? > > The debug_swap parameter simply could not be enabled in the old API > without breaking measurements. The new API *is the fix* to allow using > it (though QEMU doesn't have the option plumbed in yet). There is no > extensibility. > > Enabling debug_swap by default is also a thorny problem; it cannot be > enabled by default because not all CPUs support it, and also we'd have > the same problem that we cannot enable debug_swap on new machine types > without requiring a new kernel. Tying the default to the -cpu model > would work but it is confusing. Presumably we can tie it to '-cpu host' without much problem, and then just leave it as an opt-in feature flag for named CPU models. > But I guess we can add support for debug_swap, disabled by default and > switch to the new API if debug_swap is enabled. > > > I don't think its reasonable for QEMU to require a brand new kernel > > for new machine types, given SEV & SEV-ES have been deployed for > > many years already. > > I think it's reasonable if the fix is displayed right into the error > message. It's only needed for SEV-ES though, SEV can use the old and > new APIs interchangeably. FYI currently it is proposed to unconditionally force set legacy-vm-type=true in libvirt, so QEMU guests would *never* use the new ioctl, to fix what we consider to be a QEMU / kernel guest ABI regression. If libvirt adds this though, it is basically a forever setting we would never be able to remove as removing would break ABI compat again. https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/KP24LPFV4OCMN45TDHNXQVBPDZ56QSRR/ I'd much rather QEMU did NOT set this by default in its machine types, so libvirt doesn't have to add this forced flag. That way downstream distros who /can/ guarantee new enough kernels, can still use legacy-vm-type=false in their downstream machine types, without libvirt overriding this. With regards, Daniel
On Thu, Jul 4, 2024 at 11:39 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > The debug_swap parameter simply could not be enabled in the old API > > without breaking measurements. The new API *is the fix* to allow using > > it (though QEMU doesn't have the option plumbed in yet). There is no > > extensibility. > > > > Enabling debug_swap by default is also a thorny problem; it cannot be > > enabled by default because not all CPUs support it, and also we'd have > > the same problem that we cannot enable debug_swap on new machine types > > without requiring a new kernel. Tying the default to the -cpu model > > would work but it is confusing. > > Presumably we can tie it to '-cpu host' without much problem, and > then just leave it as an opt-in feature flag for named CPU models. '-cpu host' for SEV-SNP is also problematic, since CPUID is part of the measurement. It's okay for starting guests in a quick and dirty manner, but it cannot be used if measurement is in use. It's weird to have "-cpu" provide the default for "-object", since -object is created much earlier than CPUs. But then "-cpu" itself is weird because it's a kind of "factory" for future objects. Maybe we should redo the same exercise I did to streamline machine initialization, this time focusing on -cpu/-machine/-accel, to understand the various phases and where sev-{,snp-}guest initialization fits. > > I think it's reasonable if the fix is displayed right into the error > > message. It's only needed for SEV-ES though, SEV can use the old and > > new APIs interchangeably. > > FYI currently it is proposed to unconditionally force set legacy-vm-type=true > in libvirt, so QEMU guests would *never* use the new ioctl, to fix what we > consider to be a QEMU / kernel guest ABI regression. Ok, so it's probably best to apply both this and your patch for now. Later debug-swap can be enabled and will automatically disable legacy-vm-type if the user left it to the default. If you want to test this combo and send a pull request (either to me or to Richard), that would help because I'm mostly away for a few days. Paolo
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) {
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. 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(-)