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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.03.17 at 12:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> +enum psr_feat_type {
> +    PSR_SOCKET_L3_CAT = 0,

Pointless " = 0".

> +    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.

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

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.

> +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.

Jan


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