Re: [Xen-devel] [PATCH v2 04/15] x86: implement data structure and CPU init flow for MBA

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

On 17-08-29 14:44:32, Roger Pau Monn� wrote:
> On Thu, Aug 24, 2017 at 09:14:38AM +0800, Yi Sun wrote:
> > +#define MBA_LINEAR         (1u << 2)
> 
> Why is this shifted by 2?
> 
This is used to mask ECX register to know if MBA linear mode is set or not.

> > +    union {
> > +        struct {
> > +            /* The length of CBM got through CPUID. */
> > +            unsigned int cbm_len;
> > +        } cat_info;
> > +
> > +        struct {
> > +            /* The max throttling value got through CPUID. */
> > +            unsigned int thrtl_max;
> > +            unsigned int linear;
> 
> This seems like it wants to be a boolean?
> 
Ok, will change it to bool.

> > +        } mba_info;
> 
> Just naming the fields 'cat' and 'mba' would probably be enough IMHO,
> but that's just taste I think, and I won't argue if you prefer to
> leave them with the _info suffix.
> 
Got it, thanks!

> > +    /* We reserve cos=0 as default thrtl (0) which means no delay. */
> > +    feat->cos_reg_val[0] = 0;
> > +    wrmsrl(MSR_IA32_PSR_MBA_MASK(0), 0);
> > +
> > +    /* Add this feature into array. */
> > +    info->features[type] = feat;
> > +
> > +    if ( !opt_cpu_info )
> > +        return 0;
> > +
> > +    printk(XENLOG_INFO "MBA: enabled on socket %u, cos_max:%u,"
> > +           "thrtl_max:%u, linear:%u.\n",
> 
> Please try to avoid splitting messages, it makes it hard to grep for
> them afterward.
> 
Ok, but that would exceed 80 characters. Is that acceptable?

> > +/* MBA props */
> > +static bool mba_get_feat_info(const struct feat_node *feat,
> > +                              uint32_t data[], unsigned int array_len)
> > +{
> > +    return false;
> > +}
> 
> Shouldn't this return thrtl_max and whether it's linear?
> 
> > +
> > +static void mba_write_msr(unsigned int cos, uint32_t val,
> > +                          enum psr_val_type type)
> > +{
> 
> And this perform the MSR write? (MSR_IA32_PSR_MBA_MASK...)
> 
Because the patches may not be applied one go. I have to implement these
empty functions and register them into 'mba_props' to avoid crash. IIRC, this
was suggested by Jan before.

> Roger.

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