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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c4adbc456e702b6e04b160efb996212fe3ee9d04.camel@kernel.org>
Date: Wed, 28 May 2025 14:49:07 +0200
From: Amit Shah <amit@...nel.org>
To: Tom Lendacky <thomas.lendacky@....com>, linux-kernel@...r.kernel.org, 
	kvm@...r.kernel.org, x86@...nel.org, linux-doc@...r.kernel.org
Cc: bp@...en8.de, tglx@...utronix.de, peterz@...radead.org,
 jpoimboe@...nel.org, 	pawan.kumar.gupta@...ux.intel.com, corbet@....net,
 mingo@...hat.com, 	dave.hansen@...ux.intel.com, hpa@...or.com,
 seanjc@...gle.com, pbonzini@...hat.com, 	daniel.sneddon@...ux.intel.com,
 kai.huang@...el.com, sandipan.das@....com, 	boris.ostrovsky@...cle.com,
 Babu.Moger@....com, david.kaplan@....com, 	dwmw@...zon.co.uk,
 andrew.cooper3@...rix.com, amit.shah@....com
Subject: Re: [PATCH v5 1/1] x86: kvm: svm: set up ERAPS support for guests

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