Re: [Xen-devel] [PATCH 3/9] arm: gic: implement IPIs using SGI mechanism

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-03-19 at 17:28 +0000, Stefano Stabellini wrote:
> On Wed, 6 Mar 2013, Ian Campbell wrote:
> > From: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > ---
> >  xen/arch/arm/arm32/mode_switch.S |    2 +-
> >  xen/arch/arm/gic.c               |   88 
> > +++++++++++++++++++++++++++++++++++---
> >  xen/arch/arm/smp.c               |   14 +++---
> >  xen/include/asm-arm/gic.h        |   22 +++++++++-
> >  4 files changed, 108 insertions(+), 18 deletions(-)
> > 
> > diff --git a/xen/arch/arm/arm32/mode_switch.S 
> > b/xen/arch/arm/arm32/mode_switch.S
> > index bc2be74..d6741d0 100644
> > --- a/xen/arch/arm/arm32/mode_switch.S
> > +++ b/xen/arch/arm/arm32/mode_switch.S
> > @@ -43,7 +43,7 @@ kick_cpus:
> >          mov   r2, #0x1
> >          str   r2, [r0, #(GICD_CTLR * 4)]      /* enable distributor */
> >          mov   r2, #0xfe0000
> > -        str   r2, [r0, #(GICD_SGIR * 4)]      /* send IPI to everybody */
> > +        str   r2, [r0, #(GICD_SGIR * 4)]      /* send IPI to everybody, 
> > SGI0 = Event check */
> >          dsb
> >          str   r2, [r0, #(GICD_CTLR * 4)]      /* disable distributor */
> >          mov   pc, lr
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index bbb8e04..6592562 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -354,10 +354,55 @@ void __init gic_init(void)
> >      spin_unlock(&gic.lock);
> >  }
> >  
> > +void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
> > +{
> > +    unsigned long mask = cpumask_bits(cpumask)[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 */
> > +
> > +    dsb();
> > +
> > +    GICD[GICD_SGIR] = GICD_SGI_TARGET_LIST
> > +        | (mask<<GICD_SGI_TARGET_SHIFT)
> > +        | GICD_SGI_GROUP1
> 
> Are you sure that this is correct?
> 
> The manual says: "This field is writable only by a Secure access. Any
> Non-secure write to the GICD_SGIR generates an SGI only if the specified
> SGI is programmed as Group 1, regardless of the value of bit[15] of the
> write."

I think the "regardless of the value..." bit at the end makes it OK (but
pointless) to include this bit, but... 

> I think that we should leave bit 15 alone.

... I think this is a good idea.

> > diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
> > index 12260f4..a902d84 100644
> > --- a/xen/arch/arm/smp.c
> > +++ b/xen/arch/arm/smp.c
> > @@ -3,10 +3,11 @@
> >  #include <asm/smp.h>
> >  #include <asm/cpregs.h>
> >  #include <asm/page.h>
> > +#include <asm/gic.h>
> >  
> >  void flush_tlb_mask(const cpumask_t *mask)
> >  {
> > -    /* XXX IPI other processors */
> > +    /* No need to IPI other processors on ARM, the processor takes care of 
> > it. */
> >      flush_xen_data_tlb();
> 
> From the ARMv7 manual, chapter B3.10.5:
> 
> "For an ARMv7 implementation that does not include the Multiprocessing
> Extensions, the architecture defines that a TLB maintenance operation
> applies only to any TLBs that are used in translating memory accesses
> made by the processor performing the maintenance operation.
> 
> The ARMv7 Multiprocessing Extensions are an OPTIONAL set of extensions
> that improve the implementation of a multiprocessor system. These
> extensions provide additional TLB maintenance operations that apply to
> the TLBs of processors in the same Inner Shareable domain."

NB that we only do SMP on systems with MP extensions. I suppose systems
with multiple processors but not MP extensions are completely
non-coherent ones. In this case the "other" processor looks more like
device not another processor. e..g like an A class core with an M class
core on the side or a device which happens to have a general purpose
core in it.

> From this paragraph I understand that we wouldn't need to do an IPI if
> flush_xen_data_tlb was implemented using TLBIALLHIS. But actually
> flush_xen_data_tlb uses TLBIALLH, that it seems not to propagate across
> an Inner Shareable domain.
> Did I misunderstand something?

TLBIALLH flushes further than TLBIALLHIS AIUI, so *H is always
sufficient if *HIS is sufficient.

Xen currently maps RAM (and does MMU walks etc) as outer-shareable but
actually we should switch to inner-shareable since it will perform
better (by not flushing so aggressively). FWIW Linux uses
inner-shareable so I doubt we will see many platforms that we care about
where we need to map stuff inner-shareable, even though the meaning of
inner- and outer- is somewhat implementation defined.

Ian.


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