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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 16.07.18 at 15:15, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 13:29, Jan Beulich wrote:
>>>>> On 16.07.18 at 14:16, <andrew.cooper3@citrix.com> wrote:
>>> On 16/07/18 13:04, Jan Beulich wrote:
>>>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> 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".

Oops.

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

It was you even more than me who was worried about NULL pointers
sitting in random places, waiting to be de-referenced. Besides the
obvious reference by symbolic, there are clearly other ways to index
into this array, not to mention someone setting up a loop over it.
Anyway - I can live with it being the way it is, and I've given the ack.

Jan



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