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

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

Hi,

I just realize it is Christmas holiday. Merry Christmas!

On 16-12-22 09:13:43, Jan Beulich wrote:
> >>> On 14.12.16 at 05:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > To construct an extendible framework, we need analyze PSR features
> > and abstract the common things and feature specific things. Then,
> > encapsulate them into different data structures.
> > 
> > By analyzing PSR features, we can get below map.
> >                 +------+------+------+
> >       --------->| Dom0 | Dom1 | ...  |
> >       |         +------+------+------+
> >       |            |
> >       |Dom ID      | cos_id of domain
> >       |            V
> >       |        
> > +-----------------------------------------------------------------------------+
> > User --------->| PSR                                                        
> >            
> >       |
> >      Socket ID |  +--------------+---------------+---------------+          
> >                  |
> >                |  | Socket0 Info | Socket 1 Info |    ...        |          
> >                  |
> >                |  +--------------+---------------+---------------+          
> >                  |
> >                |    |                   cos_id=0               cos_id=1     
> >      ...         |
> >                |    |          
> > +-----------------------+-----------------------+-----------+ |
> >                |    |->Ref   : |         ref 0         |         ref 1      
> >    | ...       | |
> >                |    |          
> > +-----------------------+-----------------------+-----------+ |
> >                |    |          
> > +-----------------------+-----------------------+-----------+ |
> >                |    |->L3 CAT: |         cos 0         |         cos 1      
> >    | ...       | |
> >                |    |          
> > +-----------------------+-----------------------+-----------+ |
> >                |    |          
> > +-----------------------+-----------------------+-----------+ |
> >                |    |->L2 CAT: |         cos 0         |         cos 1      
> >    | ...       | |
> >                |    |          
> > +-----------------------+-----------------------+-----------+ |
> >                |    |          
> > +-----------+-----------+-----------+-----------+-----------+ |
> >                |    |->CDP   : | cos0 code | cos0 data | cos1 code | cos1 
> > data | ...       | |
> >                |               
> > +-----------+-----------+-----------+-----------+-----------+ |
> >                
> > +-----------------------------------------------------------------------------+
> > 
> > So, we need define a socket info data structure, 'struct
> > psr_socket_info' to manage information per socket. It contains a
> > reference count array according to COS ID and a feature list to
> > manage all features enabled. Every entry of the reference count
> > array is used to record how many domains are using the COS registers
> > according to the COS ID. For example, L3 CAT and L2 CAT are enabled,
> > Dom1 uses COS_ID=1 registers of both features to save CBM values, like
> > below.
> >         +-------+-------+-------+-----+
> >         | COS 0 | COS 1 | COS 2 | ... |
> >         +-------+-------+-------+-----+
> > L3 CAT  | 0x7ff | 0x1ff | ...   | ... |
> >         +-------+-------+-------+-----+
> > L2 CAT  | 0xff  | 0xff  | ...   | ... |
> >         +-------+-------+-------+-----+
> > 
> > If Dom2 has same CBM values, it can reuse these registers which COS_ID=1.
> > That means, both Dom1 and Dom2 use same COS registers(ID=1) to save same
> > L3/L2 values. So, the value ref[1] is 2 which means 2 domains are using
> > COS_ID 1.
> > 
> > To manage a feature, we need define a feature node data structure,
> > 'struct feat_node', to manage feature's specific HW info, its callback
> > functions (all feature's specific behaviors are encapsulated into these
> > callback functions), and an array of all COS registers values of this
> > feature. CDP is a special feature which uses two entries of the array
> > for one COS ID. So, the number of CDP COS registers is the half of L3
> > CAT. E.g. L3 CAT has 16 COS registers, then CDP has 8 COS registers if
> > it is enabled.
> 
> The special nature of CDP will make some special handling necessary,
> which may need reflection in data structure arrangement. Would you
> mind spelling out here how CDP handling is intended to work?
> 
Yes, CDP has its special handling processes. The main difference has been
described above that CDP has half number of COS registers and uses two entries.
Because of these, I split CDP out from L3 CAT and implement CDP its own feature
callback functions from patch 13 to patch 16. You can check them for details.
Thanks!

> > +/*
> > + * Per SDM 17.17.3.3 'Cache Allocation Technology: Cache Mask 
> > Configuration',
> 
> I think I've asked before to omit section numbers, which tend to
> change. Just the title will be enough.
> 
Sorry for this, will mention title only.

> > +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 {
> > +    /*
> > +     * init_feature is used in cpu initialization process to do feature
> > +     * specific initialization works.
> > +     */
> > +    void (*init_feature)(unsigned int eax, unsigned int ebx,
> > +                         unsigned int ecx, unsigned int edx,
> > +                         struct feat_node *feat,
> > +                         struct psr_socket_info *info);
> > +};
> 
> What is the reason to have a separate structure for this, when you
> don't store a pointer in struct feat_node? If this was inlined there,
> the odd forward declaration of struct feat_node wouldn't be needed
> either. (The same question may apply to struct feat_hw_info.) 
> 
I just want to make codes be clear. If you prefer inline declaration, I think I
should change it as below, right?

struct feat_node {
......
    struct feat_ops {
        ......
    } ops;
    struct feat_hw_info {
        ......
    } info;
......
};

> > +
> > +
> 
> Please avoid double blank lines.
> 
Sorry, will remove it. Thanks!

> Jan

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