Re: [Xen-devel] [PATCH v8 05/13] x86: expose CBM length and COS number information
>>> On 29.05.15 at 11:23, wrote:
> On Fri, 2015-05-29 at 09:07 +0100, Jan Beulich wrote:
>> >>> On 29.05.15 at 04:47, wrote:
>> > On Thu, May 28, 2015 at 02:26:03PM +0100, Jan Beulich wrote:
>> >>
>> >> > --- a/xen/include/public/sysctl.h
>> >> > +++ b/xen/include/public/sysctl.h
>> >> > @@ -694,6 +694,20 @@ struct xen_sysctl_pcitopoinfo {
>> >> > typedef struct xen_sysctl_pcitopoinfo xen_sysctl_pcitopoinfo_t;
>> >> > DEFINE_XEN_GUEST_HANDLE(xen_sysctl_pcitopoinfo_t);
>> >> >
>> >> > +#define XEN_SYSCTL_PSR_CAT_get_l3_info 0
>> >> > +struct xen_sysctl_psr_cat_op {
>> >> > +uint32_t cmd; /* IN: XEN_SYSCTL_PSR_CAT_* */
>> >> > +uint32_t target;/* IN: socket to be operated on */
>> >>
>> >> If this is always the socket number, why would the variable be
>> >> named anything other than "socket". If otoh subsequent patches
>> >> use it differently, I think the comment should be omitted now
>> >> rather than being dropped then (or it should be given its final
>> >> wording from the beginning).
>> >
>> > Or 'target to be operated on'?
>>
>> Fine with me. Just not something that may end up being confusing.
>>
> So, I really don't want to turn this into pure bikeshedding, but, for a
> field called 'target', a comment saying 'target to be operated on' seems
> rather pointless, and I'd go for omitting it (for now).
Right - my earlier response was merely meant to say I'm not
opposed to a non-confusing comment, not that I see a strict
need for a mostly redundant one here.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 05/13] x86: expose CBM length and COS number information
On Fri, 2015-05-29 at 09:07 +0100, Jan Beulich wrote:
> >>> On 29.05.15 at 04:47, wrote:
> > On Thu, May 28, 2015 at 02:26:03PM +0100, Jan Beulich wrote:
> >>
> >> > --- a/xen/include/public/sysctl.h
> >> > +++ b/xen/include/public/sysctl.h
> >> > @@ -694,6 +694,20 @@ struct xen_sysctl_pcitopoinfo {
> >> > typedef struct xen_sysctl_pcitopoinfo xen_sysctl_pcitopoinfo_t;
> >> > DEFINE_XEN_GUEST_HANDLE(xen_sysctl_pcitopoinfo_t);
> >> >
> >> > +#define XEN_SYSCTL_PSR_CAT_get_l3_info 0
> >> > +struct xen_sysctl_psr_cat_op {
> >> > +uint32_t cmd; /* IN: XEN_SYSCTL_PSR_CAT_* */
> >> > +uint32_t target;/* IN: socket to be operated on */
> >>
> >> If this is always the socket number, why would the variable be
> >> named anything other than "socket". If otoh subsequent patches
> >> use it differently, I think the comment should be omitted now
> >> rather than being dropped then (or it should be given its final
> >> wording from the beginning).
> >
> > Or 'target to be operated on'?
>
> Fine with me. Just not something that may end up being confusing.
>
So, I really don't want to turn this into pure bikeshedding, but, for a
field called 'target', a comment saying 'target to be operated on' seems
rather pointless, and I'd go for omitting it (for now).
Dario
--
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 05/13] x86: expose CBM length and COS number information
On Thu, 2015-05-28 at 14:26 +0100, Jan Beulich wrote:
> >>> On 21.05.15 at 10:41, wrote:
> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -694,6 +694,20 @@ struct xen_sysctl_pcitopoinfo {
> > typedef struct xen_sysctl_pcitopoinfo xen_sysctl_pcitopoinfo_t;
> > DEFINE_XEN_GUEST_HANDLE(xen_sysctl_pcitopoinfo_t);
> >
> > +#define XEN_SYSCTL_PSR_CAT_get_l3_info 0
> > +struct xen_sysctl_psr_cat_op {
> > +uint32_t cmd; /* IN: XEN_SYSCTL_PSR_CAT_* */
> > +uint32_t target;/* IN: socket to be operated on */
>
> If this is always the socket number, why would the variable be
> named anything other than "socket". If otoh subsequent patches
> use it differently, I think the comment should be omitted now
> rather than being dropped then (or it should be given its final
> wording from the beginning).
>
ISTR asking about this interface before (it might have been at the
toolstack level, though), and the answer was that it is not subsequent
patches _in_this_series_ (of course), but maybe future ones that will
modify the 'per-socket-ness' nature of this feature.
So, yes, maybe the comment should say something along those lines (and
be updated when things change).
Dario
--
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 05/13] x86: expose CBM length and COS number information
>>> On 29.05.15 at 04:47, wrote:
> On Thu, May 28, 2015 at 02:26:03PM +0100, Jan Beulich wrote:
>>
>> > --- a/xen/include/public/sysctl.h
>> > +++ b/xen/include/public/sysctl.h
>> > @@ -694,6 +694,20 @@ struct xen_sysctl_pcitopoinfo {
>> > typedef struct xen_sysctl_pcitopoinfo xen_sysctl_pcitopoinfo_t;
>> > DEFINE_XEN_GUEST_HANDLE(xen_sysctl_pcitopoinfo_t);
>> >
>> > +#define XEN_SYSCTL_PSR_CAT_get_l3_info 0
>> > +struct xen_sysctl_psr_cat_op {
>> > +uint32_t cmd; /* IN: XEN_SYSCTL_PSR_CAT_* */
>> > +uint32_t target;/* IN: socket to be operated on */
>>
>> If this is always the socket number, why would the variable be
>> named anything other than "socket". If otoh subsequent patches
>> use it differently, I think the comment should be omitted now
>> rather than being dropped then (or it should be given its final
>> wording from the beginning).
>
> Or 'target to be operated on'?
Fine with me. Just not something that may end up being confusing.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 05/13] x86: expose CBM length and COS number information
On Thu, May 28, 2015 at 02:26:03PM +0100, Jan Beulich wrote:
>
> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -694,6 +694,20 @@ struct xen_sysctl_pcitopoinfo {
> > typedef struct xen_sysctl_pcitopoinfo xen_sysctl_pcitopoinfo_t;
> > DEFINE_XEN_GUEST_HANDLE(xen_sysctl_pcitopoinfo_t);
> >
> > +#define XEN_SYSCTL_PSR_CAT_get_l3_info 0
> > +struct xen_sysctl_psr_cat_op {
> > +uint32_t cmd; /* IN: XEN_SYSCTL_PSR_CAT_* */
> > +uint32_t target;/* IN: socket to be operated on */
>
> If this is always the socket number, why would the variable be
> named anything other than "socket". If otoh subsequent patches
> use it differently, I think the comment should be omitted now
> rather than being dropped then (or it should be given its final
> wording from the beginning).
Or 'target to be operated on'?
Chao
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 05/13] x86: expose CBM length and COS number information
>>> On 21.05.15 at 10:41, wrote:
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -216,6 +216,38 @@ void psr_ctxt_switch_to(struct domain *d)
> }
> }
>
> +static int get_cat_socket_info(unsigned int socket,
> + struct psr_cat_socket_info **info)
> +{
> +if ( !cat_socket_info )
> +return -ENODEV;
> +
> +if ( socket >= nr_sockets )
> +return -EBADSLT;
> +
> +if ( !test_bit(socket, cat_socket_enable) )
> +return -ENOENT;
> +
> +*info = cat_socket_info + socket;
> +
> +return 0;
> +}
> +
> +int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len,
> +uint32_t *cos_max)
> +{
> +struct psr_cat_socket_info *info;
> +int ret = get_cat_socket_info(socket, &info);
> +
> +if ( ret )
> +return ret;
> +
> +*cbm_len = info->cbm_len;
> +*cos_max = info->cos_max;
> +
> +return 0;
> +}
I doubt all supported compiler versions will be able to see that "info"
can't be used uninitialized here. Please make this explicit.
I also think this small a function shouldn't have two return points.
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -694,6 +694,20 @@ struct xen_sysctl_pcitopoinfo {
> typedef struct xen_sysctl_pcitopoinfo xen_sysctl_pcitopoinfo_t;
> DEFINE_XEN_GUEST_HANDLE(xen_sysctl_pcitopoinfo_t);
>
> +#define XEN_SYSCTL_PSR_CAT_get_l3_info 0
> +struct xen_sysctl_psr_cat_op {
> +uint32_t cmd; /* IN: XEN_SYSCTL_PSR_CAT_* */
> +uint32_t target;/* IN: socket to be operated on */
If this is always the socket number, why would the variable be
named anything other than "socket". If otoh subsequent patches
use it differently, I think the comment should be omitted now
rather than being dropped then (or it should be given its final
wording from the beginning).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
