+x86 qemu folks On 06/07/2023 21:22, Moger, Babu wrote: > Hi John, > Thanks for the patches. Few comments below. > > On 7/6/23 14:40, John Allen wrote: >> 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. >> >> Reported-by: William Roche <william.ro...@oracle.com> >> Signed-off-by: John Allen <john.al...@amd.com> >> --- >> target/i386/cpu.c | 2 +- >> target/i386/cpu.h | 4 ++++ >> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index 06009b80e8..09fae9337a 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -5874,7 +5874,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] | >> CPUID_8000_0007_EBX_SUCCOR; > > This is adding this feature unconditionally which does not seem right. > Couple of things. > 1. Add the feature word for SUCCOR. Users can add this feature using the > feature word "+succor". >
The feature requires no specific SVM/KVM support, it's really just a bit enumerated to the guest that's used to tell whether AMD MCE handling is supported, instead of blantly crashing outright. So I think the thinking here is that GET_SUPPORTED_CPUID won't return SUCCOR and the feature gets filtered out if it's declared as a regular named feature like you do in the CPU models. Making it relevant for any hypervisor kernel. > 2. Also define CPUID_8000_0007_EBX_SUCCOR : In this case, we can add this > feature as part of the EPYC Model update. > how would you set CPUID regardless of the hypervisor enumerates should it not require active emulation by the host? I am sort of interested on what is the right way to do this, as it would be better to do this regardless of the gets advertised by ioctls alike. > Thanks > Babu >
