Mailing List Archive: [PATCH RFC 0/9] x86: Merge cpuid and msr policy

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

Mar 29, 2023, 1:51 PM

Post #1 of 9 (55 views)

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.

But this does mean that we now have

cpu_policy->basic.$X
cpu_policy->feat.$Y
cpu_policy->arch_caps.$Z

and plenty of code now reads

d->arch.cpu_policy->feat.$Y

instead of

d->arch.cpuid->feat.$Y

The latter can be half-fixed with some union magic (see patch 9 commit
message). The former can be fixed by putting cpuid/msr infixes in cpu_policy,
which is doable but very invasive, and would make plenty of code read

d->arch.cpu_policy->cpuid.feat.$Y

and the two obviously shouldn't be done together.

So, RFC. Does this code layout look ok? If we want to make changes with
naming, now is very much the right time to get them sorted.

Patches 1-8 are pretty ready to go. Patch 9 is the remainder to take out the
temporary hacks, and I'm still in the process of merging the system policy
derivation.

Andrew Cooper (9):
x86: Rename struct cpu_policy to struct old_cpuid_policy
x86: Rename {domctl,sysctl}.cpu_policy.{cpuid,msr_policy} fields
x86: Rename struct cpuid_policy to struct cpu_policy
x86: Merge struct msr_policy into struct cpu_policy
x86: Merge the system {cpuid,msr} policy objects
x86: Merge a domain's {cpuid,msr} policy objects
x86: Merge xc_cpu_policy's cpuid and msr objects
x86: Drop struct old_cpu_policy
RFC: Everything else

tools/fuzz/cpu-policy/afl-policy-fuzzer.c | 15 +-
.../fuzz/x86_instruction_emulator/fuzz-emul.c | 2 +-
tools/libs/guest/xg_cpuid_x86.c | 48 +-
tools/libs/guest/xg_private.h | 5 +-
tools/tests/cpu-policy/test-cpu-policy.c | 50 +-
tools/tests/tsx/test-tsx.c | 58 +-
tools/tests/x86_emulator/Makefile | 2 +-
tools/tests/x86_emulator/test_x86_emulator.c | 2 +-
tools/tests/x86_emulator/x86-emulate.c | 2 +-
tools/tests/x86_emulator/x86-emulate.h | 2 +-
xen/arch/x86/Makefile | 1 +
xen/arch/x86/cpu-policy.c | 67 +++
xen/arch/x86/cpu/common.c | 4 +-
xen/arch/x86/cpu/mcheck/mce_intel.c | 2 +-
xen/arch/x86/cpu/vpmu_intel.c | 4 +-
xen/arch/x86/cpuid.c | 101 ++--
xen/arch/x86/domain.c | 18 +-
xen/arch/x86/domctl.c | 51 +-
xen/arch/x86/hvm/emulate.c | 2 +-
xen/arch/x86/hvm/hvm.c | 38 +-
xen/arch/x86/hvm/ioreq.c | 4 +-
xen/arch/x86/hvm/mtrr.c | 2 +-
xen/arch/x86/hvm/svm/svm.c | 18 +-
xen/arch/x86/hvm/svm/svmdebug.c | 2 +-
xen/arch/x86/hvm/vlapic.c | 2 +-
xen/arch/x86/hvm/vmx/vmx.c | 12 +-
xen/arch/x86/hvm/vmx/vvmx.c | 2 +-
xen/arch/x86/include/asm/cpu-policy.h | 18 +
xen/arch/x86/include/asm/cpuid.h | 10 -
xen/arch/x86/include/asm/domain.h | 4 +-
xen/arch/x86/include/asm/guest_pt.h | 4 +-
xen/arch/x86/include/asm/msr.h | 13 +-
xen/arch/x86/include/asm/paging.h | 2 +-
xen/arch/x86/mm/mem_sharing.c | 3 +-
xen/arch/x86/mm/shadow/hvm.c | 2 +-
xen/arch/x86/msr.c | 98 +---
xen/arch/x86/pv/domain.c | 2 +-
xen/arch/x86/pv/emul-priv-op.c | 6 +-
xen/arch/x86/pv/ro-page-fault.c | 2 +-
xen/arch/x86/sysctl.c | 77 +--
xen/arch/x86/traps.c | 2 +-
xen/arch/x86/x86_emulate.c | 2 +-
xen/arch/x86/x86_emulate/x86_emulate.c | 166 +++---
xen/arch/x86/x86_emulate/x86_emulate.h | 6 +-
xen/arch/x86/xstate.c | 4 +-
xen/include/public/domctl.h | 4 +-
xen/include/public/sysctl.h | 4 +-
xen/include/xen/lib/x86/cpu-policy.h | 540 +++++++++++++++++-
xen/include/xen/lib/x86/cpuid.h | 475 ---------------
xen/include/xen/lib/x86/msr.h | 104 ----
xen/lib/x86/cpuid.c | 12 +-
xen/lib/x86/msr.c | 6 +-
xen/lib/x86/policy.c | 8 +-
53 files changed, 986 insertions(+), 1104 deletions(-)
create mode 100644 xen/arch/x86/cpu-policy.c
create mode 100644 xen/arch/x86/include/asm/cpu-policy.h
delete mode 100644 xen/include/xen/lib/x86/cpuid.h
delete mode 100644 xen/include/xen/lib/x86/msr.h

--
2.30.2

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

jbeulich at suse

Mar 30, 2023, 3:18 AM

Post #2 of 9 (55 views)

Permalink

On 29.03.2023 22:51, 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.
>
> But this does mean that we now have
>
> cpu_policy->basic.$X
> cpu_policy->feat.$Y
> cpu_policy->arch_caps.$Z
>
> and plenty of code now reads
>
> d->arch.cpu_policy->feat.$Y
>
> instead of
>
> d->arch.cpuid->feat.$Y
>
> The latter can be half-fixed with some union magic (see patch 9 commit
> message). The former can be fixed by putting cpuid/msr infixes in cpu_policy,
> which is doable but very invasive, and would make plenty of code read
>
> d->arch.cpu_policy->cpuid.feat.$Y
>
> and the two obviously shouldn't be done together.
>
> So, RFC. Does this code layout look ok? If we want to make changes with
> naming, now is very much the right time to get them sorted.
>
> Patches 1-8 are pretty ready to go. Patch 9 is the remainder to take out the
> temporary hacks, and I'm still in the process of merging the system policy
> derivation.
>
> Andrew Cooper (9):
> x86: Rename struct cpu_policy to struct old_cpuid_policy
> x86: Rename {domctl,sysctl}.cpu_policy.{cpuid,msr_policy} fields

Nit: I guess the last closing brace wants moving forward a little.

> x86: Rename struct cpuid_policy to struct cpu_policy
> x86: Merge struct msr_policy into struct cpu_policy
> x86: Merge the system {cpuid,msr} policy objects
> x86: Merge a domain's {cpuid,msr} policy objects
> x86: Merge xc_cpu_policy's cpuid and msr objects
> x86: Drop struct old_cpu_policy

With the small comments on individual patches taken care of one way or
another, up to here:

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

> RFC: Everything else
>
> tools/fuzz/cpu-policy/afl-policy-fuzzer.c | 15 +-
> .../fuzz/x86_instruction_emulator/fuzz-emul.c | 2 +-
> tools/libs/guest/xg_cpuid_x86.c | 48 +-
> tools/libs/guest/xg_private.h | 5 +-
> tools/tests/cpu-policy/test-cpu-policy.c | 50 +-
> tools/tests/tsx/test-tsx.c | 58 +-
> tools/tests/x86_emulator/Makefile | 2 +-
> tools/tests/x86_emulator/test_x86_emulator.c | 2 +-
> tools/tests/x86_emulator/x86-emulate.c | 2 +-
> tools/tests/x86_emulator/x86-emulate.h | 2 +-
> xen/arch/x86/Makefile | 1 +
> xen/arch/x86/cpu-policy.c | 67 +++
> xen/arch/x86/cpu/common.c | 4 +-
> xen/arch/x86/cpu/mcheck/mce_intel.c | 2 +-
> xen/arch/x86/cpu/vpmu_intel.c | 4 +-
> xen/arch/x86/cpuid.c | 101 ++--
> xen/arch/x86/domain.c | 18 +-
> xen/arch/x86/domctl.c | 51 +-
> xen/arch/x86/hvm/emulate.c | 2 +-
> xen/arch/x86/hvm/hvm.c | 38 +-
> xen/arch/x86/hvm/ioreq.c | 4 +-
> xen/arch/x86/hvm/mtrr.c | 2 +-
> xen/arch/x86/hvm/svm/svm.c | 18 +-
> xen/arch/x86/hvm/svm/svmdebug.c | 2 +-
> xen/arch/x86/hvm/vlapic.c | 2 +-
> xen/arch/x86/hvm/vmx/vmx.c | 12 +-
> xen/arch/x86/hvm/vmx/vvmx.c | 2 +-
> xen/arch/x86/include/asm/cpu-policy.h | 18 +
> xen/arch/x86/include/asm/cpuid.h | 10 -
> xen/arch/x86/include/asm/domain.h | 4 +-
> xen/arch/x86/include/asm/guest_pt.h | 4 +-
> xen/arch/x86/include/asm/msr.h | 13 +-
> xen/arch/x86/include/asm/paging.h | 2 +-
> xen/arch/x86/mm/mem_sharing.c | 3 +-
> xen/arch/x86/mm/shadow/hvm.c | 2 +-
> xen/arch/x86/msr.c | 98 +---
> xen/arch/x86/pv/domain.c | 2 +-
> xen/arch/x86/pv/emul-priv-op.c | 6 +-
> xen/arch/x86/pv/ro-page-fault.c | 2 +-
> xen/arch/x86/sysctl.c | 77 +--
> xen/arch/x86/traps.c | 2 +-
> xen/arch/x86/x86_emulate.c | 2 +-
> xen/arch/x86/x86_emulate/x86_emulate.c | 166 +++---
> xen/arch/x86/x86_emulate/x86_emulate.h | 6 +-
> xen/arch/x86/xstate.c | 4 +-
> xen/include/public/domctl.h | 4 +-
> xen/include/public/sysctl.h | 4 +-
> xen/include/xen/lib/x86/cpu-policy.h | 540 +++++++++++++++++-
> xen/include/xen/lib/x86/cpuid.h | 475 ---------------
> xen/include/xen/lib/x86/msr.h | 104 ----
> xen/lib/x86/cpuid.c | 12 +-
> xen/lib/x86/msr.c | 6 +-
> xen/lib/x86/policy.c | 8 +-
> 53 files changed, 986 insertions(+), 1104 deletions(-)
> create mode 100644 xen/arch/x86/cpu-policy.c
> create mode 100644 xen/arch/x86/include/asm/cpu-policy.h
> delete mode 100644 xen/include/xen/lib/x86/cpuid.h
> delete mode 100644 xen/include/xen/lib/x86/msr.h
>

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

roger.pau at citrix

Mar 30, 2023, 4:07 AM

Post #3 of 9 (55 views)

Permalink

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?

Or the problem is rather with the domain struct containing separate
cpuid/msr fields not encapsulated in the cpu_policy struct?

I don't think the current set of cpu_policy operations permit you to
operate on incomplete objects anyway.

I assume you are worried about the usage of x86_msr_copy_from_buffer()
for example that could load data into the MSR_ARCH_CAPS field for the
msr object without checking that the corresponding CPUID bit is set?

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

Thanks, Roger.

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

andrew.cooper3 at citrix

Mar 30, 2023, 5:08 AM

Post #4 of 9 (55 views)

Permalink

On 30/03/2023 11:18 am, Jan Beulich wrote:
> On 29.03.2023 22:51, Andrew Cooper wrote:
>> Andrew Cooper (9):
>> x86: Rename struct cpu_policy to struct old_cpuid_policy
>> x86: Rename {domctl,sysctl}.cpu_policy.{cpuid,msr_policy} fields
> Nit: I guess the last closing brace wants moving forward a little.

Oops yes.

>> x86: Rename struct cpuid_policy to struct cpu_policy
>> x86: Merge struct msr_policy into struct cpu_policy
>> x86: Merge the system {cpuid,msr} policy objects
>> x86: Merge a domain's {cpuid,msr} policy objects
>> x86: Merge xc_cpu_policy's cpuid and msr objects
>> x86: Drop struct old_cpu_policy
> With the small comments on individual patches taken care of one way or
> another, up to here:
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks. I'll fold this in on these patches, but I don't intend to
commit anything until I've got the full series together.

Right now there's still a lot of shuffling of minor things (comments)
between patches, and not-so-minor things (deciding to split things
differently).

~Andrew

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

andrew.cooper3 at citrix

Mar 30, 2023, 5:59 AM

Post #5 of 9 (55 views)

Permalink

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.

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.

~Andrew

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

roger.pau at citrix

Mar 30, 2023, 7:43 AM

Post #6 of 9 (55 views)

Permalink

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.

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

andrew.cooper3 at citrix

Apr 3, 2023, 3:55 AM

Post #7 of 9 (55 views)

Permalink

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

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

roger.pau at citrix

Apr 3, 2023, 4:47 AM

Post #8 of 9 (55 views)

Permalink

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

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

andrew.cooper3 at citrix

Apr 3, 2023, 4:48 AM

Post #9 of 9 (55 views)

Permalink

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