Re: [Xen-devel] [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/07/18 13:29, Jan Beulich wrote:
>>>> On 16.07.18 at 14:16, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 16/07/18 13:04, Jan Beulich wrote:
>>>>>> On 13.07.18 at 22:03, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> --- a/xen/arch/x86/sysctl.c
>>>> +++ b/xen/arch/x86/sysctl.c
>>>> @@ -31,6 +31,33 @@
>>>>  #include <asm/psr.h>
>>>>  #include <asm/cpuid.h>
>>>>  
>>>> +const struct cpu_policy system_policies[] = {
>>> By the end of the series the array remains unused outside this
>>> source file. I'd appreciate if it was made extern only when actually
>>> needed, not the least because ...
>>>
>>>> +    [ XEN_SYSCTL_cpu_policy_raw ] = {
>>>> +        &raw_cpuid_policy,
>>>> +        &raw_msr_policy,
>>>> +    },
>>>> +    [ XEN_SYSCTL_cpu_policy_host ] = {
>>>> +        &host_cpuid_policy,
>>>> +        &host_msr_policy,
>>>> +    },
>>>> +    [ XEN_SYSCTL_cpu_policy_pv_max ] = {
>>>> +        &pv_max_cpuid_policy,
>>>> +        &pv_max_msr_policy,
>>>> +    },
>>>> +    [ XEN_SYSCTL_cpu_policy_hvm_max ] = {
>>>> +        &hvm_max_cpuid_policy,
>>>> +        &hvm_max_msr_policy,
>>>> +    },
>>>> +    [ XEN_SYSCTL_cpu_policy_pv_default ] = {
>>>> +        &pv_max_cpuid_policy,
>>>> +        &pv_max_msr_policy,
>>>> +    },
>>>> +    [ XEN_SYSCTL_cpu_policy_hvm_default ] = {
>>>> +        &hvm_max_cpuid_policy,
>>>> +        &hvm_max_msr_policy,
>>>> +    },
>>>> +};
>>> ... this does not make obvious (without consulting sysctl.h) that
>>> there are now holes (and hence hidden NULL pointers); this is
>>> perhaps already undesirable with the user of this array that the
>>> next patch adds.
>> What holes?  There shouldn't be any, and gdb confirms my expectations:
>>
>> (gdb) p/x system_policies
>> $1 = {{cpuid = 0xffff82d080474a80, msr = 0xffff82d080475960}, {cpuid =
>> 0xffff82d080474340, msr = 0xffff82d08047595c}, {cpuid =
>> 0xffff82d080473c00, msr = 0xffff82d080475954}, {cpuid =
>> 0xffff82d0804734c0, msr = 0xffff82d080475958}, {cpuid =
>> 0xffff82d080473c00, msr = 0xffff82d080475954}, {cpuid =
>> 0xffff82d0804734c0, msr = 0xffff82d080475958}}
> I didn't say there are holes, I've said "does not make obvious".

You did, but I guess what you meant to write was "... are no holes"
rather "... are now holes".

> For example, it is not unreasonable to imagine for the
> XEN_SYSCTL_cpu_policy_* values to start at 1 rather than zero, in
> which case there would be two hidden NULLs at the start of the
> array.

Why does this matter?  We have similar patterns elsewhere, and the array
cannot reasonably be used without the symbolic names (as it is really an
unordered set happening to be layed out in array form).

>
>>> With "static" added and the "extern" dropped from the header
>>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>> I'm not going to waste even more time by committing something which is
>> wrong, and having to undo it again in a later patch.
>>
>> The user, DOMCTL_set_cpu_policy, is deferred from this series because it
>> is still under development, but there is absolutely no question that
>> this array needs to be externally accessible.
> Well, maybe I should have phrased this differently: I'm unconvinced
> sysctl.c is the right place for this to live. Granted neither cpuid.c nor
> msr.c are any better.

If you can suggest a better place then I'm all ears, but it has to live
somewhere and here was the least-bad option I could come up with.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel