Re: [Xen-devel] [PATCH v10 1/6] x86: detect and initialize Cache QoS Monitoring feature

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.03.14 at 17:33, <dongxiao.xu@xxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> >>> On 26.03.14 at 07:35, <dongxiao.xu@xxxxxxxxx> wrote:
>> > +static int cqm_add_socket(int socket)
>> 
>> unsigned int.
> 
> This function returns negative return values in error case.

Oh, sorry, this wasn't precise enough - I meant this for the parameter.

>> > +    /* According to Intel SDM, the possible maximum rmid number is 2^10 = 
>> > 1024,
>> > +     * thus one page is enough to hold cqm->rmid_to_dom structure */
>> > +    cqm->rmid_to_dom = alloc_xenheap_page();
>> 
>> But please validate that this condition is being met (not necessarily here,
>> perhaps rather where rmid_max gets determined).
> 
> Okay, will add one ASSERT() to validate this condition.

An ASSERT() is the wrong thing here - non-debug builds would then
happily continue. Just check the value to be in range, and stay away
from enabling cache QoS if it's not.

>> > +
>> > +    /* Allocate the buffer that holds MFNs of per-socket CQM LLC */
>> > +    order = get_order_from_bytes(sizeof(unsigned long) * NR_CPUS);
>> > +    cqm->socket_l3c_mfn = alloc_xenheap_pages(order, 0);
>> 
>> You index this buffer by socket ID, yet socket ID values aren't
>> necessarily linear/contiguous, and hence I don't think there are
>> really any guarantees that the ID would be less than NR_CPUS. For
>> the n-th time: I don't think you'll get around doing this properly, and
>> I still think sufficient data should be retrievable from ACPI to
>> determine a proper upper bound here.
> 
> I think we need to assume socket ID is less than NR_CPUS, otherwise many of 
> current Xen's code is wrong, say credit scheduler 2.
> xen/common/sched_credit2.c: init_pcpu()
>       rqi = cpu_to_socket(cpu);
>       rqd=prv->rqd + rqi;
> Here the priv->rqd is defined as:
>       struct csched_runqueue_data rqd[NR_CPUS];

Bad precedents are never a reason to introduce more problems.

> For getting socket info from ACPI, do you mean ACPI MADT table?
> It can enumerate the existing APIC structures, which based on the fact that 
> the CPU is already plugged in the socket.
> Per my understanding, I don't think it can enumerate empty CPU sockets.
> 
> Can you help to elaborate more about how to get all socket number from ACPI?

Iirc disabled CPUs (i.e. hot-pluggable ones) are still listed in MADT,
including their LAPIC ID (which the socket number is being calculated
from).

Jan


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