Re: [Xen-devel] [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Tim Deegan <tim@xxxxxxx>]

At 16:53 +0100 on 25 Sep (1380128037), Ian Campbell wrote:
> On Wed, 2013-09-25 at 16:48 +0100, Ian Campbell wrote:
> > On Wed, 2013-09-25 at 16:42 +0100, Julien Grall wrote:
> > > On 09/25/2013 04:35 PM, Ian Campbell wrote:
> > > > On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
> > > >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> > > >> +{
> > > >> +    unsigned int cpu;
> > > >> +    unsigned int mask = 0;
> > > >> +    cpumask_t possible_mask;
> > > >> +
> > > >> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> > > >> +    for_each_cpu(cpu, &possible_mask)
> > > >> +    {
> > > >> +        ASSERT(cpu < NR_GIC_CPU_IF);
> > > >> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
> > > > 
> > > > This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I
> > > > think we can live without this check. After all the CPU is in possible
> > > > map.
> > > 
> > > Being in cpu possible map doesn't mean that the per cpu region is
> > > initialized for the given cpu.
> > 
> > So you expect this ASSERT to trigger in practice? That's not good...
> > 
> > > I have noticed the INVALID_PERCPU_AREA is the same both Intel and ARM
> > > platform. Can we move this define in percpu.h?
> > 
> > I'm wondering if it should be the same, I think it was chosen on x86 to
> > result in a non-canonical address (i.e. a guaranteed fault) and ARM
> > copied it.
> > 
> > __per_cpu_start on ARM is in the first 2MB so -__per_cpu_start is 2MB
> > from the top of the 64 bit address space, which is also invalid. I
> > think, Tim, did you consider this or just copy the x86 value?
> 
> Actually, since the offset of the per_cpu var gets added the result is
> not invalid at all, it'll be the offset within the percpu of the area
> (i.e. a small number). Luckily we deliberately don't map anything at
> 0..2MB and Xen is smaller than 2MB and per_cpu stuff fits inside Xen. So
> it works out OK (where OK means deliberately traps). Phew.

Yes, it maps to NULL+offset, where offset is comfortably < 1 page in the
current build; AFAICS this was deliberate on x86 and certainly
deliberate when I copied it.  The stronger property that offset must
always < 2MB didn't occur to me, but is reassuring. :)

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel