Re: [Xen-devel] [PATCH v2 07/15] x86: implement set value flow for MBA

[Roger Pau Monn� <roger.pau@xxxxxxxxxx>]

On Thu, Aug 31, 2017 at 10:20:10AM +0800, Yi Sun wrote:
> On 17-08-30 09:31:04, Roger Pau Monn� wrote:
> > On Thu, Aug 24, 2017 at 09:14:41AM +0800, Yi Sun wrote:
> > > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> > > index 4a0c982..ce82975 100644
> > > --- a/xen/arch/x86/psr.c
> > > +++ b/xen/arch/x86/psr.c
> > > @@ -138,6 +138,12 @@ static const struct feat_props {
> > >  
> > >      /* write_msr is used to write out feature MSR register. */
> > >      void (*write_msr)(unsigned int cos, uint32_t val, enum psr_val_type 
> > > type);
> > > +
> > > +    /*
> > > +     * check_val is used to check if input val fulfills SDM requirement.
> > > +     * Change it to valid value if SDM allows.
> > 
> > I'm not really sure it's a good idea to change the value to a valid
> > one, IMHO you should just check and print an error if the value is
> > invalid (and return false of course).
> > 
> Per SDM, the HW has ability to automatically change the input value to what it
> wants. E.g:
>   Linear mode: HW wants the input value be 10/20/30/.../90. But user inputs 
> 15.
>                Then, HW can automatically change it to 10.

This seems like a very, very bad idea IMHO. What if I input 15 (which
is invalid), but I would rather prefer 20 instead of 10?

I would very much prefer an error, so I can get to chose a valid value
myself instead of the hardware deciding on my back.

> Even user inputs a value that does not fulfill HW requirement, HW can handle 
> it.
> So, we do not need return error to user. Otherwise, user needs to know details
> of MBA.
> 
> But the issue here is how we get the actual value and show it to user. There 
> are
> two ways to do that:
> 1. When setting value, check and change it to valid one and save it to our 
> cache.

Instead of performing those checks manually, why not simply write the
value and read it back to see the one the hardware has actually
chosen (and cache it)?

Is there any risk in writing an invalid value?

> 2. When getting value, call rdmsr to read the actual value back from HW.
> 
> I think option 1 has better performance and the code looks better.
> 
> > > +     */
> > > +    bool (*check_val)(const struct feat_node *feat, unsigned long *val);
> > >  } *feat_props[FEAT_TYPE_NUM];
> > >  
> [...]
> 
> > >  /* L3 CAT props */
> > >  static void l3_cat_write_msr(unsigned int cos, uint32_t val,
> > >                               enum psr_val_type type)
> > > @@ -446,6 +453,7 @@ static const struct feat_props l3_cat_props = {
> > >      .alt_type = PSR_VAL_TYPE_UNKNOWN,
> > >      .get_feat_info = cat_get_feat_info,
> > >      .write_msr = l3_cat_write_msr,
> > > +    .check_val = cat_check_cbm,
> > >  };
> > 
> > Maybe the introduction of check_val should be a separate patch? It's
> > mostly code movement and some fixup.
> > 
> Ok, may consider it.
> 
> [...]
> 
> > > +static bool mba_check_thrtl(const struct feat_node *feat, unsigned long 
> > > *thrtl)
> > > +{
> > > +    if ( *thrtl > feat->mba_info.thrtl_max )
> > > +        return false;
> > > +
> > > +    /*
> > > +     * Per SDM (chapter "Memory Bandwidth Allocation Configuration"):
> > > +     * 1. Linear mode: In the linear mode the input precision is defined
> > > +     *    as 100-(MBA_MAX). For instance, if the MBA_MAX value is 90, the
> > > +     *    input precision is 10%. Values not an even multiple of the
> > > +     *    precision (e.g., 12%) will be rounded down (e.g., to 10% delay
> > > +     *    applied).
> > > +     * 2. Non-linear mode: Input delay values are powers-of-two from zero
> > > +     *    to the MBA_MAX value from CPUID. In this case any values not a
> > > +     *    power of two will be rounded down the next nearest power of 
> > > two.
> > > +     */
> > > +    if ( feat->mba_info.linear )
> > > +    {
> > > +        unsigned int mod;
> > > +
> > > +        mod = *thrtl % (100 - feat->mba_info.thrtl_max);
> > > +        *thrtl -= mod;
> > > +    }
> > > +    else
> > > +    {
> > > +        /* Not power of 2. */
> > > +        if ( *thrtl && (*thrtl & (*thrtl - 1)) )
> > 
> > This can be joined with the else to avoid another indentation level:
> > 
> > else if ( *thrtl && (*thrtl & (*thrtl - 1)) )
> > ...
> > 
> Thanks!
> 
> > > +            *thrtl = *thrtl & (1 << (flsl(*thrtl) - 1));
> > > +    }
> > > +
> > > +    return true;
> > >  }
> > >  
> [...]
> 
> > >  static void do_write_psr_msrs(void *data)
> > >  {
> > >      const struct cos_write_info *info = data;
> > > -    struct feat_node *feat = info->feature;
> > > -    const struct feat_props *props = info->props;
> > > -    unsigned int i, cos = info->cos, cos_num = props->cos_num;
> > > +    unsigned int i, j, index = 0, array_len = info->array_len, cos = 
> > > info->cos;
> > > +    const uint32_t *val_array = info->val;
> > >  
> > > -    for ( i = 0; i < cos_num; i++ )
> > > +    for ( i = 0; i < ARRAY_SIZE(feat_props); i++ )
> > >      {
> > 
> > index and j can be defined here, they are only used inside of this for
> > loop AFAICT.
> > 
> I think definition of j can be moved into the loop. But index cannot unless I
> declared it to be 'static'. The index is used as a accumulator.

Right, I've got my indentation messed up. Please move j.

Thanks, Roger.

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