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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

In the end the point of wanting things static for as long as they need
to be so is such that reviewers can notice when something gains
wider use. I fully accept that the "set" operation will want access to
the array, but we can't demand of other contributors to avoid
needlessly making symbols global and at the same time behave
differently ourselves. My ack stands without the requested adjustment
if this goes in together with the patch implementing "set".

Jan



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