Re: [PULL 39/42] i386: Add support for SUCCOR feature

2024-06-27 Thread Paolo Bonzini 

On 6/13/24 11:50, Xiaoyao Li wrote:

On 6/8/2024 4:34 PM, Paolo Bonzini wrote:

From: John Allen 

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 
Reviewed-by: Joao Martins 
Signed-off-by: John Allen 
Message-ID: <20240603193622.47156-3-john.al...@amd.com>
Signed-off-by: Paolo Bonzini 


[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 == 0x8007 && 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

Re: [PULL 39/42] i386: Add support for SUCCOR feature

2024-06-24 Thread John Allen 
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 
> > 
> > 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 
> > Reviewed-by: Joao Martins 
> > Signed-off-by: John Allen 
> > Message-ID: <20240603193622.47156-3-john.al...@amd.com>
> > Signed-off-by: Paolo Bonzini 
> 
> [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 == 0x8007 && 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-e8ad2a7e4...@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
>

Re: [PULL 39/42] i386: Add support for SUCCOR feature

2024-06-13 Thread Xiaoyao Li 

On 6/8/2024 4:34 PM, Paolo Bonzini wrote:

From: John Allen 

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 
Reviewed-by: Joao Martins 
Signed-off-by: John Allen 
Message-ID: <20240603193622.47156-3-john.al...@amd.com>
Signed-off-by: Paolo Bonzini 


[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 == 0x8007 && 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

[PULL 39/42] i386: Add support for SUCCOR feature

2024-06-08 Thread Paolo Bonzini 
From: John Allen 

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 
Reviewed-by: Joao Martins 
Signed-off-by: John Allen 
Message-ID: <20240603193622.47156-3-john.al...@amd.com>
Signed-off-by: Paolo Bonzini 
---
 target/i386/cpu.h |  4 
 target/i386/cpu.c | 18 +-
 target/i386/kvm/kvm.c |  2 ++
 3 files changed, 23 insertions(+), 1 deletion(-)

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 = 0x8007, .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 0x8007:
 *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 == 0x8007 && 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
-- 
2.45.1