Re: [PATCH v5 1/1] x86: kvm: svm: set up ERAPS support for guests

[Amit Shah <amit@kernel.org>]

On Mon, 2025-05-19 at 16:22 -0500, Tom Lendacky wrote:
> On 5/15/25 10:26, Amit Shah wrote:
> 

[...]

> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 571c906ffcbf..0cca1865826e 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -1187,6 +1187,9 @@ void kvm_set_cpu_caps(void)
> >  		F(SRSO_USER_KERNEL_NO),
> >  	);
> >  
> > +	if (tdp_enabled)
> > +		kvm_cpu_cap_check_and_set(X86_FEATURE_ERAPS);
> 
> Should this be moved to svm_set_cpu_caps() ? And there it can be
> 
> 	if (npt_enabled)
> 		kvm_cpu_cap...

Yea, I don't mind moving that to svm-only code.  Will do.

> >  	case 0x80000021:
> > -		entry->ebx = entry->ecx = entry->edx = 0;
> > +		entry->ecx = entry->edx = 0;
> >  		cpuid_entry_override(entry, CPUID_8000_0021_EAX);
> > +		if (kvm_cpu_cap_has(X86_FEATURE_ERAPS))
> > +			entry->ebx &= GENMASK(23, 16);
> > +		else
> > +			entry->ebx = 0;
> > +
> 
> Extra blank line.

Hm, helps with visual separation of the if-else and the break.  I
prefer to keep it, unless it breaks style guidelines.

> >  		break;
> >  	/* AMD Extended Performance Monitoring and Debug */
> >  	case 0x80000022: {
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index a89c271a1951..a2b075ed4133 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -1363,6 +1363,9 @@ static void init_vmcb(struct kvm_vcpu *vcpu)
> >  	if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL))
> >  		set_msr_interception(vcpu, svm->msrpm,
> > MSR_IA32_SPEC_CTRL, 1, 1);
> >  
> > +	if (boot_cpu_has(X86_FEATURE_ERAPS) && npt_enabled)
> 
> Should this be:
> 
> 	if (kvm_cpu_cap_has(X86_FEATURE_ERAPS))
> 
> ?

Indeed this is better.  There was some case I wanted to cover
initially, but I don't think it needs to only depend on the host caps
in the current version at least.

[...]
 
> > +static inline void vmcb_set_flush_guest_rap(struct vmcb *vmcb)
> > +{
> > +	vmcb->control.erap_ctl |= ERAP_CONTROL_FLUSH_RAP;
> > +}
> > +
> > +static inline void vmcb_clr_flush_guest_rap(struct vmcb *vmcb)
> > +{
> > +	vmcb->control.erap_ctl &= ~ERAP_CONTROL_FLUSH_RAP;
> > +}
> > +
> > +static inline void vmcb_enable_extended_rap(struct vmcb *vmcb)
> 
> s/extended/larger/ to match the bit name ?

I also prefer it with the "larger" name, but that is a confusing bit
name -- so after the last round of review, I renamed the accessor
functions to be "better", while leaving the bit defines match what the
CPU has.

I don't mind switching this back - anyone else have any other opinions?

> 
> > +{
> > +	vmcb->control.erap_ctl |= ERAP_CONTROL_ALLOW_LARGER_RAP;
> > +}
> > +
> > +static inline bool vmcb_is_extended_rap(struct vmcb *vmcb)
> 
> s/is_extended/has_larger/
> 
> Thanks,
> Tom

Thanks for the review!

		Amit