Re: [PATCH for-4.16] Revert "x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2021 09:37, Roger Pau Monné wrote:
> On Fri, Nov 26, 2021 at 09:22:50AM +0100, Jan Beulich wrote:
>> On 25.11.2021 18:28, Andrew Cooper wrote:
>>> On 25/11/2021 10:43, Roger Pau Monné wrote:
>>>> On Thu, Nov 25, 2021 at 11:25:36AM +0100, Jan Beulich wrote:
>>>>> On 24.11.2021 22:11, Andrew Cooper wrote:
>>>>>> OSSTest has identified a 3rd regression caused by this change.  Migration
>>>>>> between Xen 4.15 and 4.16 on the nocera pair of machines (AMD Opteron 
>>>>>> 4133)
>>>>>> fails with:
>>>>>>
>>>>>>   xc: error: Failed to set CPUID policy: leaf 00000000, subleaf 
>>>>>> ffffffff, msr ffffffff (22 = Invalid argument): Internal error
>>>>>>   xc: error: Restore failed (22 = Invalid argument): Internal error
>>>>>>
>>>>>> which is a safety check to prevent resuming the guest when the CPUID 
>>>>>> data has
>>>>>> been truncated.  The problem is caused by shrinking of the max policies, 
>>>>>> which
>>>>>> is an ABI that needs handling compatibly between different versions of 
>>>>>> Xen.
>>>>>>
>>>>>> Furthermore, shrinking of the default policies also breaks things in some
>>>>>> cases, because certain cpuid= settings in a VM config file which used to 
>>>>>> have
>>>>>> an effect will now be silently discarded.
>>>>>>
>>>>>> This reverts commit 540d911c2813c3d8f4cdbb3f5672119e5e768a3d, as well as 
>>>>>> the
>>>>>> partial fix attempt in 81da2b544cbb003a5447c9b14d275746ad22ab37 (which 
>>>>>> added
>>>>>> one new case where cpuid= settings might not apply correctly) and 
>>>>>> restores the
>>>>>> same behaviour as Xen 4.15.
>>>>>>
>>>>>> Fixes: 540d911c2813 ("x86/CPUID: shrink max_{,sub}leaf fields according 
>>>>>> to actual leaf contents")
>>>>>> Fixes: 81da2b544cbb ("x86/cpuid: prevent shrinking migrated policies max 
>>>>>> leaves")
>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>> While not strictly needed with Roger having given his already,
>>>>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> to signal my (basic) agreement with the course of action taken.
>>>>> Nevertheless I fear this is going to become yet one more case where
>>>>> future action is promised, but things then die out.
>>>> I'm certainly happy to look at newer versions of this patch, but I
>>>> think we should consider doing the shrinking only on the toolstack
>>>> said, and only after all the manipulations on the policy have been
>>>> performed.
>>>
>>> Correct.
>>>
>>> The max policies cannot be shrunk - they are, by definition, the upper
>>> bounds that we audit against.  (More precisely, they must never end up
>>> lower than an older Xen used to offer on the same configuration, and
>>> must not be lower anything the user may opt in to.)
>>
>> I disagree: For one, the user cannot opt in to anything beyond max policy.
>> Or else that policy isn't "max" anymore. The user may opt in to a higher
>> than useful max (sub)leaf, but that's independent.
> 
> Wouldn't it be possible for Xen to offer certain features to guests
> that are not part of the native CPUID policy, and that thus might
> require setting bit(s) on leaves that could otherwise be empty? That
> also requires some explicit checks in Xen in order to assert that
> leaves above the max one are empty.

Extra available features imo still need representing in the max policies.

> TBH I'm not sure what's the benefit of shrinking the max policies, as
> those are not to be used as-is by guests. They are an upper bound, but
> should be tailored for each guest usage, and should be shrunk before
> being used by guests on a case-by-case basis (ie: by the toolstack).

Well, if on an AMX-capable system the admin passed "cpuid=no-amx-tile"
(or whatever better equivalent to mean "AMX as a whole"), I wouldn't
see why logic everywhere would need working with 30 leaves when 13
would do. But yes, beyond saving some effort the effect of shrinking
max leaves should be non-noticable in the guest visible result.

Anyway, the main reason I will want to continue carrying the patch in
some form is for the actual function, as that gets modified by later
patches. Where ultimately it gets called from is secondary from that
perspective.

Jan