|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v8 03/24] x86: refactor psr: implement main data structures.
On Wed, Feb 15, 2017 at 04:49:18PM +0800, Yi Sun 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. CDP uses the COS registers array as below.
>
>
> +-----------+-----------+-----------+-----------+-----------+
> CDP cos_reg_val[] index: | 0 | 1 | 2 | 3 |
> ... |
>
> +-----------+-----------+-----------+-----------+-----------+
> value: | cos0 code | cos0 data | cos1 code | cos1 data |
> ... |
>
> +-----------+-----------+-----------+-----------+-----------+
>
> For more details, please refer SDM and patches to implement 'get value' and
> 'set value'.
I would recommend that you merge this with a patch that actually makes use of
this structures, or else it's hard to review it's usage IMHO.
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
> xen/arch/x86/psr.c | 108
> ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 107 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index 96a8589..5acd9ca 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -13,16 +13,122 @@
> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> * more details.
> */
> -#include <xen/init.h>
> #include <xen/cpu.h>
> #include <xen/err.h>
> +#include <xen/init.h>
> +#include <xen/list.h>
> #include <xen/sched.h>
> #include <asm/psr.h>
>
> +/*
> + * Terminology:
> + * - CAT Cache Allocation Technology
> + * - CBM Capacity BitMasks
> + * - CDP Code and Data Prioritization
> + * - COS/CLOS Class of Service. Also mean COS registers.
> + * - COS_MAX Max number of COS for the feature (minus 1)
> + * - MSRs Machine Specific Registers
> + * - PSR Intel Platform Shared Resource
> + */
> +
> #define PSR_CMT (1<<0)
> #define PSR_CAT (1<<1)
> #define PSR_CDP (1<<2)
>
> +/*
> + * Per SDM chapter 'Cache Allocation Technology: Cache Mask Configuration',
> + * the MSRs ranging from 0C90H through 0D0FH (inclusive), enables support for
> + * up to 128 L3 CAT Classes of Service. The COS_ID=[0,127].
> + *
> + * The MSRs ranging from 0D10H through 0D4FH (inclusive), enables support for
> + * up to 64 L2 CAT COS. The COS_ID=[0,63].
> + *
> + * So, the maximum COS register count of one feature is 128.
> + */
> +#define MAX_COS_REG_CNT 128
> +
> +/*
> + * PSR features are managed per socket. Below structure defines the members
> + * used to manage these features.
> + * feat_mask - Mask used to record features enabled on socket. There may be
> + * some features enabled at same time.
> + * nr_feat - Record how many features enabled.
> + * feat_list - A list used to manage all features enabled.
> + * cos_ref - A reference count array to record how many domains are using
> the
> + * COS_ID.
> + * Every entry of cos_ref corresponds to one COS ID.
> + * ref_lock - A lock to protect cos_ref.
> + */
> +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;
> + unsigned int nr_feat;
> + struct list_head feat_list;
Isn't it a little bit overkill to use a list when there can only be a maximum
of 3 features? (and the feat_mask is currently 32bits, so I guess you don't
really foresee having more than 32 features).
I would suggest using:
struct feat_node *features[PSR_SOCKET_MAX_FEAT];
(PSR_SOCKET_MAX_FEAT comes from the expansion of the enum below). Then you can
simply use the enum value of each feature as the position of it's corresponding
feat_node into the array.
> + unsigned int cos_ref[MAX_COS_REG_CNT];
> + spinlock_t ref_lock;
> +};
> +
> +enum psr_feat_type {
> + PSR_SOCKET_L3_CAT = 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;
> +};
> +
> +/* Encapsulate feature specific HW info here. */
> +struct feat_hw_info {
> + union {
> + struct psr_cat_hw_info l3_cat_info;
> + };
> +};
Why don't you use an union here directly, instead of encapsulating an union
inside of a struct?
union feat_hw_info {
struct psr_cat_hw_info l3_cat_info;
};
> +
> +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);
> +};
> +
> +/*
> + * This structure represents one feature.
> + * feature - Which feature it is.
> + * 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).
> + * list - Feature list.
> + */
> +struct feat_node {
> + enum psr_feat_type feature;
If you index them in an array with the key being psr_feat_type I don't think
you need that field.
> + struct feat_ops ops;
I would place the function hooks in this struct directly, instead of nesting
them inside of another struct. The hooks AFAICT are shared between all the
different PSR features.
> + struct feat_hw_info info;
Same with this, you can place the actual union for storage here directly,
instead of nesting it inside of feat_hw_info, so:
union {
struct psr_cat_hw_info l3_cat_info;
} hw_info;
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |