Re: [Xen-devel] [PATCH v4 02/15] Rename PSR sysctl/domctl interfaces and xsm policy to make them be general

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-09-25 17:15:21, Roger Pau Monn� wrote:
> On Sat, Sep 23, 2017 at 09:48:11AM +0000, Yi Sun wrote:
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
[...]

> > -        case XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA:
> > -            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> > +        case XEN_DOMCTL_PSR_GET_L3_DATA:
> > +            ret = psr_get_val(d, domctl->u.psr_alloc.target,
> >                                &val32, PSR_CBM_TYPE_L3_DATA);
> > -            domctl->u.psr_cat_op.data = val32;
> > +            domctl->u.psr_alloc.data = val32;
> >              copyback = true;
> >              break;
> >  
> > -        case XEN_DOMCTL_PSR_CAT_OP_GET_L2_CBM:
> > -            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> > +        case XEN_DOMCTL_PSR_GET_L2_CBM:
> > +            ret = psr_get_val(d, domctl->u.psr_alloc.target,
> >                                &val32, PSR_CBM_TYPE_L2);
> > -            domctl->u.psr_cat_op.data = val32;
> > +            domctl->u.psr_alloc.data = val32;
> >              copyback = true;
> 
> This:
> 
> ret = psr_get_val(...);
> domctl->u.psr...;
> copyback = true;
> 
> Construct is quite repetitive, maybe you could consider adding a local
> macro to hide it?
> 
> Maybe then you could also hide the val32 declaration inside of it.
> 
Thanks for the suggestion! Let me try.

[...]

> >  
> > --- a/xen/arch/x86/sysctl.c
> > +++ b/xen/arch/x86/sysctl.c
> > @@ -171,45 +171,45 @@ long arch_do_sysctl(
> >  
> >          break;
> >  
> > -    case XEN_SYSCTL_psr_cat_op:
> > -        switch ( sysctl->u.psr_cat_op.cmd )
> > +    case XEN_SYSCTL_psr_alloc:
> > +        switch ( sysctl->u.psr_alloc.cmd )
> >          {
> >              uint32_t data[PSR_INFO_ARRAY_SIZE];
> >  
> > -        case XEN_SYSCTL_PSR_CAT_get_l3_info:
> > +        case XEN_SYSCTL_PSR_get_l3_info:
> >          {
> > -            ret = psr_get_info(sysctl->u.psr_cat_op.target,
> > +            ret = psr_get_info(sysctl->u.psr_alloc.target,
> >                                 PSR_CBM_TYPE_L3, data, ARRAY_SIZE(data));
> >              if ( ret )
> >                  break;
> >  
> > -            sysctl->u.psr_cat_op.u.cat_info.cos_max =
> > +            sysctl->u.psr_alloc.u.cat_info.cos_max =
> >                                        data[PSR_INFO_IDX_COS_MAX];
> > -            sysctl->u.psr_cat_op.u.cat_info.cbm_len =
> > +            sysctl->u.psr_alloc.u.cat_info.cbm_len =
> >                                        data[PSR_INFO_IDX_CAT_CBM_LEN];
> > -            sysctl->u.psr_cat_op.u.cat_info.flags =
> > +            sysctl->u.psr_alloc.u.cat_info.flags =
> >                                        data[PSR_INFO_IDX_CAT_FLAG];
> >  
> > -            if ( __copy_field_to_guest(u_sysctl, sysctl, u.psr_cat_op) )
> > +            if ( __copy_field_to_guest(u_sysctl, sysctl, u.psr_alloc) )
> >                  ret = -EFAULT;
> >              break;
> >          }
> >  
> > -        case XEN_SYSCTL_PSR_CAT_get_l2_info:
> > +        case XEN_SYSCTL_PSR_get_l2_info:
> >          {
> > -            ret = psr_get_info(sysctl->u.psr_cat_op.target,
> > +            ret = psr_get_info(sysctl->u.psr_alloc.target,
> >                                 PSR_CBM_TYPE_L2, data, ARRAY_SIZE(data));
> >              if ( ret )
> >                  break;
> >  
> > -            sysctl->u.psr_cat_op.u.cat_info.cos_max =
> > +            sysctl->u.psr_alloc.u.cat_info.cos_max =
> >                                        data[PSR_INFO_IDX_COS_MAX];
> > -            sysctl->u.psr_cat_op.u.cat_info.cbm_len =
> > +            sysctl->u.psr_alloc.u.cat_info.cbm_len =
> >                                        data[PSR_INFO_IDX_CAT_CBM_LEN];
> > -            sysctl->u.psr_cat_op.u.cat_info.flags =
> > +            sysctl->u.psr_alloc.u.cat_info.flags =
> >                                        data[PSR_INFO_IDX_CAT_FLAG];
> 
> The above repetition is also unneeded AFAICT. Why not simplify the
> switch to:
> 
> uint32_t data[PSR_INFO_ARRAY_SIZE];
> enum cbm_type type;
> 
> switch ( sysctl->u.psr_alloc.cmd )
> {
> case XEN_SYSCTL_PSR_get_l3_info:
>     type = PSR_CBM_TYPE_L3;
>     break;
> case XEN_SYSCTL_PSR_get_l2_info:
>     type = PSR_CBM_TYPE_L2;
>     break;
> default:
>     return -EOPNOTSUPP;
> }
> 
> ret = psr_get_info(sysctl->u.psr_alloc.target, type, data, ARRAY_SIZE(data));
> if ( ret )
>     break;
> 
> sysctl->u.psr_alloc.u.cat_info.cos_max = data[PSR_INFO_IDX_COS_MAX];
> ...
> 
> It would prevent some of this code repetition IMHO.
> 
Thank you! But this way seems only good to CAT features but could not cover MBA.
Although I can assign "type = PSR_TYPE_MBA_THRTL" for MBA, the assignment of
'sysctl->u.psr_alloc.u.mba_info' and 'sysctl->u.psr_alloc.u.cat_info' cannot be
converged. We have to check the type to decide to assign value to which 'info'.

> Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel