Re: [PATCH v2 3/3] x86: Add support for CpuidUserDis

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Thu, May 11, 2023 at 01:05:42PM +0200, Jan Beulich wrote:
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > @@ -279,8 +279,12 @@ static void __init noinline amd_init_levelling(void)
> >      * that can only be present when Xen is itself virtualized (because
> >      * it can be emulated)
> >      */
> > -   if (cpu_has_hypervisor && probe_cpuid_faulting())
> > +   if ((cpu_has_hypervisor && probe_cpuid_faulting()) ||
> > +       boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)) {
> 
> ... imo the probe_cpuid_faulting() call would better be avoided when
> the CPUID bit is set.

I wrote it like that originally. However, it felt wrong to leave
raw_policy.platform_info unset, as it's set inside probe_cpuid_faulting().
While it's highly unlikely a real AMD machine will have CPUID faulting
support, Xen might see both if it's itself virtualized under Xen.

The crux of the matter here is whether we want the raw policy to be an
accurate representation of _all_ the features of the machine (real or
virtual) or we're ok with it not having features we don't intend to use in
practice. It certainly can be argued either way. CpuidUserDis naturally
gets to the policy through CPUID leaf enumeration, so that's done
regardless.

My .02 is that raw means uncooked and as such should have the actual
physical features reported by the machine, but I could be persuaded either
way.

> 
> > +           expected_levelling_cap |= LCAP_faulting;
> > +           levelling_caps |=  LCAP_faulting;
> 
> Further the movement of these two lines from ...
> 
> > @@ -144,8 +145,6 @@ bool __init probe_cpuid_faulting(void)
> >             return false;
> >     }
> >  
> > -   expected_levelling_cap |= LCAP_faulting;
> > -   levelling_caps |=  LCAP_faulting;
> >     setup_force_cpu_cap(X86_FEATURE_CPUID_FAULTING);
> 
> ... here (and also to intel.c) should imo be part of patch 2. While
> moving them, I think you also want to deal with the stray double
> blank.
> 
> Jan

Sure.

Alejandro