Re: [PATCH v5 0/8] Display IO topology when PXM data is available (plus some cleanup)

[Julien Grall <julien.grall@linaro.org>]

Hi,

On 23/03/15 13:47, Boris Ostrovsky wrote:
> On 03/23/2015 08:42 AM, Julien Grall wrote:
>> On 19/03/15 21:53, Boris Ostrovsky wrote:
>>> A few patches that add interface for querying hypervisor about device
>>> topology and allow 'xl info -n' display this information if PXM object
>>> is provided by ACPI.
>>>
>>> This series also makes some optimizations and cleanup of current CPU
>>> topology and NUMA sysctl queries.
>> I saw that some of these patches was pushed upstream last week. It
>> actually breaks compilation on ARM.
>>
>> While the first error is trivial to fix (it's a missing include
>> xen/numa.h in xen/pci.h ). The second one is more difficult
>>
>> sysctl.c: In function ‘do_sysctl’:
>> sysctl.c:349:42: error: ‘BAD_APICID’ undeclared (first use in this
>> function)
>>                       if ( cputopo.core == BAD_APICID )
>>                                            ^
>> The value BAD_APICID doesn't have any meaning on ARM. Therefore the
>> usage in common code looks wrong to me. I'm not sure what should be the
>> testing value here given that cpu_to_core is not yet correctly
>> implemented on ARM.
> 
> How about this (only x86 compile-tested). And perhaps, while at it, fix
> types for cpu_core_id and phys_proc_id.

This patch build on ARM/ARM64.

> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index c73dfc9..b319be7 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -354,10 +354,10 @@ long
> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>                  if ( cpu_present(i) )
>                  {
>                      cputopo.core = cpu_to_core(i);
> -                    if ( cputopo.core == BAD_APICID )
> +                    if ( cputopo.core == INVALID_COREID )
>                          cputopo.core = XEN_INVALID_CORE_ID;
>                      cputopo.socket = cpu_to_socket(i);
> -                    if ( cputopo.socket == BAD_APICID )
> +                    if ( cputopo.socket == INVALID_SOCKETID )
>                          cputopo.socket = XEN_INVALID_SOCKET_ID;
>                      cputopo.node = cpu_to_node(i);
>                      if ( cputopo.node == NUMA_NO_NODE )
> diff --git a/xen/include/asm-x86/processor.h
> b/xen/include/asm-x86/processor.h
> index 87d80ff..07cee2a 100644
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -214,8 +214,19 @@ extern void detect_extended_topology(struct
> cpuinfo_x86 *c);
> 
>  extern void detect_ht(struct cpuinfo_x86 *c);
> 
> -#define cpu_to_core(_cpu)   (cpu_data[_cpu].cpu_core_id)
> -#define cpu_to_socket(_cpu) (cpu_data[_cpu].phys_proc_id)
> +inline int cpu_to_core(unsigned cpu)
> +{
> +    if ( cpu_data[cpu].cpu_core_id == BAD_APICID )
> +        return INVALID_COREID;
> +    return cpu_data[cpu].cpu_core_id;
> +}
> +
> +inline int cpu_to_socket(unsigned cpu)
> +{
> +    if ( cpu_data[cpu].phys_proc_id == BAD_APICID )
> +        return INVALID_SOCKETID;
> +    return cpu_data[cpu].phys_proc_id;
> +}
> 
>  unsigned int apicid_to_socket(unsigned int);
> 
> diff --git a/xen/include/xen/smp.h b/xen/include/xen/smp.h
> index 6febb56..0b4c660 100644
> --- a/xen/include/xen/smp.h
> +++ b/xen/include/xen/smp.h
> @@ -3,6 +3,9 @@
> 
>  #include <asm/smp.h>
> 
> +#define INVALID_COREID   -1
> +#define INVALID_SOCKETID -1
> +
>  /*
>   * stops all CPUs but the current one:
>   */

Regards,

-- 
Julien Grall