Re: [Xen-devel] [PATCH v4 1/4] x86: Support enable CDP by boot parameter and add get CDP status

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.09.15 at 11:35, <he.chen@xxxxxxxxxxxxxxx> wrote:
> Add boot parameter `psr=cdp` to enable CDP at boot time.
> Intel Code/Data Prioritization(CDP) feature is based on CAT. Note that
> cos_max would be half when CDP is on. struct psr_cat_cbm is extended to
> support CDP operation. Extend psr_get_cat_l3_info sysctl to get CDP
> status.
> 
> Signed-off-by: He Chen <he.chen@xxxxxxxxxxxxxxx>

Brief list of changes in v4 missing here.

> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -21,9 +21,16 @@
>  
>  #define PSR_CMT        (1<<0)
>  #define PSR_CAT        (1<<1)
> +#define PSR_CDP        (1<<2)
>  
>  struct psr_cat_cbm {
> -    uint64_t cbm;
> +    union {
> +        uint64_t cbm;
> +        struct {
> +            uint64_t code;
> +            uint64_t data;
> +        };
> +    } u;

Didn't we already agree to remove this unnecessary u?

> @@ -261,8 +270,14 @@ static struct psr_cat_socket_info 
> *get_cat_socket_info(unsigned int socket)
>      return cat_socket_info + socket;
>  }
>  
> +static inline bool_t cdp_is_enabled(unsigned int socket,
> +                                    unsigned long *cdp_socket_enable)
> +{
> +    return (cdp_socket_enable && test_bit(socket, cdp_socket_enable));

Stray pair of parentheses.

> @@ -387,16 +421,16 @@ int psr_set_l3_cbm(struct domain *d, unsigned int 
> socket, uint64_t cbm)
>      }
>  
>      cos = found - map;
> -    if ( found->cbm != cbm )
> +    if ( found->u.cbm != cbm )
>      {
> -        int ret = write_l3_cbm(socket, cos, cbm);
> +        int ret = write_l3_cbm(socket, cos, cbm, 0, 0);

I think it would be more natural to pass cbm twice instead of passing
zero for cbm_data. The change also looks misplaced here, as there's
no other caller of write_l3_cbm() until (presumably) the next patch.

> --- a/xen/include/asm-x86/psr.h
> +++ b/xen/include/asm-x86/psr.h
> @@ -27,6 +27,15 @@
>  /* L3 Monitoring Features */
>  #define PSR_CMT_L3_OCCUPANCY           0x1
>  
> +/* CDP Capability */
> +#define PSR_CAT_CDP_CAPABILITY       (1u << 2)
> +
> +/* L3 CDP Enable bit*/
> +#define PSR_L3_QOS_CDP_ENABLE_BIT       0x0
> +
> +/* L3 CDP flag bit */
> +#define PSR_CAT_FLAG_L3_CDP       (1u << 0)

Afaict this is the same as ...

> @@ -704,6 +704,8 @@ struct xen_sysctl_psr_cat_op {
>          struct {
>              uint32_t cbm_len;   /* OUT: CBM length */
>              uint32_t cos_max;   /* OUT: Maximum COS */
> +            #define XEN_SYSCTL_PSR_CAT_L3_CDP       (1u << 0)

... this. In which case only the latter (public) one should be kept.

Also at the very least the # belongs in column 1.

Jan


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