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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: "Roger Pau Monné" <roger.pau@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: Mon, 3 Apr 2023 12:48:49 +0100	[thread overview]
Message-ID: <6788d260-fe5c-f3e8-d479-329a2149fba4@citrix.com> (raw)
In-Reply-To: <ZCq8yDv/FaOvtTPo@Air-de-Roger>

On 03/04/2023 12:47 pm, Roger Pau Monné wrote:
> On Mon, Apr 03, 2023 at 11:55:57AM +0100, Andrew Cooper wrote:
>> 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:
>>>>>> 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.
> I see.  I'm fine with this.  There's still the remote possibility of
> field name clash between cpuid and msr names, but we can likely sort
> this out if we ever get into this position.

Thanks.  Yeah, we can rename if things become problematic.

~Andrew