Re: [Xen-devel] [PATCH 2/4] xen/x86: Drop erronious barriers

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 06/12/2016 20:32, Stefano Stabellini wrote:
> On Tue, 6 Dec 2016, Stefano Stabellini wrote:
> > On Tue, 6 Dec 2016, Andrew Cooper wrote:
> > > On 05/12/2016 19:17, Stefano Stabellini wrote:
> > > > On Mon, 5 Dec 2016, Andrew Cooper wrote:
> > > > > None of these barriers serve any purpose, as they are not synchronising with
> > > > > any anything on remote CPUs.
> > > > >
> > > > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > > > ---
> > > > > CC: Jan Beulich <JBeulich@xxxxxxxx>
> > > > > CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > > CC: Julien Grall <julien.grall@xxxxxxx>
> > > > >
> > > > > Restricting to just the $ARCH maintainers, as this is a project-wide sweep.
> > > > >
> > > > > Julien/Stefano: I think the ARM smpboot inhereted the erronious barrier usage
> > > > > from x86, but I don't know whether further development has gained a dependence
> > > > > on them.
> > > > We turned them into smp_wmb already (kudos to IanC).
> > > Right, but the entire point I am trying to argue is that they are not
> > > needed in the first place.
> Just to be clear, on ARM the barriers are unneeded only if it is
> unimportant that "init stuff" (which correspond to all the
> initialization done in start_secondary up to smp_wmb) below is completed
> before "write cpu_online_map". But it looks like we do want to complete
> mmu, irq, timer initializations and set the current vcpu before marking
> the cpu as online, right?
*mb are memory barriers. This would not prevent write to system 
registers and co-processor registers happening before "write 
cpu_online_map". Only an dsb(sy); isb() would ensure this.
However, I don't think we really care about the state of the hardware 
for a specific CPU. This should never be accessed by another one.
> > This is the current code:
> >
> >     CPU 1                                  CPU 0
> >     -----                                  -----
> >
> >     init stuff                             read cpu_online_map
> >
> >     write barrier
> >
> >     write cpu_online_map                   do more initialization
> >
> >     write barrier
> >
> >     init more stuff
> >
> >
> > I agree that it's wrong, because the second write barrier in
> > start_secondary is useless and in addition we are missing a read barrier
> > in __cpu_up, corresponding to the first write barrier in
> > start_secondary.
> >
> > I think it should look like:
> >
> >
> >     CPU 1                                  CPU 0
> >     -----                                  -----
> >
> >     init stuff                             read cpu_online_map
> >
> >     write barrier                          read barrier
> >
> >     write cpu_online_map                   do more initialization
> >
> >     init more stuff
> >
> >
> > The patch is as follow.
> >
> > Julien, what do you think?
> >
> > Also, do we need to change the remaming smp_wmb() in start_secondary to
> > wmb() to ensure execution ordering as well as memory access ordering?
I don't think so. If synchronization of hardware access was necessary it 
would have been taken care by the driver itself.
What we should care here is if there any xen internal state that are 
required between CPU0 and CPU1.
In this specific case, I think we should have the smp_wmb() barrier to 
ensure all write happening whilst calling local notifiers will be 
visible before the CPU mark itself as online. So IHMO, the patch looks 
good to me (see a small comment below).
> > Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >
> > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > index 90ad1d0..c841a15 100644
> > --- a/xen/arch/arm/smpboot.c
> > +++ b/xen/arch/arm/smpboot.c
> > @@ -311,7 +311,6 @@ void start_secondary(unsigned long boot_phys_offset,
> >
> >      /* Now report this CPU is up */
> >      cpumask_set_cpu(cpuid, &cpu_online_map);
> > -    smp_wmb();
> >
> >      local_irq_enable();
> >      local_abort_enable();
> > @@ -408,6 +407,7 @@ int __cpu_up(unsigned int cpu)
> >          cpu_relax();
> >          process_pending_softirqs();
> >      }
> > +    smp_rmb();
It would be good to start annotating the pair of barrier with "This 
barrier corresponds with the one in...". It would avoid "wild" barrier 
in the code :).
> >      /*
> >       * Nuke start of day info before checking one last time if the CPU
Regards,

--
Julien Grall

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