Re: [RFC PATCH] xen/arm: arm32: Enable smpboot on Arm32 based systems

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 3 May 2023, Julien Grall wrote:
> Hi Stefano,
> 
> On 03/05/2023 18:54, Stefano Stabellini wrote:
> > On Wed, 3 May 2023, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 03/05/2023 00:55, Stefano Stabellini wrote:
> > > > > +    {
> > > > > +        printk("CPU%d: No release addr\n", cpu);
> > > > > +        return -ENODEV;
> > > > > +    }
> > > > > +
> > > > > +    release = ioremap_nocache(cpu_release_addr[cpu], 4);
> > > > > +    if ( !release )
> > > > > +    {
> > > > > +        dprintk(XENLOG_ERR, "CPU%d: Unable to map release address\n",
> > > > > cpu);
> > > > > +        return -EFAULT;
> > > > > +    }
> > > > > +
> > > > > +    writel(__pa(init_secondary), release);
> > > > > +
> > > > > +    iounmap(release);
> > > > 
> > > > I think we need a wmb() ?
> > > 
> > > I am not sure why we would need a wmb() here.
> > 
> > The code does:
> > 
> > writel(__pa(init_secondary), release);
> > iounmap
> > sev();
> > 
> > I was thinking of trying to make sure the write is completed before
> > issuing a sev(). Technically it is possible for the CPU to reorder the
> > sev() before the write as there is no explicit dependency between the
> > two?
> 
> I would assume that iounmap() would contain an wmb(). But I guess this is not
> something we should rely on.
> 
> > 
> > > Instead, looking at the Linux
> > > version, I think we are missing a cache flush (so does on arm64) which
> > > would
> > > be necessary if the CPU waiting for the release doesn't have cache
> > > enabled.
> > 
> > I thought about it as well but here the patch is calling
> > ioremap_nocache(), so cache flushes should not be needed?
> 
> Hmmm... You are right, we are using ioremap_nocache(). I got confused because
> Linux is using ioremap_cache(). I am now wondering whether we are using the
> correct helper.
> 
> AFAIU, spin table is part of the reserved memory section in the Device-Tree.
> From section 5.3 in [1], the memory could be mapped cacheable by the other
> end. So for correctness, it seems to me that we would need to use
> ioremap_cache() + cache flush.

Good point


> BTW, writel_relaxed() should be sufficient here as there is no ordering
> necessary with any previous write.