Re: [Xen-devel] [PATCH v9 03/25] x86: refactor psr: implement main data structures.

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

On 17-03-24 10:19:30, Jan Beulich wrote:
> >>> On 16.03.17 at 12:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > +enum psr_feat_type {
> > +    PSR_SOCKET_L3_CAT = 0,
> 
> Pointless " = 0".
> 
Ok, will remove it.

> > +    PSR_SOCKET_L3_CDP,
> > +    PSR_SOCKET_L2_CAT,
> > +    PSR_SOCKET_MAX_FEAT,
> > +};
> > +
> > +/* CAT/CDP HW info data structure. */
> > +struct psr_cat_hw_info {
> > +    unsigned int cbm_len;
> > +    unsigned int cos_max;
> 
> So you have this field, and ...
> 
> > +};
> > +
> > +/*
> > + * This structure represents one feature.
> > + * feat_ops    - Feature operation callback functions.
> > + * info        - Feature HW info.
> > + * cos_reg_val - Array to store the values of COS registers. One entry 
> > stores
> > + *               the value of one COS register.
> > + *               For L3 CAT and L2 CAT, one entry corresponds to one 
> > COS_ID.
> > + *               For CDP, two entries correspond to one COS_ID. E.g.
> > + *               COS_ID=0 corresponds to cos_reg_val[0] (Data) and
> > + *               cos_reg_val[1] (Code).
> > + * cos_num     - COS registers number that feature uses in one time access.
> > + */
> > +struct feat_node {
> > +    /*
> > +     * This structure defines feature operation callback functions. Every 
> > feature
> > +     * enabled MUST implement such callback functions and register them to 
> > ops.
> > +     *
> > +     * Feature specific behaviors will be encapsulated into these callback
> > +     * functions. Then, the main flows will not be changed when 
> > introducing a new
> > +     * feature.
> > +     */
> > +    struct feat_ops {
> > +        /* get_cos_max is used to get feature's cos_max. */
> > +        unsigned int (*get_cos_max)(const struct feat_node *feat);
> 
> ... you have this op, suggesting that you expect all features
> to have a cos_max. Why don't you then store the value in a
> field which is not per-feature, just like ...
> 
> > +    } ops;
> > +
> > +    /* Encapsulate feature specific HW info here. */
> > +    union {
> > +        struct psr_cat_hw_info cat_info;
> > +    } info;
> > +
> > +    uint32_t cos_reg_val[MAX_COS_REG_CNT];
> > +    unsigned int cos_num;
> 
> ... this. I'm pretty sure that during v8 review I did say that
> this approach should be extended to all pieces of information
> where it can be applied.
> 
I thought this when implementing v9. As cos_max is part of feature HW info, I
thought it would be better to keep it in hw_info structure. Different features
may have different hw_info, so the callback function is needed to get cos_max.
Of course, we can keep a copy in feat_node but it is redundant. How do you
think?

> Also please place the array last, so that accesses to most/all
> other fields have a better chance of working with 8-bit
> displacements.
> 
Sure. Thanks!

> Furthermore, didn't we settle on ops being a pointer to a const
> struct, initialized by taking the address of a static const object?
> There is no reason to duplicate all the pointers in every node.
> 
Will correct this.

> > +struct psr_socket_info {
> > +    /*
> > +     * It maps to values defined in 'enum psr_feat_type' below. Value in 
> > 'enum
> > +     * psr_feat_type' means the bit position.
> > +     * bit 0:   L3 CAT
> > +     * bit 1:   L3 CDP
> > +     * bit 2:   L2 CAT
> > +     */
> > +    unsigned int feat_mask;
> 
> Comment or not I don't understand what use this mask is, and
> this is again something which I'm pretty sure I've mentioned in
> v8 review, when the switch to ...
> 
> > +    struct feat_node *features[PSR_SOCKET_MAX_FEAT];
> > +    unsigned int cos_ref[MAX_COS_REG_CNT];
> 
> ... this array was suggested by Roger. The pointers in the
> array being non-NULL can - afaict - easily fulfill the role of
> the mask bits, so the latter are redundant.
> 
I thought 'feat_mask' can facilitate things handling. If we check feature array
to know if any feature has been initialized, we have to iterate the array. But
it is simple to check 'feat_mask'.
1. In 'psr_cpu_init', we can use it to check if initialization on the socket
   has been done.
    if ( info->feat_mask )
        goto assoc_init;
2. Same purpose in 'psr_assoc_init'.
    if ( info->feat_mask )
        psra->cos_mask = ((1ull << get_count_order(cos_max)) - 1) <<
                         PSR_ASSOC_REG_SHIFT;
3. Same purpose in 'get_socket_info'.
    if ( !socket_info[socket].feat_mask )
 
What is your advice? Thanks!

> Jan

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