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
