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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

Thanks, Roger.