[RFC PATCH 0/2] target/i386: SEV: allow running SNP guests with "-cpu host"

[RFC PATCH 0/2] target/i386: SEV: allow running SNP guests with "-cpu host"

[Paolo Bonzini]

Some CPUID features may be provided by KVM for some guests, independent of
processor support, for example TSC deadline or TSC adjust.  They are not going
to be present in named models unless the vendor implements them in hardware,
but they will be present in "-cpu host".

If these bits are not supported by the confidential computing firmware,
however, the guest will fail to start, and indeed this is a problem when
you run SNP guests with "-cpu host".  This series fixes the issue.

However, I am marking this as RFC because it's not future proof.
If in the future AMD processors do provide any of these bits, this is
going to break (tsc_deadline and tsc_adjust are the most likely one).
Including the bits if they are present in host CPUID is not super safe
either, since the firmware might not be updated to follow suit.

Michael, any ideas?  Is there a way for the host to retrieve the supported
CPUID bits for SEV-SNP guests?

One possibility is to set up a fake guest---either in QEMU or when KVM
starts---to do a LAUNCH_UPDATE for the CPUID page, but even that is not
perfect.  For example, I got

 > function 0x7, index: 0x0 provided: edx: 0xbc000010, expected: edx: 0x00000000

even though the FSRM bit (0x10) is supported.  That might be just a
firmware bug however.

Paolo

Based-on: <20240627140628.1025317-1-pbonzini@redhat.com>

Paolo Bonzini (4):
  target/i386: add support for masking CPUID features in confidential
    guests
  target/i386/SEV: implement mask_cpuid_features

 target/i386/confidential-guest.h | 24 ++++++++++++++++++++++++
 target/i386/cpu.c                |  9 +++++++++
 target/i386/cpu.h                |  4 ++++
 target/i386/kvm/kvm.c            |  5 +++++
 target/i386/sev.c                | 33 +++++++++++++++++++++++++++++++++
 5 files changed, 75 insertions(+)

-- 
2.45.2

[RFC PATCH 1/2] target/i386: add support for masking CPUID features in confidential guests

[Paolo Bonzini]

Some CPUID features may be provided by KVM for some guests, independent of
processor support, for example TSC deadline or TSC adjust.  If these are
not supported by the confidential computing firmware, however, the guest
will fail to start.  Add support for removing unsupported features from
"-cpu host".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/confidential-guest.h | 24 ++++++++++++++++++++++++
 target/i386/cpu.c                |  9 +++++++++
 target/i386/kvm/kvm.c            |  5 +++++
 3 files changed, 38 insertions(+)

diff --git a/target/i386/confidential-guest.h b/target/i386/confidential-guest.h
index 532e172a60b..7342d2843aa 100644
--- a/target/i386/confidential-guest.h
+++ b/target/i386/confidential-guest.h
@@ -39,6 +39,8 @@ struct X86ConfidentialGuestClass {
 
     /* <public> */
     int (*kvm_type)(X86ConfidentialGuest *cg);
+    uint32_t (*mask_cpuid_features)(X86ConfidentialGuest *cg, uint32_t feature, uint32_t index,
+                                    int reg, uint32_t value);
 };
 
 /**
@@ -56,4 +58,26 @@ static inline int x86_confidential_guest_kvm_type(X86ConfidentialGuest *cg)
         return 0;
     }
 }
+
+/**
+ * x86_confidential_guest_mask_cpuid_features:
+ *
+ * Removes unsupported features from a confidential guest's CPUID values, returns
+ * the value with the bits removed.  The bits removed should be those that KVM
+ * provides independent of host-supported CPUID features, but are not supported by
+ * the confidential computing firmware.
+ */
+static inline int x86_confidential_guest_mask_cpuid_features(X86ConfidentialGuest *cg,
+                                                             uint32_t feature, uint32_t index,
+                                                             int reg, uint32_t value)
+{
+    X86ConfidentialGuestClass *klass = X86_CONFIDENTIAL_GUEST_GET_CLASS(cg);
+
+    if (klass->mask_cpuid_features) {
+        return klass->mask_cpuid_features(cg, feature, index, reg, value);
+    } else {
+        return value;
+    }
+}
+
 #endif
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index dd8b0f33136..056d117cd11 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -548,6 +548,11 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
         ret |= 1U << KVM_HINTS_REALTIME;
     }
 
+    if (current_machine->cgs) {
+        ret = x86_confidential_guest_mask_cpuid_features(
+            X86_CONFIDENTIAL_GUEST(current_machine->cgs),
+            function, index, reg, ret);
+    }
     return ret;
 }
 
-- 
2.45.2

[RFC PATCH 2/2] target/i386/SEV: implement mask_cpuid_features

[Paolo Bonzini]

SNP firmware rejects several features that KVM implements without needing
hardware support.  If these are specified, for example with "-cpu host",
the guest will fail to start.

I am marking this as RFC because it's not future proof.  If in the future
AMD processors do provide any of these bits, this is going to break
(tsc_deadline and tsc_adjust are the most likely one).  Including the
bits if they are present in host CPUID is not super safe either, since
the firmware might not be updated to follow suit.

Reported-by: Zixi Chen <zixchen@redhat.com>
Not-quite-signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/cpu.h |  4 ++++
 target/i386/sev.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 9bea7142bf4..31c9b43849e 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -812,6 +812,8 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w);
 
 /* Support RDFSBASE/RDGSBASE/WRFSBASE/WRGSBASE */
 #define CPUID_7_0_EBX_FSGSBASE          (1U << 0)
+/* Support TSC adjust MSR */
+#define CPUID_7_0_EBX_TSC_ADJUST        (1U << 1)
 /* Support SGX */
 #define CPUID_7_0_EBX_SGX               (1U << 2)
 /* 1st Group of Advanced Bit Manipulation Extensions */
@@ -1002,6 +1004,8 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w);
 #define CPUID_8000_0008_EBX_STIBP_ALWAYS_ON    (1U << 17)
 /* Speculative Store Bypass Disable */
 #define CPUID_8000_0008_EBX_AMD_SSBD    (1U << 24)
+/* Paravirtualized Speculative Store Bypass Disable MSR */
+#define CPUID_8000_0008_EBX_VIRT_SSBD   (1U << 25)
 /* Predictive Store Forwarding Disable */
 #define CPUID_8000_0008_EBX_AMD_PSFD    (1U << 28)
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 2a0f94d390d..280eaef8723 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -945,6 +945,38 @@ out:
     return ret;
 }
 
+static uint32_t
+sev_snp_mask_cpuid_features(X86ConfidentialGuest *cg, uint32_t feature, uint32_t index,
+                            int reg, uint32_t value)
+{
+    switch (feature) {
+    case 1:
+        if (reg == R_ECX) {
+            return value & ~CPUID_EXT_TSC_DEADLINE_TIMER;
+        }
+        break;
+    case 7:
+        if (index == 0 && reg == R_EBX) {
+            return value & ~CPUID_7_0_EBX_TSC_ADJUST;
+        }
+        if (index == 0 && reg == R_EDX) {
+            return value & ~(CPUID_7_0_EDX_SPEC_CTRL |
+                             CPUID_7_0_EDX_STIBP |
+                             CPUID_7_0_EDX_FLUSH_L1D |
+                             CPUID_7_0_EDX_ARCH_CAPABILITIES |
+                             CPUID_7_0_EDX_CORE_CAPABILITY |
+                             CPUID_7_0_EDX_SPEC_CTRL_SSBD);
+        }
+        break;
+    case 0x80000008:
+        if (reg == R_EBX) {
+            return value & ~CPUID_8000_0008_EBX_VIRT_SSBD;
+        }
+        break;
+    }
+    return value;
+}
+
 static int
 sev_launch_update_data(SevCommonState *sev_common, hwaddr gpa,
                        uint8_t *addr, size_t len)
@@ -2315,6 +2347,7 @@ sev_snp_guest_class_init(ObjectClass *oc, void *data)
     klass->launch_finish = sev_snp_launch_finish;
     klass->launch_update_data = sev_snp_launch_update_data;
     klass->kvm_init = sev_snp_kvm_init;
+    x86_klass->mask_cpuid_features = sev_snp_mask_cpuid_features;
     x86_klass->kvm_type = sev_snp_kvm_type;
 
     object_class_property_add(oc, "policy", "uint64",
-- 
2.45.2

Re: [RFC PATCH 0/2] target/i386: SEV: allow running SNP guests with "-cpu host"

[Michael Roth]

On Wed, Jul 03, 2024 at 01:01:32PM +0200, Paolo Bonzini wrote:
> Some CPUID features may be provided by KVM for some guests, independent of
> processor support, for example TSC deadline or TSC adjust.  They are not going
> to be present in named models unless the vendor implements them in hardware,
> but they will be present in "-cpu host".
> 
> If these bits are not supported by the confidential computing firmware,
> however, the guest will fail to start, and indeed this is a problem when
> you run SNP guests with "-cpu host".  This series fixes the issue.
> 
> However, I am marking this as RFC because it's not future proof.
> If in the future AMD processors do provide any of these bits, this is
> going to break (tsc_deadline and tsc_adjust are the most likely one).
> Including the bits if they are present in host CPUID is not super safe
> either, since the firmware might not be updated to follow suit.
> 
> Michael, any ideas?  Is there a way for the host to retrieve the supported
> CPUID bits for SEV-SNP guests?

If we want to support -cpu host, then I don't really see a way around
needing to maintain a filter of some sort sanitize what gets passed to
firmware. Generally, every new CPU model is likely to have some features
which might be a liability security-wise to allow in SNP guests, so the
CPUID validation is sort of a whitelist of curated features that make
sense for guests and can be enabled securely in the context of SNP.

Everything else would need to be filtered out, so we'd need to keep that
list constantly updated.

I think that may be possible, but do we have a strong use-case for
supporting -cpu host in conjunction with SNP guests that this would be
a worthwhile endeavor?

> 
> One possibility is to set up a fake guest---either in QEMU or when KVM
> starts---to do a LAUNCH_UPDATE for the CPUID page, but even that is not
> perfect.  For example, I got

Yah, the firmware-provided responses are more of a debug tool and not
something I think we can rely on to enumerate capabilities.

You could in theory take the ruleset in the PPR (Chapter 2, CPUID Policy
Enforcement), turn that into something programmatic, and apply that
against the host's CPUID values, but the policies are a bit more
specific in some cases, and the PPR is per-CPU-model so both the rules
and inputs can change from one host to the next.

So I don't see a great way to leverage that to make things easier here.
The manually-maintained filter you've proposed here seems more reliable
to me.

> 
>  > function 0x7, index: 0x0 provided: edx: 0xbc000010, expected: edx: 0x00000000
> 
> even though the FSRM bit (0x10) is supported.  That might be just a
> firmware bug however.

That's a possibility. It seems like 'BitMask' fields (as documented in
the PPR section "CPUID Policy Enforcement") generally return the AND of
what the host supports with what is passed in. I'll look into this a
bit more and raise a ticket with firmware folks if it is unexpected.

Thanks,

Mike

> 
> Paolo
> 
> Based-on: <20240627140628.1025317-1-pbonzini@redhat.com>
> 
> Paolo Bonzini (4):
>   target/i386: add support for masking CPUID features in confidential
>     guests
>   target/i386/SEV: implement mask_cpuid_features
> 
>  target/i386/confidential-guest.h | 24 ++++++++++++++++++++++++
>  target/i386/cpu.c                |  9 +++++++++
>  target/i386/cpu.h                |  4 ++++
>  target/i386/kvm/kvm.c            |  5 +++++
>  target/i386/sev.c                | 33 +++++++++++++++++++++++++++++++++
>  5 files changed, 75 insertions(+)
> 
> -- 
> 2.45.2
> 

Re: [RFC PATCH 0/2] target/i386: SEV: allow running SNP guests with "-cpu host"

[Paolo Bonzini]

On Thu, Jul 4, 2024 at 2:26 AM Michael Roth <michael.roth@amd.com> wrote:
> > Michael, any ideas?  Is there a way for the host to retrieve the supported
> > CPUID bits for SEV-SNP guests?
>
> If we want to support -cpu host, then I don't really see a way around
> needing to maintain a filter of some sort sanitize what gets passed to
> firmware. Generally, every new CPU model is likely to have some features
> which might be a liability security-wise to allow in SNP guests, so the
> CPUID validation is sort of a whitelist of curated features that make
> sense for guests and can be enabled securely in the context of SNP.
>
> Everything else would need to be filtered out, so we'd need to keep that
> list constantly updated.

It would be per new model and right now there are only a handful of
bits that have to be blocked; so it wouldn't be particularly bad.

> I think that may be possible, but do we have a strong use-case for
> supporting -cpu host in conjunction with SNP guests that this would be
> a worthwhile endeavor?

It's a common way to launch a guest if you're not interested in
migration (which is obviously the case for SNP right now), so it's
more like "why not". :)

> > One possibility is to set up a fake guest---either in QEMU or when KVM
> > starts---to do a LAUNCH_UPDATE for the CPUID page, but even that is not
> > perfect.  For example, I got
>
> Yah, the firmware-provided responses are more of a debug tool and not
> something I think we can rely on to enumerate capabilities.
>
> You could in theory take the ruleset in the PPR (Chapter 2, CPUID Policy
> Enforcement), turn that into something programmatic, and apply that
> against the host's CPUID values, but the policies are a bit more
> specific in some cases, and the PPR is per-CPU-model so both the rules
> and inputs can change from one host to the next.

Yeah, and if you mix that with knowledge of what KVM can/cannot
virtualize that doesn't exist in the processor (which isn't that
much), then you end up with something a lot like patch 2

It would be nice if the policy enforcement were changed to allow the
TSC deadline timer and X2APIC bits (you probably don't want TSC
adjust, that's the right call; and virt SSBD is not accessible because
you use V_SPEC_CTRL instead). But then there would be no way to find
out if the change actually happened.

> So I don't see a great way to leverage that to make things easier here.
> The manually-maintained filter you've proposed here seems more reliable
> to me.

Yep, I think I'll include that patch as the maintainability doesn't seem bad.

Paolo