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
