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

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, January 20, 2014 9:52 PM
> To: Xu, Dongxiao
> Cc: andrew.cooper3@xxxxxxxxxx; dario.faggioli@xxxxxxxxxx;
> Ian.Campbell@xxxxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx;
> stefano.stabellini@xxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: Re: [PATCH v6 4/7] x86: collect CQM information from all sockets
> 
> >>> 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".

Will adopt this way in following patch.

> 
> > +        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.

Will use the spin_lock() in following patch.

> 
> > +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.

Okay, will allocate it by "xzalloc" function.

> 
> > +
> > +        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.

Currently the caller (libxc) sets big num_rmid and num_sockets which are the 
maximum values that could be available inside the hypervisor.
If you think this approach is not enough to ensure the security, what about the 
caller (libxc) issue an hypercall to get the two values from hypervisor, then 
allocating the same size of num_rmid and num_sockets?

> 
> 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.

Do you mean we check the size of the total size, and allocate MFNs one by one, 
then vmap them?

> 
> > +        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()?

Okay.

> 
> > +        {
> > +            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.

Okay.

Thanks,
Dongxiao

> 
> Jan

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