Re: [PATCH 06/10] xen: arm: Use SMP barriers when that is all which is required.

[Tim Deegan <tim@xen.org>]

At 16:24 +0100 on 01 Jul (1372695881), Ian Campbell wrote:
> On Mon, 2013-07-01 at 16:19 +0100, Stefano Stabellini wrote:
> > On Fri, 28 Jun 2013, Ian Campbell wrote:
> > > As explained in the previous commit SMP barriers can be used when all we care
> > > about is synchronising against other processors.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > >  xen/arch/arm/mm.c      |    2 +-
> > >  xen/arch/arm/smpboot.c |    4 ++--
> > >  2 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index c5213f2..3f049cb 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -776,7 +776,7 @@ void share_xen_page_with_guest(struct page_info *page,
> > >      page->u.inuse.type_info |= PGT_validated | 1;
> > >  
> > >      page_set_owner(page, d);
> > > -    wmb(); /* install valid domain ptr before updating refcnt. */
> > > +    smp_wmb(); /* install valid domain ptr before updating refcnt. */
> > >      ASSERT((page->count_info & ~PGC_xen_heap) == 0);
> > >  
> > >      /* Only add to the allocation list if the domain isn't dying. */
> > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > > index 8011987..727e09f 100644
> > > --- a/xen/arch/arm/smpboot.c
> > > +++ b/xen/arch/arm/smpboot.c
> > > @@ -170,11 +170,11 @@ void __cpuinit start_secondary(unsigned long boot_phys_offset,
> > >  
> > >      /* Run local notifiers */
> > >      notify_cpu_starting(cpuid);
> > > -    wmb();
> > > +    smp_wmb();
> > >  
> > >      /* Now report this CPU is up */
> > >      cpumask_set_cpu(cpuid, &cpu_online_map);
> > > -    wmb();
> > > +    smp_wmb();
> > >  
> > >      local_irq_enable();
> > 
> > Did you missed few mb() in smpboot.c?
> 
> The ones in __cpu_disable and __cpu_die?
> 
> I think I just wasn't 100% sure they might not be touching hardware
> (i.e. some platform register to shutdown a CPU) and since they weren't
> performance critical I punted on them.
> 
> Looking it again the first half of that logic seems to be bogus (that
> code goes nowhere near any peripheral).

Yes, I think they can both be smp_mb().  With or without that change,

Acked-by: Tim Deegan <tim@xen.org>