| Message ID | 1379510122-9467-4-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 09/18/2013 02:15 PM, Julien Grall wrote: > The GIC mapping of CPU interfaces does not necessarily match the logical > CPU numbering. > > When Xen wants to send an SGI to specific CPU, it needs to use the GIC CPU ID. > It can be retrieved from ITARGETSR0, in fact when this field is read, the GIC > will return a value that corresponds only to the processor reading the register. > So Xen can use the PPI 0 to initialize the mapping. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Changes in v3: > - Correctly create the mask in gic_cpu_mask > > Changes in v2: > - Use per-cpu variable instead of an array > - Add comment for NR_GIC_CPU_IF > --- > xen/arch/arm/gic.c | 37 ++++++++++++++++++++++++++++++------- > 1 file changed, 30 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index b969d23..4061691 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -57,6 +57,29 @@ static DEFINE_PER_CPU(uint64_t, lr_mask); > > static unsigned nr_lrs; > > +/* The GIC mapping of CPU interfaces does not necessarily match the > + * logical CPU numbering. Let's use mapping as returned by the GIC > + * itself > + */ > +static DEFINE_PER_CPU(u8, gic_cpu_id); > + Following the discussion on another thread (http://lists.xen.org/archives/html/xen-devel/2013-09/msg02072.html), we need to initialize each CPU ID to 0xff by default, to be able to bring up secondary CPUs with an SGI (send_SGI_mask). But as I understand, per_cpu area for a give area is initialized in cpu_up and there is no way to give a default value. So user could call send_SGI_mask to early function and use an invalid CPU area. IMHO, this solution is too fragile and I would like to use the same solution as my first version (ie with an array of 8 cells). Ian, is ok for you? > +/* Maximum cpu interface per GIC */ > +#define NR_GIC_CPU_IF 8 > + > +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) > +{ > + unsigned int cpu; > + unsigned int mask = 0; > + > + for_each_cpu(cpu, cpumask) > + { > + ASSERT(cpu < NR_GIC_CPU_IF); > + mask |= per_cpu(gic_cpu_id, cpu); > + } > + > + return mask; > +} > + > unsigned int gic_number_lines(void) > { > return gic.lines; > @@ -189,9 +212,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, > { > volatile unsigned char *bytereg; > uint32_t cfg, edgebit; > - unsigned int mask = cpumask_bits(cpu_mask)[0]; > - > - ASSERT(!(mask & ~0xff)); /* Target bitmap only support 8 CPUS */ > + unsigned int mask = gic_cpu_mask(cpu_mask); > > /* Set edge / level */ > cfg = GICD[GICD_ICFGR + irq / 16]; > @@ -300,6 +321,8 @@ static void __cpuinit gic_cpu_init(void) > { > int i; > > + this_cpu(gic_cpu_id) = GICD[GICD_ITARGETSR] & 0xff; > + > /* The first 32 interrupts (PPI and SGI) are banked per-cpu, so > * even though they are controlled with GICD registers, they must > * be set up here with the other per-cpu state. */ > @@ -431,13 +454,13 @@ void __init gic_init(void) > > void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) > { > - unsigned long mask = cpumask_bits(cpumask)[0]; > + cpumask_t online_mask; > + unsigned int mask = 0; > > ASSERT(sgi < 16); /* There are only 16 SGIs */ > > - mask &= cpumask_bits(&cpu_online_map)[0]; > - > - ASSERT(mask < 0x100); /* The target bitmap only supports 8 CPUs */ > + cpumask_and(&online_mask, cpumask, &cpu_online_map); > + mask = gic_cpu_mask(&online_mask); > > dsb(); > >
On Fri, 2013-09-20 at 13:44 +0100, Julien Grall wrote: > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > > index b969d23..4061691 100644 > > --- a/xen/arch/arm/gic.c > > +++ b/xen/arch/arm/gic.c > > @@ -57,6 +57,29 @@ static DEFINE_PER_CPU(uint64_t, lr_mask); > > > > static unsigned nr_lrs; > > > > +/* The GIC mapping of CPU interfaces does not necessarily match the > > + * logical CPU numbering. Let's use mapping as returned by the GIC > > + * itself > > + */ > > +static DEFINE_PER_CPU(u8, gic_cpu_id); > > + > > Following the discussion on another thread > (http://lists.xen.org/archives/html/xen-devel/2013-09/msg02072.html), we > need to initialize each CPU ID to 0xff by default, to be able to bring > up secondary CPUs with an SGI (send_SGI_mask). We may as well use the send_SGI_allbutself mechanism, which doesn't rely on knowing and target ids. > But as I understand, per_cpu area for a give area is initialized in > cpu_up and there is no way to give a default value. So user could call > send_SGI_mask to early function and use an invalid CPU area. If we need to go down this path then I would write something like cpu_online(cpu) ? this_cpu(thing, cpu) : 0xff but I don't think we need to because we can easily avoid the problem. > IMHO, this solution is too fragile and I would like to use the same > solution as my first version (ie with an array of 8 cells). I think cpu bring up should use allbutself and the other callers can reasonable be restricted to only operate on cpus which are up and therefore have a valid mask already allocated and initialised. Ian.
On 09/20/2013 02:36 PM, Ian Campbell wrote: > On Fri, 2013-09-20 at 13:44 +0100, Julien Grall wrote: >>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >>> index b969d23..4061691 100644 >>> --- a/xen/arch/arm/gic.c >>> +++ b/xen/arch/arm/gic.c >>> @@ -57,6 +57,29 @@ static DEFINE_PER_CPU(uint64_t, lr_mask); >>> >>> static unsigned nr_lrs; >>> >>> +/* The GIC mapping of CPU interfaces does not necessarily match the >>> + * logical CPU numbering. Let's use mapping as returned by the GIC >>> + * itself >>> + */ >>> +static DEFINE_PER_CPU(u8, gic_cpu_id); >>> + >> >> Following the discussion on another thread >> (http://lists.xen.org/archives/html/xen-devel/2013-09/msg02072.html), we >> need to initialize each CPU ID to 0xff by default, to be able to bring >> up secondary CPUs with an SGI (send_SGI_mask). > > We may as well use the send_SGI_allbutself mechanism, which doesn't rely > on knowing and target ids. >> But as I understand, per_cpu area for a give area is initialized in >> cpu_up and there is no way to give a default value. So user could call >> send_SGI_mask to early function and use an invalid CPU area. > > If we need to go down this path then I would write something like > cpu_online(cpu) ? this_cpu(thing, cpu) : 0xff > but I don't think we need to because we can easily avoid the problem. > >> IMHO, this solution is too fragile and I would like to use the same >> solution as my first version (ie with an array of 8 cells). > > I think cpu bring up should use allbutself and the other callers can > reasonable be restricted to only operate on cpus which are up and > therefore have a valid mask already allocated and initialised. Ok, so I will, at least, add a comment in the gic code to explain this restriction.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index b969d23..4061691 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -57,6 +57,29 @@ static DEFINE_PER_CPU(uint64_t, lr_mask); static unsigned nr_lrs; +/* The GIC mapping of CPU interfaces does not necessarily match the + * logical CPU numbering. Let's use mapping as returned by the GIC + * itself + */ +static DEFINE_PER_CPU(u8, gic_cpu_id); + +/* Maximum cpu interface per GIC */ +#define NR_GIC_CPU_IF 8 + +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) +{ + unsigned int cpu; + unsigned int mask = 0; + + for_each_cpu(cpu, cpumask) + { + ASSERT(cpu < NR_GIC_CPU_IF); + mask |= per_cpu(gic_cpu_id, cpu); + } + + return mask; +} + unsigned int gic_number_lines(void) { return gic.lines; @@ -189,9 +212,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, { volatile unsigned char *bytereg; uint32_t cfg, edgebit; - unsigned int mask = cpumask_bits(cpu_mask)[0]; - - ASSERT(!(mask & ~0xff)); /* Target bitmap only support 8 CPUS */ + unsigned int mask = gic_cpu_mask(cpu_mask); /* Set edge / level */ cfg = GICD[GICD_ICFGR + irq / 16]; @@ -300,6 +321,8 @@ static void __cpuinit gic_cpu_init(void) { int i; + this_cpu(gic_cpu_id) = GICD[GICD_ITARGETSR] & 0xff; + /* The first 32 interrupts (PPI and SGI) are banked per-cpu, so * even though they are controlled with GICD registers, they must * be set up here with the other per-cpu state. */ @@ -431,13 +454,13 @@ void __init gic_init(void) void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) { - unsigned long mask = cpumask_bits(cpumask)[0]; + cpumask_t online_mask; + unsigned int mask = 0; ASSERT(sgi < 16); /* There are only 16 SGIs */ - mask &= cpumask_bits(&cpu_online_map)[0]; - - ASSERT(mask < 0x100); /* The target bitmap only supports 8 CPUs */ + cpumask_and(&online_mask, cpumask, &cpu_online_map); + mask = gic_cpu_mask(&online_mask); dsb();
The GIC mapping of CPU interfaces does not necessarily match the logical CPU numbering. When Xen wants to send an SGI to specific CPU, it needs to use the GIC CPU ID. It can be retrieved from ITARGETSR0, in fact when this field is read, the GIC will return a value that corresponds only to the processor reading the register. So Xen can use the PPI 0 to initialize the mapping. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v3: - Correctly create the mask in gic_cpu_mask Changes in v2: - Use per-cpu variable instead of an array - Add comment for NR_GIC_CPU_IF --- xen/arch/arm/gic.c | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-)