| Message ID | 20240608083415.2769160-40-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/42] target/i386: fix pushed value of EFLAGS.RF | expand |
On 6/8/2024 4:34 PM, Paolo Bonzini wrote: > From: John Allen <john.allen@amd.com> > > Add cpuid bit definition for the SUCCOR feature. This cpuid bit is required to > be exposed to guests to allow them to handle machine check exceptions on AMD > hosts. > > ---- > v2: > - Add "succor" feature word. > - Add case to kvm_arch_get_supported_cpuid for the SUCCOR feature. > > Reported-by: William Roche <william.roche@oracle.com> > Reviewed-by: Joao Martins <joao.m.martins@oracle.com> > Signed-off-by: John Allen <john.allen@amd.com> > Message-ID: <20240603193622.47156-3-john.allen@amd.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> [snip] ... > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > index 55a9e8a70cf..56d8e2a89ec 100644 > --- a/target/i386/kvm/kvm.c > +++ b/target/i386/kvm/kvm.c > @@ -532,6 +532,8 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, > */ > cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX); > ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES; > + } else if (function == 0x80000007 && reg == R_EBX) { > + ret |= CPUID_8000_0007_EBX_SUCCOR; IMO, this is incorrect. Why make it supported unconditionally? Shouldn't there be a KVM patch to report it in KVM_GET_SUPPORTED_CPUID? If make it supported unconditionally, all VMs boot with "-cpu host/max" will see the CPUID bits, even if it is Intel VMs. > } else if (function == KVM_CPUID_FEATURES && reg == R_EAX) { > /* kvm_pv_unhalt is reported by GET_SUPPORTED_CPUID, but it can't > * be enabled without the in-kernel irqchip
On Thu, Jun 13, 2024 at 05:50:08PM +0800, Xiaoyao Li wrote: > On 6/8/2024 4:34 PM, Paolo Bonzini wrote: > > From: John Allen <john.allen@amd.com> > > > > Add cpuid bit definition for the SUCCOR feature. This cpuid bit is required to > > be exposed to guests to allow them to handle machine check exceptions on AMD > > hosts. > > > > ---- > > v2: > > - Add "succor" feature word. > > - Add case to kvm_arch_get_supported_cpuid for the SUCCOR feature. > > > > Reported-by: William Roche <william.roche@oracle.com> > > Reviewed-by: Joao Martins <joao.m.martins@oracle.com> > > Signed-off-by: John Allen <john.allen@amd.com> > > Message-ID: <20240603193622.47156-3-john.allen@amd.com> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > [snip] > ... > > > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > > index 55a9e8a70cf..56d8e2a89ec 100644 > > --- a/target/i386/kvm/kvm.c > > +++ b/target/i386/kvm/kvm.c > > @@ -532,6 +532,8 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, > > */ > > cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX); > > ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES; > > + } else if (function == 0x80000007 && reg == R_EBX) { > > + ret |= CPUID_8000_0007_EBX_SUCCOR; > > IMO, this is incorrect. > > Why make it supported unconditionally? Shouldn't there be a KVM patch to > report it in KVM_GET_SUPPORTED_CPUID? > > If make it supported unconditionally, all VMs boot with "-cpu host/max" will > see the CPUID bits, even if it is Intel VMs. Paolo, This change in kvm_arch_get_supported_cpuid was added based on your suggestion here: https://lore.kernel.org/all/d4c1bb9b-8438-ed00-c79d-e8ad2a7e4eed@redhat.com/ Is there something missing from the patch needed to prevent the bits from being seen on Intel VMs? I am planning to send a patch early this week to report the bits for KVM and a patch that removes the above change for qemu. Is there another way you would prefer to handle it? Thanks, John > > > > } else if (function == KVM_CPUID_FEATURES && reg == R_EAX) { > > /* kvm_pv_unhalt is reported by GET_SUPPORTED_CPUID, but it can't > > * be enabled without the in-kernel irqchip >
On 6/13/24 11:50, Xiaoyao Li wrote: > On 6/8/2024 4:34 PM, Paolo Bonzini wrote: >> From: John Allen <john.allen@amd.com> >> >> Add cpuid bit definition for the SUCCOR feature. This cpuid bit is >> required to >> be exposed to guests to allow them to handle machine check exceptions >> on AMD >> hosts. >> >> ---- >> v2: >> - Add "succor" feature word. >> - Add case to kvm_arch_get_supported_cpuid for the SUCCOR feature. >> >> Reported-by: William Roche <william.roche@oracle.com> >> Reviewed-by: Joao Martins <joao.m.martins@oracle.com> >> Signed-off-by: John Allen <john.allen@amd.com> >> Message-ID: <20240603193622.47156-3-john.allen@amd.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > [snip] > ... > >> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c >> index 55a9e8a70cf..56d8e2a89ec 100644 >> --- a/target/i386/kvm/kvm.c >> +++ b/target/i386/kvm/kvm.c >> @@ -532,6 +532,8 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, >> uint32_t function, >> */ >> cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX); >> ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES; >> + } else if (function == 0x80000007 && reg == R_EBX) { >> + ret |= CPUID_8000_0007_EBX_SUCCOR; > > IMO, this is incorrect. > > Why make it supported unconditionally? Shouldn't there be a KVM patch to > report it in KVM_GET_SUPPORTED_CPUID? Yes, but since the bit doesn't need any hypervisor support it is common to also add it in QEMU. > If make it supported unconditionally, all VMs boot with "-cpu host/max" > will see the CPUID bits, even if it is Intel VMs. It should be harmless, but you're right, it's not tidy and we don't know for sure that it doesn't trigger weird paths in guest OSes. I've send a series to fix this. Paolo
diff --git a/target/i386/cpu.h b/target/i386/cpu.h index e6d5d1b483c..6786055ec6b 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -630,6 +630,7 @@ typedef enum FeatureWord { FEAT_7_1_EAX, /* CPUID[EAX=7,ECX=1].EAX */ FEAT_8000_0001_EDX, /* CPUID[8000_0001].EDX */ FEAT_8000_0001_ECX, /* CPUID[8000_0001].ECX */ + FEAT_8000_0007_EBX, /* CPUID[8000_0007].EBX */ FEAT_8000_0007_EDX, /* CPUID[8000_0007].EDX */ FEAT_8000_0008_EBX, /* CPUID[8000_0008].EBX */ FEAT_8000_0021_EAX, /* CPUID[8000_0021].EAX */ @@ -982,6 +983,9 @@ uint64_t x86_cpu_get_supported_feature_word(FeatureWord w, /* Packets which contain IP payload have LIP values */ #define CPUID_14_0_ECX_LIP (1U << 31) +/* RAS Features */ +#define CPUID_8000_0007_EBX_SUCCOR (1U << 1) + /* CLZERO instruction */ #define CPUID_8000_0008_EBX_CLZERO (1U << 0) /* Always save/restore FP error pointers */ diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 383230fa479..c5a532a254e 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -1180,6 +1180,22 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = { .tcg_features = TCG_APM_FEATURES, .unmigratable_flags = CPUID_APM_INVTSC, }, + [FEAT_8000_0007_EBX] = { + .type = CPUID_FEATURE_WORD, + .feat_names = { + NULL, "succor", NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, + .cpuid = { .eax = 0x80000007, .reg = R_EBX, }, + .tcg_features = 0, + .unmigratable_flags = 0, + }, [FEAT_8000_0008_EBX] = { .type = CPUID_FEATURE_WORD, .feat_names = { @@ -6887,7 +6903,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, break; case 0x80000007: *eax = 0; - *ebx = 0; + *ebx = env->features[FEAT_8000_0007_EBX]; *ecx = 0; *edx = env->features[FEAT_8000_0007_EDX]; break; diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index 55a9e8a70cf..56d8e2a89ec 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -532,6 +532,8 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, */ cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX); ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES; + } else if (function == 0x80000007 && reg == R_EBX) { + ret |= CPUID_8000_0007_EBX_SUCCOR; } else if (function == KVM_CPUID_FEATURES && reg == R_EAX) { /* kvm_pv_unhalt is reported by GET_SUPPORTED_CPUID, but it can't * be enabled without the in-kernel irqchip