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

[Julien Grall <julien@xxxxxxx>]

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.
BTW, writel_relaxed() should be sufficient here as there is no ordering 
necessary with any previous write.
Cheers,

[1] 
https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3
--
Julien Grall