Re: [PATCH] x86/cpu-policy: Extend the guest max policy max leaf/subleaves

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.10.2024 18:10, Roger Pau Monné wrote:
> On Wed, Oct 30, 2024 at 04:51:34PM +0000, Andrew Cooper wrote:
>> On 30/10/2024 3:13 pm, Roger Pau Monné wrote:
>>> On Wed, Oct 30, 2024 at 02:45:19PM +0000, Andrew Cooper wrote:
>>>> On 30/10/2024 11:03 am, Roger Pau Monné wrote:
>>>>> On Wed, Oct 30, 2024 at 10:39:12AM +0000, Andrew Cooper wrote:
>>>>>> On 30/10/2024 8:59 am, Roger Pau Monné wrote:
>>>>>>> On Tue, Oct 29, 2024 at 05:55:05PM +0000, Andrew Cooper wrote:
>>>>>>>> diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
>>>>>>>> index b6d9fad56773..78bc9872b09a 100644
>>>>>>>> --- a/xen/arch/x86/cpu-policy.c
>>>>>>>> +++ b/xen/arch/x86/cpu-policy.c
>>>>>>>> @@ -391,6 +391,27 @@ static void __init calculate_host_policy(void)
>>>>>>>>      p->platform_info.cpuid_faulting = cpu_has_cpuid_faulting;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +/*
>>>>>>>> + * Guest max policies can have any max leaf/subleaf within bounds.
>>>>>>>> + *
>>>>>>>> + * - Some incoming VMs have a larger-than-necessary feat max_subleaf.
>>>>>>>> + * - Some VMs we'd like to synthesise leaves not present on the host.
>>>>>>>> + */
>>>>>>>> +static void __init guest_common_max_leaves(struct cpu_policy *p)
>>>>>>>> +{
>>>>>>>> +    p->basic.max_leaf       = ARRAY_SIZE(p->basic.raw) - 1;
>>>>>>>> +    p->feat.max_subleaf     = ARRAY_SIZE(p->feat.raw) - 1;
>>>>>>>> +    p->extd.max_leaf        = 0x80000000U + ARRAY_SIZE(p->extd.raw) - 
>>>>>>>> 1;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/* Guest default policies inherit the host max leaf/subleaf settings. 
>>>>>>>> */
>>>>>>>> +static void __init guest_common_default_leaves(struct cpu_policy *p)
>>>>>>>> +{
>>>>>>>> +    p->basic.max_leaf       = host_cpu_policy.basic.max_leaf;
>>>>>>>> +    p->feat.max_subleaf     = host_cpu_policy.feat.max_subleaf;
>>>>>>>> +    p->extd.max_leaf        = host_cpu_policy.extd.max_leaf;
>>>>>>>> +}
>>>>>>> I think this what I'm going to ask is future work.  After the
>>>>>>> modifications done to the host policy by max functions
>>>>>>> (calculate_{hvm,pv}_max_policy()) won't the max {sub,}leaf adjustments
>>>>>>> better be done taking into account the contents of the policy, rather
>>>>>>> than capping to the host values?
>>>>>>>
>>>>>>> (note this comment is strictly for guest_common_default_leaves(), the
>>>>>>> max version is fine using ARRAY_SIZE).
>>>>>> I'm afraid I don't follow.
>>>>>>
>>>>>> calculate_{pv,hvm}_max_policy() don't modify the host policy.
>>>>> Hm, I don't think I've expressed myself clearly, sorry.  Let me try
>>>>> again.
>>>>>
>>>>> calculate_{hvm,pv}_max_policy() extends the host policy by possibly
>>>>> setting new features, and such extended policy is then used as the
>>>>> base for the PV/HVM default policies.
>>>>>
>>>>> Won't the resulting policy in calculate_{hvm,pv}_def_policy() risks
>>>>> having bits set past the max {sub,}leaf in the host policy, as it's
>>>>> based in {hvm,pv}_def_cpu_policy that might have such bits set?
>>>> Oh, right.
>>>>
>>>> This patch doesn't change anything WRT that.
>>> Indeed, didn't intend my comment to block it, just that I think at
>>> some point the logic in guest_common_default_leaves() will need to be
>>> expanded.
>>>
>>>> But I think you're right that we do risk getting into that case (in
>>>> principle at least) because of how guest_common_*_feature_adjustment() 
>>>> work.
>>>>
>>>> Furthermore, the bug will typically get hidden because we serialise
>>>> based on the max_leaf/subleaf, and will discard feature words outside of
>>>> the max_leaf/subleaf bounds.
>>> Yes, once we serialize it for toolstack consumption the leafs will be
>>> implicitly zeroed.
>>>
>>>> I suppose we probably want a variation of x86_cpu_featureset_to_policy()
>>>> which extends the max_leaf/subleaf based on non-zero values in leaves. 
>>>> (This already feels like it's going to be an ugly algorithm.)
>>> Hm, I was thinking that we would need to adjust
>>> guest_common_default_leaves() to properly shrink the max {sub,}leaf
>>> fields from the max policies.
>>
>> Hmm.  What we'd do is have default inherit max's ARRAY_SIZES(), then do
>> all the existing logic, then as the final step, shrink the default
>> policies, vaguely per Jan's plan.
>>
>> i.e. we'd end up deleting guest_common_default_leaves()
>>
>> That way we don't need to encode any knowledge of which feature bit
>> means what WRT max_leaf/subleaf.
> 
> What about Alejandro's concern about runtime populated {sub,}leafs,
> won't we risk shrinking too much if the last leaf intended to be kept
> happens to be a fully runtime populated one?
> 
> Do we need some kind of special magic for fully run-time populated
> leafs (like the topology ones IIRC?) in order to account for them when
> doing those max calculations?

Contrary to Andrew's reply I think we will need to take runtime-populated
leaves into account specially, as you suggest. Just thinking of something
APIC-ID-like in a very high leaf, which (presumably) ought to be zero in
max/default. While keeping such fields at zero in max/default for external
exposure, filling them with a non-zero value at policy creation (maybe
simply their max value) might help keep the shrinking logic agnostic to
such special cases.

Jan