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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	Jan Beulich <JBeulich@suse.com>, Wei Liu <wl@xen.org>
Subject: Re: [PATCH RFC 0/9] x86: Merge cpuid and msr policy
Date: Thu, 30 Mar 2023 16:43:43 +0200	[thread overview]
Message-ID: <ZCWgHxCL4yXD6CxC@Air-de-Roger> (raw)
In-Reply-To: <9108a58f-da8f-14e4-de88-a7c8c8abb0f7@citrix.com>

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.

> 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.

Thanks, Roger.