Re: [PATCH] KVM: x86: Advertise SUCCOR and OVERFLOW_RECOV cpuid bits

[John Allen]

On Tue, Jul 30, 2024 at 11:00:57AM -0700, Sean Christopherson wrote:
> On Tue, Jul 30, 2024, John Allen wrote:
> > Handling deferred, uncorrected MCEs on AMD guests is now possible with
> > additional support in qemu. Ensure that the SUCCOR and OVERFLOW_RECOV
> > bits are advertised to the guest in KVM.
> > 
> > Suggested-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Signed-off-by: John Allen <john.allen@xxxxxxx>
> > ---
> >  arch/x86/kvm/cpuid.c   | 2 +-
> >  arch/x86/kvm/svm/svm.c | 7 +++++++
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 2617be544480..4745098416c3 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -1241,7 +1241,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
> >  
> >  		/* mask against host */
> >  		entry->edx &= boot_cpu_data.x86_power;
> > -		entry->eax = entry->ebx = entry->ecx = 0;
> > +		entry->eax = entry->ecx = 0;
> 
> Needs an override to prevent reporting all of EBX to userspace.
> 
> 		cpuid_entry_override(entry, CPUID_8000_0007_EBX);

Right, I see what you mean. We just want to expose these specific bits
and not all of EBX. I think with the patch as it is along with the
change you suggest below, this should resolve this as the above case
already has the cpuid_entry_override just above where it cuts off. Or is
there another place we need it?

Thanks,
John

> 
> >  		break;
> >  	case 0x80000008: {
> >  		/*
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index c115d26844f7..a6820b0915db 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -5199,6 +5199,13 @@ static __init void svm_set_cpu_caps(void)
> >  		kvm_cpu_cap_set(X86_FEATURE_SVME_ADDR_CHK);
> >  	}
> >  
> > +	/* CPUID 0x80000007 */
> > +	if (boot_cpu_has(X86_FEATURE_SUCCOR))
> > +		kvm_cpu_cap_set(X86_FEATURE_SUCCOR);
> > +
> > +	if (boot_cpu_has(X86_FEATURE_OVERFLOW_RECOV))
> > +		kvm_cpu_cap_set(X86_FEATURE_OVERFLOW_RECOV);
> 
> This _could_ use kvm_cpu_cap_check_and_set(), but given that this an AMD specific
> leaf and unlikely to ever be used by Intel, I'm inclined to handle this in cpuid.c,
> with an opporunustic "conversion" to one feature per line[*]:
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 2617be544480..ea11a7e45174 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -743,6 +743,11 @@ void kvm_set_cpu_caps(void)
>         if (!tdp_enabled && IS_ENABLED(CONFIG_X86_64))
>                 kvm_cpu_cap_set(X86_FEATURE_GBPAGES);
>  
> +       kvm_cpu_cap_mask(CPUID_8000_0007_EBX,
> +               F(OVERFLOW_RECOV) |
> +               F(SUCCOR)
> +       );
> +
>         kvm_cpu_cap_init_kvm_defined(CPUID_8000_0007_EDX,
>                 SF(CONSTANT_TSC)
>         );
> 
> 
> [*] https://lore.kernel.org/all/ZoxooTvO5vIEnS5V@xxxxxxxxxx