[PATCH 0/2] A couple of regression fixes on ARM

[PATCH 0/2] A couple of regression fixes on ARM

[Boris Ostrovsky]

These two patches fix ARM regressions reported by Julien.

-boris


Boris Ostrovsky (2):
  pci: Include asm/numa.h in pci.h
  x86: Don't use BAD_APICID for non-APICID fields

 xen/arch/x86/cpu/common.c       |    6 +++---
 xen/arch/x86/smpboot.c          |    6 +++---
 xen/common/sched_credit2.c      |    6 +++---
 xen/common/sysctl.c             |    4 ++--
 xen/include/asm-x86/processor.h |    6 +++---
 xen/include/asm-x86/smp.h       |    3 ++-
 xen/include/xen/pci.h           |    1 +
 xen/include/xen/smp.h           |    3 +++
 8 files changed, 20 insertions(+), 15 deletions(-)

[PATCH 1/2] pci: Include asm/numa.h in pci.h

[Boris Ostrovsky]

Commit 4fa6b0bacf9c ("pci: stash device's PXM information in struct
pci_dev") added node field to xen/include/xen/pci.h. Its type,
nodeid_t, is defined in asm/numa.h and we should include this file
explicitly in pci.h

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reported-by: Julien Grall <julien.grall@linaro.org>
---
 xen/include/xen/pci.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index b281790..4377f3e 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -14,6 +14,7 @@
 #include <xen/pci_regs.h>
 #include <xen/pfn.h>
 #include <asm/device.h>
+#include <asm/numa.h>
 #include <asm/pci.h>
 
 /*
-- 
1.7.1

Re: [PATCH 1/2] pci: Include asm/numa.h in pci.h

[Ian Campbell]

On Mon, 2015-03-23 at 15:54 -0400, Boris Ostrovsky wrote:
> Commit 4fa6b0bacf9c ("pci: stash device's PXM information in struct
> pci_dev") added node field to xen/include/xen/pci.h. Its type,
> nodeid_t, is defined in asm/numa.h and we should include this file
> explicitly in pci.h
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Reported-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  xen/include/xen/pci.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index b281790..4377f3e 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -14,6 +14,7 @@
>  #include <xen/pci_regs.h>
>  #include <xen/pfn.h>
>  #include <asm/device.h>
> +#include <asm/numa.h>
>  #include <asm/pci.h>
>  
>  /*

[PATCH 2/2] x86: Don't use BAD_APICID for non-APICID fields

[Boris Ostrovsky]

BAD_APICID is used by cpuinfo_x86's phys_proc_id, cpu_core_id
and compute_unit_id even though these fields don't hold an APIC ID
itself but rather its derivative.

Provide appropriate macros for each of those three (and make them
unsigned).

This also fixes regression introduced by commit 2090f14c5cbd ("sysctl:
make XEN_SYSCTL_topologyinfo sysctl a little more efficient") which
leaked BAD_APICID into common code, breaking ARM.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reported-by: Julien Grall <julien.grall@linaro.org>
---

I left sysctl with

    cputopo.core = cpu_to_core(i);
    if ( cputopo.core == INVALID_COREID )
        cputopo.core = XEN_INVALID_CORE_ID;
    cputopo.socket = cpu_to_socket(i);
    if ( cputopo.socket == INVALID_SOCKETID )
        cputopo.socket = XEN_INVALID_SOCKET_ID;

since this is the only place that would use suggested inlines for external
(public) calls.



 xen/arch/x86/cpu/common.c       |    6 +++---
 xen/arch/x86/smpboot.c          |    6 +++---
 xen/common/sched_credit2.c      |    6 +++---
 xen/common/sysctl.c             |    4 ++--
 xen/include/asm-x86/processor.h |    6 +++---
 xen/include/asm-x86/smp.h       |    3 ++-
 xen/include/xen/smp.h           |    3 +++
 7 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 5c8d3c2..5c5e8e9 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -277,9 +277,9 @@ void __cpuinit identify_cpu(struct cpuinfo_x86 *c)
 	c->x86_max_cores = 1;
 	c->x86_num_siblings = 1;
 	c->x86_clflush_size = 0;
-	c->phys_proc_id = BAD_APICID;
-	c->cpu_core_id = BAD_APICID;
-	c->compute_unit_id = BAD_APICID;
+	c->phys_proc_id = INVALID_SOCKETID;
+	c->cpu_core_id = INVALID_COREID;
+	c->compute_unit_id = INVALID_CUID;
 	memset(&c->x86_capability, 0, sizeof c->x86_capability);
 
 	generic_identify(c);
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 314e253..e9e60e5 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -792,9 +792,9 @@ remove_siblinginfo(int cpu)
         cpumask_clear_cpu(cpu, per_cpu(cpu_sibling_mask, sibling));
     cpumask_clear(per_cpu(cpu_sibling_mask, cpu));
     cpumask_clear(per_cpu(cpu_core_mask, cpu));
-    c[cpu].phys_proc_id = BAD_APICID;
-    c[cpu].cpu_core_id = BAD_APICID;
-    c[cpu].compute_unit_id = BAD_APICID;
+    c[cpu].phys_proc_id = INVALID_SOCKETID;
+    c[cpu].cpu_core_id = INVALID_COREID;
+    c[cpu].compute_unit_id = INVALID_CUID;
     cpumask_clear_cpu(cpu, &cpu_sibling_setup_map);
 }
 
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index be6859a..0c5f3ff 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -1952,7 +1952,7 @@ static void deactivate_runqueue(struct csched2_private *prv, int rqi)
 
 static void init_pcpu(const struct scheduler *ops, int cpu)
 {
-    int rqi;
+    unsigned rqi;
     unsigned long flags;
     struct csched2_private *prv = CSCHED2_PRIV(ops);
     struct csched2_runqueue_data *rqd;
@@ -1977,7 +1977,7 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
     else
         rqi = cpu_to_socket(cpu);
 
-    if ( rqi < 0 )
+    if ( rqi == INVALID_SOCKETID )
     {
         printk("%s: cpu_to_socket(%d) returned %d!\n",
                __func__, cpu, rqi);
@@ -2020,7 +2020,7 @@ csched2_alloc_pdata(const struct scheduler *ops, int cpu)
 {
     /* Check to see if the cpu is online yet */
     /* Note: cpu 0 doesn't get a STARTING callback */
-    if ( cpu == 0 || cpu_to_socket(cpu) >= 0 )
+    if ( cpu == 0 || cpu_to_socket(cpu) != INVALID_SOCKETID )
         init_pcpu(ops, cpu);
     else
         printk("%s: cpu %d not online yet, deferring initializatgion\n",
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index fc7b3d5..4566b5c 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -346,10 +346,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..a9b4e06 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -180,9 +180,9 @@ struct cpuinfo_x86 {
     __u32 booted_cores;  /* number of cores as seen by OS */
     __u32 x86_num_siblings; /* cpuid logical cpus per chip value */
     __u32 apicid;
-    int   phys_proc_id; /* package ID of each logical CPU */
-    int   cpu_core_id; /* core ID of each logical CPU*/
-    int   compute_unit_id; /* AMD compute unit ID of each logical CPU */
+    __u32 phys_proc_id;    /* package ID of each logical CPU */
+    __u32 cpu_core_id;     /* core ID of each logical CPU*/
+    __u32 compute_unit_id; /* AMD compute unit ID of each logical CPU */
     unsigned short x86_clflush_size;
 } __cacheline_aligned;
 
diff --git a/xen/include/asm-x86/smp.h b/xen/include/asm-x86/smp.h
index 81f8610..c4b55fb 100644
--- a/xen/include/asm-x86/smp.h
+++ b/xen/include/asm-x86/smp.h
@@ -16,7 +16,8 @@
 #include <asm/mpspec.h>
 #endif
 
-#define BAD_APICID -1U
+#define BAD_APICID   -1U
+#define INVALID_CUID (~0U)   /* AMD Compute Unit ID */
 #ifndef __ASSEMBLY__
 
 /*
diff --git a/xen/include/xen/smp.h b/xen/include/xen/smp.h
index 6febb56..334ab6c 100644
--- a/xen/include/xen/smp.h
+++ b/xen/include/xen/smp.h
@@ -3,6 +3,9 @@
 
 #include <asm/smp.h>
 
+#define INVALID_COREID   (~0U)
+#define INVALID_SOCKETID (~0U)
+
 /*
  * stops all CPUs but the current one:
  */
-- 
1.7.1

Re: [PATCH 2/2] x86: Don't use BAD_APICID for non-APICID fields

[Jan Beulich]

>>> On 23.03.15 at 20:54, <boris.ostrovsky@oracle.com> wrote:
> BAD_APICID is used by cpuinfo_x86's phys_proc_id, cpu_core_id
> and compute_unit_id even though these fields don't hold an APIC ID
> itself but rather its derivative.
> 
> Provide appropriate macros for each of those three (and make them
> unsigned).
> 
> This also fixes regression introduced by commit 2090f14c5cbd ("sysctl:
> make XEN_SYSCTL_topologyinfo sysctl a little more efficient") which
> leaked BAD_APICID into common code, breaking ARM.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Reported-by: Julien Grall <julien.grall@linaro.org>
> ---
> 
> I left sysctl with
> 
>     cputopo.core = cpu_to_core(i);
>     if ( cputopo.core == INVALID_COREID )
>         cputopo.core = XEN_INVALID_CORE_ID;
>     cputopo.socket = cpu_to_socket(i);
>     if ( cputopo.socket == INVALID_SOCKETID )
>         cputopo.socket = XEN_INVALID_SOCKET_ID;
> 
> since this is the only place that would use suggested inlines for external
> (public) calls.

But again - why did you not avoid the new #defines and use the
XEN_-prefixed ones right away?

> --- a/xen/include/asm-x86/smp.h
> +++ b/xen/include/asm-x86/smp.h
> @@ -16,7 +16,8 @@
>  #include <asm/mpspec.h>
>  #endif
>  
> -#define BAD_APICID -1U
> +#define BAD_APICID   -1U
> +#define INVALID_CUID (~0U)   /* AMD Compute Unit ID */

Touching the BAD_APICID line should have resulted in also adding
parentheses there.

Jan

Re: [PATCH 2/2] x86: Don't use BAD_APICID for non-APICID fields

[Boris Ostrovsky]

On 03/24/2015 04:07 AM, Jan Beulich wrote:
>>>> On 23.03.15 at 20:54, <boris.ostrovsky@oracle.com> wrote:
>> BAD_APICID is used by cpuinfo_x86's phys_proc_id, cpu_core_id
>> and compute_unit_id even though these fields don't hold an APIC ID
>> itself but rather its derivative.
>>
>> Provide appropriate macros for each of those three (and make them
>> unsigned).
>>
>> This also fixes regression introduced by commit 2090f14c5cbd ("sysctl:
>> make XEN_SYSCTL_topologyinfo sysctl a little more efficient") which
>> leaked BAD_APICID into common code, breaking ARM.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Reported-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>
>> I left sysctl with
>>
>>      cputopo.core = cpu_to_core(i);
>>      if ( cputopo.core == INVALID_COREID )
>>          cputopo.core = XEN_INVALID_CORE_ID;
>>      cputopo.socket = cpu_to_socket(i);
>>      if ( cputopo.socket == INVALID_SOCKETID )
>>          cputopo.socket = XEN_INVALID_SOCKET_ID;
>>
>> since this is the only place that would use suggested inlines for external
>> (public) calls.
> But again - why did you not avoid the new #defines and use the
> XEN_-prefixed ones right away?

I did want to use XEN_* macros but then I realized that I need a new 
#define for cpuinfo_x86.compute_unit_id as well. This #define does not 
need to be public so for consistency I thought that having 
hypervisor-internal INVALID_* macros would be better.


-boris

Re: [PATCH 0/2] A couple of regression fixes on ARM

[Julien Grall]

Hi Boris,

On 23/03/2015 19:54, Boris Ostrovsky wrote:
> These two patches fix ARM regressions reported by Julien.

Thank you for the 2 fixes:

Tested-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall