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

[Xiaoyao Li <xiaoyao.li@intel.com>]

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