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

[Paolo Bonzini <pbonzini@redhat.com>]

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