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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.05.2023 14:12, Alejandro Vallejo wrote:
> 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.

I think I would be okay if that was (in perhaps slightly abridged form)
made part of the description (or if the code comment there said so, then
also preventing someone [like me] coming and re-ordering the conditional).

Nevertheless having raw_policy populated like this seems a little fragile
in the first place. Andrew - any particular thoughts from you in this
regard?

Jan