Re: [Xen-devel] [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
On Fri, Jul 13, 2018 at 09:03:12PM +0100, Andrew Cooper wrote: > This is prep work for the following patch - please refer to it as well. > > When auditing and manipulating policies, it is necessary to do so with a > complete set of policies, due to the interdependences of the contents. A > containing structure like this will allow for clearer APIs and code. > > As a first user, this structure is convenient for the mapping used by > XEN_SYSCTL_get_cpu_policy (implemented in the next patch), and for auditing > (later when XEN_DOMCTL_set_cpu_policy is implemented). > > At this point, the distinction between *_max and *_default is introduced into > the ABI. For now, *_default is mapped to *_max, but future development work > will result in *_default being a logical subset of *_max. > > Signed-off-by: Andrew Cooper Reviewed-by: Roger Pau Monné Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
On Fri, Jul 13, 2018 at 09:03:12PM +0100, Andrew Cooper wrote: > This is prep work for the following patch - please refer to it as well. > > When auditing and manipulating policies, it is necessary to do so with a > complete set of policies, due to the interdependences of the contents. A > containing structure like this will allow for clearer APIs and code. > > As a first user, this structure is convenient for the mapping used by > XEN_SYSCTL_get_cpu_policy (implemented in the next patch), and for auditing > (later when XEN_DOMCTL_set_cpu_policy is implemented). > > At this point, the distinction between *_max and *_default is introduced into > the ABI. For now, *_default is mapped to *_max, but future development work > will result in *_default being a logical subset of *_max. > > Signed-off-by: Andrew Cooper Reviewed-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
>>> On 13.07.18 at 22:03, wrote:
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -31,6 +31,33 @@
> #include
> #include
>
> +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.
With "static" added and the "extern" dropped from the header
Acked-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
On 16/07/18 13:04, Jan Beulich wrote:
On 13.07.18 at 22:03, wrote:
>> --- a/xen/arch/x86/sysctl.c
>> +++ b/xen/arch/x86/sysctl.c
>> @@ -31,6 +31,33 @@
>> #include
>> #include
>>
>> +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 = 0x82d080474a80, msr = 0x82d080475960}, {cpuid =
0x82d080474340, msr = 0x82d08047595c}, {cpuid =
0x82d080473c00, msr = 0x82d080475954}, {cpuid =
0x82d0804734c0, msr = 0x82d080475958}, {cpuid =
0x82d080473c00, msr = 0x82d080475954}, {cpuid =
0x82d0804734c0, msr = 0x82d080475958}}
>
> With "static" added and the "extern" dropped from the header
> Acked-by: Jan Beulich
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
>>> On 16.07.18 at 14:16, wrote:
> On 16/07/18 13:04, Jan Beulich wrote:
> On 13.07.18 at 22:03, wrote:
>>> --- a/xen/arch/x86/sysctl.c
>>> +++ b/xen/arch/x86/sysctl.c
>>> @@ -31,6 +31,33 @@
>>> #include
>>> #include
>>>
>>> +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 = 0x82d080474a80, msr = 0x82d080475960}, {cpuid =
> 0x82d080474340, msr = 0x82d08047595c}, {cpuid =
> 0x82d080473c00, msr = 0x82d080475954}, {cpuid =
> 0x82d0804734c0, msr = 0x82d080475958}, {cpuid =
> 0x82d080473c00, msr = 0x82d080475954}, {cpuid =
> 0x82d0804734c0, msr = 0x82d080475958}}
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
>
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
On 16/07/18 13:29, Jan Beulich wrote:
On 16.07.18 at 14:16, wrote:
>> On 16/07/18 13:04, Jan Beulich wrote:
>> On 13.07.18 at 22:03, wrote:
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -31,6 +31,33 @@
#include
#include
+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 = 0x82d080474a80, msr = 0x82d080475960}, {cpuid =
>> 0x82d080474340, msr = 0x82d08047595c}, {cpuid =
>> 0x82d080473c00, msr = 0x82d080475954}, {cpuid =
>> 0x82d0804734c0, msr = 0x82d080475958}, {cpuid =
>> 0x82d080473c00, msr = 0x82d080475954}, {cpuid =
>> 0x82d0804734c0, msr = 0x82d080475958}}
> 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
>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 11/13] x86: Introduce struct cpu_policy to refer to a group of individual policies
>>> On 16.07.18 at 15:15, wrote:
> On 16/07/18 13:29, Jan Beulich wrote:
> On 16.07.18 at 14:16, wrote:
>>> On 16/07/18 13:04, Jan Beulich wrote:
>>> On 13.07.18 at 22:03, wrote:
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -31,6 +31,33 @@
> #include
> #include
>
> +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 = 0x82d080474a80, msr = 0x82d080475960}, {cpuid =
>>> 0x82d080474340, msr = 0x82d08047595c}, {cpuid =
>>> 0x82d080473c00, msr = 0x82d080475954}, {cpuid =
>>> 0x82d0804734c0, msr = 0x82d080475958}, {cpuid =
>>> 0x82d080473c00, msr = 0x82d080475954}, {cpuid =
>>> 0x82d0804734c0, msr = 0x82d080475958}}
>> 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
