Re: [PATCH 1/2] i386: Add support for SUCCOR feature

[Joao Martins <joao.m.martins@oracle.com>]

+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.roche@oracle.com>
>> Signed-off-by: John Allen <john.allen@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
>