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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 03/23/2015 08:42 AM, Julien Grall wrote:
> Hi,
>
> 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.

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:
   */



> It would be nice to at least build test it ARM/ARM64 before pushing
> patches which modify common code.

I unfortunately have no ability to build on ARM (but I should have 
realized that APICID has no meaning there).


-boris