| Message ID | 1377869433-15385-5-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 2013-08-30 at 14:30 +0100, 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> > --- > xen/arch/arm/gic.c | 35 ++++++++++++++++++++++++++++------- > 1 file changed, 28 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index cadc258..4f3a8a5 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -57,6 +57,27 @@ 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 > + */ > +#define NR_GIC_CPU_IF 8 > +static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly = {0xff}; Isn't this mapping from logical cpus ids to gic cpus ids? IOW the size of this array is the wrong way around? Is what you want here is a per-cpu variable? > + > +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 |= gic_cpu_map[cpu]; Further to above, can't cpu here be e.g. 0x100 for an a7 on a big.ITTLE system? > + } > + > + return mask; > +} > + > unsigned int gic_number_lines(void) > { > return gic.lines; > @@ -206,9 +227,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]; > @@ -317,6 +336,8 @@ static void __cpuinit gic_cpu_init(void) > { > int i; > > + gic_cpu_map[smp_processor_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. */ > @@ -443,13 +464,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 09/09/2013 02:28 PM, Ian Campbell wrote: > On Fri, 2013-08-30 at 14:30 +0100, 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> >> --- >> xen/arch/arm/gic.c | 35 ++++++++++++++++++++++++++++------- >> 1 file changed, 28 insertions(+), 7 deletions(-) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index cadc258..4f3a8a5 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -57,6 +57,27 @@ 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 >> + */ >> +#define NR_GIC_CPU_IF 8 >> +static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly = {0xff}; > > Isn't this mapping from logical cpus ids to gic cpus ids? IOW the size > of this array is the wrong way around? This array maps a logical cpu id (the index) to a gic cpus id. The size if correct, because Xen will allocate logical cpu id from 0 to 7. > Is what you want here is a per-cpu variable? Does per-cpu variable allow any cpu to retrieve data of another cpu? I didn't find a such function. >> + >> +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 |= gic_cpu_map[cpu]; > > Further to above, can't cpu here be e.g. 0x100 for an a7 on a big.ITTLE > system? 0x100 is the hardware CPU id. Xen will allocate a logical cpu id from 0. >> + } >> + >> + return mask; >> +} >> + >> unsigned int gic_number_lines(void) >> { >> return gic.lines; >> @@ -206,9 +227,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]; >> @@ -317,6 +336,8 @@ static void __cpuinit gic_cpu_init(void) >> { >> int i; >> >> + gic_cpu_map[smp_processor_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. */ >> @@ -443,13 +464,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 Tue, 2013-09-10 at 16:42 +0100, Julien Grall wrote: > On 09/09/2013 02:28 PM, Ian Campbell wrote: > > On Fri, 2013-08-30 at 14:30 +0100, 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> > >> --- > >> xen/arch/arm/gic.c | 35 ++++++++++++++++++++++++++++------- > >> 1 file changed, 28 insertions(+), 7 deletions(-) > >> > >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > >> index cadc258..4f3a8a5 100644 > >> --- a/xen/arch/arm/gic.c > >> +++ b/xen/arch/arm/gic.c > >> @@ -57,6 +57,27 @@ 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 > >> + */ > >> +#define NR_GIC_CPU_IF 8 > >> +static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly = {0xff}; > > > > Isn't this mapping from logical cpus ids to gic cpus ids? IOW the size > > of this array is the wrong way around? > > This array maps a logical cpu id (the index) to a gic cpus id. The size > if correct, because Xen will allocate logical cpu id from 0 to 7. Oh right, yes. I think NR_CPUS (which should be 8 given the other limitations) is the right size for this array. > > > Is what you want here is a per-cpu variable? > > Does per-cpu variable allow any cpu to retrieve data of another cpu? I > didn't find a such function. per_cpu(variable, cpu) will do it. > > >> + > >> +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 |= gic_cpu_map[cpu]; > > > > Further to above, can't cpu here be e.g. 0x100 for an a7 on a big.ITTLE > > system? > > 0x100 is the hardware CPU id. Xen will allocate a logical cpu id from 0. Right.
On 09/10/2013 04:52 PM, Ian Campbell wrote: > On Tue, 2013-09-10 at 16:42 +0100, Julien Grall wrote: >> On 09/09/2013 02:28 PM, Ian Campbell wrote: >>> On Fri, 2013-08-30 at 14:30 +0100, 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> >>>> --- >>>> xen/arch/arm/gic.c | 35 ++++++++++++++++++++++++++++------- >>>> 1 file changed, 28 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >>>> index cadc258..4f3a8a5 100644 >>>> --- a/xen/arch/arm/gic.c >>>> +++ b/xen/arch/arm/gic.c >>>> @@ -57,6 +57,27 @@ 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 >>>> + */ >>>> +#define NR_GIC_CPU_IF 8 >>>> +static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly = {0xff}; >>> >>> Isn't this mapping from logical cpus ids to gic cpus ids? IOW the size >>> of this array is the wrong way around? >> >> This array maps a logical cpu id (the index) to a gic cpus id. The size >> if correct, because Xen will allocate logical cpu id from 0 to 7. > > Oh right, yes. > > I think NR_CPUS (which should be 8 given the other limitations) is the > right size for this array. > >> >>> Is what you want here is a per-cpu variable? >> >> Does per-cpu variable allow any cpu to retrieve data of another cpu? I >> didn't find a such function. > > per_cpu(variable, cpu) will do it. Is it fast ? I think this solution is slower, because the mapping is used often used, mainly when an SGI is sent to another cpu. >> >>>> + >>>> +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 |= gic_cpu_map[cpu]; >>> >>> Further to above, can't cpu here be e.g. 0x100 for an a7 on a big.ITTLE >>> system? >> >> 0x100 is the hardware CPU id. Xen will allocate a logical cpu id from 0. > > Right. > >
On Tue, 2013-09-10 at 17:45 +0100, Julien Grall wrote: > On 09/10/2013 04:52 PM, Ian Campbell wrote: > > On Tue, 2013-09-10 at 16:42 +0100, Julien Grall wrote: > >> On 09/09/2013 02:28 PM, Ian Campbell wrote: > >>> On Fri, 2013-08-30 at 14:30 +0100, 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> > >>>> --- > >>>> xen/arch/arm/gic.c | 35 ++++++++++++++++++++++++++++------- > >>>> 1 file changed, 28 insertions(+), 7 deletions(-) > >>>> > >>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > >>>> index cadc258..4f3a8a5 100644 > >>>> --- a/xen/arch/arm/gic.c > >>>> +++ b/xen/arch/arm/gic.c > >>>> @@ -57,6 +57,27 @@ 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 > >>>> + */ > >>>> +#define NR_GIC_CPU_IF 8 > >>>> +static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly = {0xff}; > >>> > >>> Isn't this mapping from logical cpus ids to gic cpus ids? IOW the size > >>> of this array is the wrong way around? > >> > >> This array maps a logical cpu id (the index) to a gic cpus id. The size > >> if correct, because Xen will allocate logical cpu id from 0 to 7. > > > > Oh right, yes. > > > > I think NR_CPUS (which should be 8 given the other limitations) is the > > right size for this array. > > > >> > >>> Is what you want here is a per-cpu variable? > >> > >> Does per-cpu variable allow any cpu to retrieve data of another cpu? I > >> didn't find a such function. > > > > per_cpu(variable, cpu) will do it. > > Is it fast ? It looks up the cpus offset in an array and then adds it to the address of variable before loading the result. > I think this solution is slower, because the mapping is > used often used, mainly when an SGI is sent to another cpu. I doubt it is measurable. Ian > > >> > >>>> + > >>>> +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 |= gic_cpu_map[cpu]; > >>> > >>> Further to above, can't cpu here be e.g. 0x100 for an a7 on a big.ITTLE > >>> system? > >> > >> 0x100 is the hardware CPU id. Xen will allocate a logical cpu id from 0. > > > > Right. > > > > > >
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index cadc258..4f3a8a5 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -57,6 +57,27 @@ 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 + */ +#define NR_GIC_CPU_IF 8 +static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly = {0xff}; + +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 |= gic_cpu_map[cpu]; + } + + return mask; +} + unsigned int gic_number_lines(void) { return gic.lines; @@ -206,9 +227,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]; @@ -317,6 +336,8 @@ static void __cpuinit gic_cpu_init(void) { int i; + gic_cpu_map[smp_processor_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. */ @@ -443,13 +464,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> --- xen/arch/arm/gic.c | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-)