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