Re: [PATCH RFC 0/9] x86: Merge cpuid and msr policy

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/03/2023 3:43 pm, Roger Pau Monné wrote:
> On Thu, Mar 30, 2023 at 01:59:37PM +0100, Andrew Cooper wrote:
>> On 30/03/2023 12:07 pm, Roger Pau Monné wrote:
>>> On Wed, Mar 29, 2023 at 09:51:28PM +0100, Andrew Cooper wrote:
>>>> tl;dr to add MSR_ARCH_CAPS features sensibly, cpu_{featureset<->policy}() 
>>>> need
>>>> to not operate on objects of differing lifetimes, so structs
>>>> {cpuid,msr}_policy need merging and cpu_policy is the obvious name.
>>> So the problem is that there's a chance we might get a cpu_policy
>>> object that contains a valid (allocated) cpuid object, but not an msr
>>> one?
>> No - not cpu_policy.  It is that we can get a cpuid_policy and an
>> msr_policy that aren't at the same point in their lifecycle.
>>
>> ... which is exactly what happens right now for the raw/host msr right
>> now if you featureset_to_policy() to include MSR data.
> I see, but that's mostly because we handle the featureset_to_policy()
> in two different places for CPUID vs MSR, those need to be unified
> into a single helper that does both at the same point.
>
> I assume not having such pointers in side of cpu_policy makes it
> clearer that both msr and cpuid should be handled at the same time,
> but ultimately this would imply passing a cpu_policy object to
> featureset_to_policy() so that both CPUID and MSR sub-structs are
> filled from the same featureset.
>
> Sorry, maybe I'm being a bit dull here, just would like to understand
> the motivation of the change.

That's pretty much it.  Forcing them to be one object removes a class of
errors, and makes the resulting code easier to follow.

(Based on having tried to do the non-merged approach first, and deciding
that it's not code I'm willing to try putting upstream...)

>> Merging the two together into cpu_policy causes there to be a single
>> object lifecycle.
>>
>>
>> It's probably worth repeating the advise from the footnote in
>> https://lwn.net/Articles/193245/ again.  Get your datastructures right,
>> and the code takes care of itself.  Don't get them right, and the code
>> tends to be unmaintainable.
>>
>>
>>>> But this does mean that we now have
>>>>
>>>>   cpu_policy->basic.$X
>>>>   cpu_policy->feat.$Y
>>>>   cpu_policy->arch_caps.$Z
>>> I'm not sure I like the fact that we now can't differentiate between
>>> policy fields related to MSRs or CPUID leafs.
>>>
>>> Isn't there a chance we might in the future get some name space
>>> collision by us having decided to unify both?
>> The names are chosen by me so far, and the compiler will tell us if
>> things actually collide.
>>
>> And renaming the existing field is a perfectly acceptable way of
>> resolving a conflict which arises in the future.
>>
>> But yes - this was the whole point of asking the question.
> I think I would prefer to keep the cpu_policy->{cpuid,msr}.
> distinction if it doesn't interfere with further work.

Unfortunately that's the opposite of what Jan asked for.  What I have
done, based on the prior conversation is:

struct arch_domain {
    ...

    /*
     * The domain's CPU Policy.  "cpu_policy" is considered the canonical
     * pointer, but the "cpuid" and "msr" aliases exist so the most
     * appropriate one can be used for local code clarity.
     */
    union {
        struct cpu_policy *cpu_policy;
        struct cpu_policy *cpuid;
        struct cpu_policy *msr;
    };

So all the cases where you do have d->arch.cpuid.feat.$X, this continues
to work.

The cases where you pull a cpu_policy out into a local variable, there
will be no cpuid or msr infix, but these cases already have no cpuid/msr
part to their naming.

~Andrew