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: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}}


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

~Andrew

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