Re: [Xen-devel] [PATCH v6 4/7] x86: collect CQM information from all sockets

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.12.13 at 10:38, Dongxiao Xu <dongxiao.xu@xxxxxxxxx> wrote:
> @@ -126,6 +127,12 @@ bool_t system_supports_cqm(void)
>      return !!cqm;
>  }
>  
> +unsigned int get_cqm_count(void)
> +{
> +    ASSERT(system_supports_cqm());
> +    return cqm->max_rmid + 1;
> +}
> +
>  int alloc_cqm_rmid(struct domain *d)
>  {
>      int rc = 0;
> @@ -170,6 +177,48 @@ void free_cqm_rmid(struct domain *d)
>      d->arch.pqos_cqm_rmid = 0;
>  }
>  
> +static void read_cqm_data(void *arg)
> +{
> +    uint64_t cqm_data;
> +    unsigned int rmid;
> +    int socket = cpu_to_socket(smp_processor_id());
> +    struct xen_socket_cqmdata *data = arg;
> +    unsigned long flags, i;

Either i can be "unsigned int" ...

> +
> +    ASSERT(system_supports_cqm());
> +
> +    if ( socket < 0 )
> +        return;
> +
> +    spin_lock_irqsave(&cqm_lock, flags);
> +    for ( rmid = cqm->min_rmid; rmid <= cqm->max_rmid; rmid++ )
> +    {
> +        if ( cqm->rmid_to_dom[rmid] == DOMID_INVALID )
> +            continue;
> +
> +        wrmsr(MSR_IA32_QOSEVTSEL, QOS_MONITOR_EVTID_L3, rmid);
> +        rdmsrl(MSR_IA32_QMC, cqm_data);
> +
> +        i = socket * (cqm->max_rmid + 1) + rmid;

... or this calculation needs one of the two operands of * cast
to "unsigned long".

> +        data[i].valid = !(cqm_data & IA32_QM_CTR_ERROR_MASK);
> +        if ( data[i].valid )
> +        {
> +            data[i].l3c_occupancy = cqm_data * cqm->upscaling_factor;
> +            data[i].socket = socket;
> +            data[i].domid = cqm->rmid_to_dom[rmid];
> +        }
> +    }
> +    spin_unlock_irqrestore(&cqm_lock, flags);
> +}

Also, please clarify why the locking here is necessary: You don't
seem to be modifying global data, and the only possibly mutable
thing you read is cqm->rmid_to_dom[]. A race on that one with
an addition/deletion doesn't appear to be problematic though.

> +void get_cqm_info(cpumask_t *cpu_cqmdata_map, struct xen_socket_cqmdata 
> *data)

const cpumask_t *

> +    case XEN_SYSCTL_getcqminfo:
> +    {
> +        struct xen_socket_cqmdata *info;
> +        uint32_t num_sockets;
> +        uint32_t num_rmids;
> +        cpumask_t cpu_cqmdata_map;

Unless absolutely avoidable, not CPU masks on the stack please.

> +
> +        if ( !system_supports_cqm() )
> +        {
> +            ret = -ENODEV;
> +            break;
> +        }
> +
> +        select_socket_cpu(&cpu_cqmdata_map);
> +
> +        num_sockets = min((unsigned int)cpumask_weight(&cpu_cqmdata_map) + 1,
> +                          sysctl->u.getcqminfo.num_sockets);
> +        num_rmids = get_cqm_count();
> +        info = xzalloc_array(struct xen_socket_cqmdata,
> +                             num_rmids * num_sockets);

While unlikely right now, you ought to consider the case of this
multiplication overflowing.

Also - how does the caller know how big the buffer needs to be?
Only num_sockets can be restricted by it...

And what's worse - you allow the caller to limit num_sockets and
allocate info based on this limited value, but you don't restrict
cpu_cqmdata_map to just the socket covered, i.e. if the caller
specified a lower number, then you'll corrupt memory.

And finally, I think the total size of the buffer here can easily
exceed a page, i.e. this then ends up being a non-order-0
allocation, which may _never_ succeed (i.e. the operation is
then rendered useless). I guest it'd be better to e.g. vmap()
the MFNs underlying the guest buffer.

> +        if ( !info )
> +        {
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        get_cqm_info(&cpu_cqmdata_map, info);
> +
> +        if ( copy_to_guest_offset(sysctl->u.getcqminfo.buffer,
> +                                  0, info, num_rmids * num_sockets) )

If the offset is zero anyway, why do you use copy_to_guest_offset()
rather than copy_to_guest()?

> +        {
> +            ret = -EFAULT;
> +            xfree(info);
> +            break;
> +        }
> +
> +        sysctl->u.getcqminfo.num_rmids = num_rmids;
> +        sysctl->u.getcqminfo.num_sockets = num_sockets;
> +
> +        if ( copy_to_guest(u_sysctl, sysctl, 1) )

__copy_to_guest() is sufficient here.

Jan

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