Re: [Xen-devel] [PATCH v13 06/10] x86: collect global QoS monitoring information

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.08.14 at 04:17, <dongxiao.xu@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/cpu/intel_cacheinfo.c
> +++ b/xen/arch/x86/cpu/intel_cacheinfo.c
> @@ -12,6 +12,7 @@
>  #include <xen/lib.h>
>  #include <xen/errno.h>
>  #include <asm/processor.h>
> +#include <asm/intel_cacheinfo.h>
>  
>  #define LVL_1_INST   1
>  #define LVL_1_DATA   2
> @@ -81,54 +82,9 @@ static struct _cache_table cache_table[] __cpuinitdata =
>       { 0x00, 0, 0}
>  };
>  
> -
> -enum _cache_type
> -{
> -     CACHE_TYPE_NULL = 0,
> -     CACHE_TYPE_DATA = 1,
> -     CACHE_TYPE_INST = 2,
> -     CACHE_TYPE_UNIFIED = 3
> -};

Rather than moving this and what follows into a header the patch
doesn't even include, I think the proper place would be
asm/cpufeature.h

> +    case XEN_SYSCTL_pqos_monitor_op:
> +        if ( !pqos_monitor_enabled() )
> +            return -ENODEV;
> +
> +        sysctl->u.pqos_monitor_op.flags = 0;

???

> +        switch ( sysctl->u.pqos_monitor_op.cmd )
> +        {
> +        case XEN_SYSCTL_PQOS_MONITOR_cqm_enabled:
> +            sysctl->u.pqos_monitor_op.data =
> +                (pqosm->qm_features & QOS_MONITOR_TYPE_L3) &&
> +                (pqosm->l3m.l3_features & L3_FEATURE_OCCUPANCY);
> +            break;
> +        case XEN_SYSCTL_PQOS_MONITOR_get_total_rmid:
> +            sysctl->u.pqos_monitor_op.data =
> +                pqosm->rmid_max - pqosm->rmid_min + 1;
> +            break;
> +        case XEN_SYSCTL_PQOS_MONITOR_get_l3_upscaling_factor:
> +            sysctl->u.pqos_monitor_op.data = pqosm->l3m.upscaling_factor;
> +            break;
> +        case XEN_SYSCTL_PQOS_MONITOR_get_l3_cache_size:
> +        {
> +            struct cpuid4_info info;
> +
> +            ret = cpuid4_cache_lookup(3, &info);
> +            if ( ret < 0 )
> +                break;
> +
> +            sysctl->u.pqos_monitor_op.data = info.size / 1024; /* in KB unit 
> */

This comment belongs primarily into the public header; whether you
also put it here is up to you.

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -636,6 +636,18 @@ struct xen_sysctl_coverage_op {
>  typedef struct xen_sysctl_coverage_op xen_sysctl_coverage_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_coverage_op_t);
>  
> +#define XEN_SYSCTL_PQOS_MONITOR_get_total_rmid            0
> +#define XEN_SYSCTL_PQOS_MONITOR_get_l3_upscaling_factor   1
> +#define XEN_SYSCTL_PQOS_MONITOR_get_l3_cache_size         2
> +#define XEN_SYSCTL_PQOS_MONITOR_cqm_enabled               3
> +struct xen_sysctl_pqos_monitor_op {
> +    uint32_t cmd;
> +    uint32_t flags;      /* padding variable, may be extended for future use 
> */

Ah, here you (implicitly) say what is needed in the handler: Verify
that the field is zero, not set it to zero.

Jan


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