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

[Andrew Cooper <amc96@xxxxxxxx>]

On 25/11/2021 09:12, Roger Pau Monné wrote:
> On Wed, Nov 24, 2021 at 09:11:52PM +0000, 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.
> I don't think they will be silently discarded. xc_cpuid_xend_policy
> will attempt to get the CPUID leaf from the current/host/default
> policies and fail because the leaf couldn't be found.

Hmm yes.  Now I look at the code again, the error handling is plumbed up
fully.

So yes - there will be a rejection now where previously it worked.

> There's an issue with callers of xc_cpuid_apply_policy that pass a
> featureset (which is not used by the upstream tools) as in that case
> the featureset is translated to a CPUID policy without checking that
> the set features are correctly exposed regarding the maximum leaves
> set in CPUID, and thus features could be silently dropped, but as said
> that's not used by the `cpuid=` config file option.

Good catch.  I'd neglected to consider this aspect.

> This possibly needs fixing anyway after the release, in order to
> assert that the bits set in the featureset are correctly exposed given
> the max leaves reported in the policy.
>
> I think the above paragraph should be rewored as:
>
> "Furthermore, shrinking of the default policies also breaks things in
> some cases, because certain cpuid= settings in a VM config file which
> used to work will now be refused. Also external toolstacks that
> attempt to set the CPUID policy from a featureset might now see some
> filled leaves not reachable due to the shrinking done to the default
> domain policy before applying the featureset."
>
>> 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>
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>
> Likely with the paragraph adjusted if agreed.

Ok.

~Andrew