| Message ID | 1395064352-2128-3-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Ian, On 03/17/2014 01:52 PM, Ian Campbell wrote: > This ensures that the writeq to the release address has occurred. > > In reality there is a dsb() in the iounmap() (in the eventual write_pte()) but > make it explicit. There other place (see xen/arch/arm/platforms/exynos5.c) where we rely on the dsb in write_pte. I think we should move the dsb in iounmap(). > > The ARMv8 ARM recommends that sev() is usually accompanied by a dsb(), the > only other uses are in the v7 spinlock code which includes a dsb() already. Would it be better to add dsb in the sev() macro? Regards,
On Mon, 2014-03-17 at 14:18 +0000, Julien Grall wrote: > Hi Ian, > > On 03/17/2014 01:52 PM, Ian Campbell wrote: > > This ensures that the writeq to the release address has occurred. > > > > In reality there is a dsb() in the iounmap() (in the eventual write_pte()) but > > make it explicit. > > There other place (see xen/arch/arm/platforms/exynos5.c) where we rely > on the dsb in write_pte. I think we should move the dsb in iounmap(). Now you put it like that it does seem logical that any writes would be issued prior to/as part of the iounmap. In that context I think it would in fact be fine for iounmap to rely on the dsb in write_pte since both a "change the mappings" functions. IOW maybe we don't need to do anything after all. > > The ARMv8 ARM recommends that sev() is usually accompanied by a dsb(), the > > only other uses are in the v7 spinlock code which includes a dsb() already. > > Would it be better to add dsb in the sev() macro? ARM32 has a dsb_sev macro, maybe we should do the same for ARM64. Ian
On 03/17/2014 02:39 PM, Ian Campbell wrote: >>> The ARMv8 ARM recommends that sev() is usually accompanied by a dsb(), the >>> only other uses are in the v7 spinlock code which includes a dsb() already. >> >> Would it be better to add dsb in the sev() macro? > > ARM32 has a dsb_sev macro, maybe we should do the same for ARM64. Do we really need to rename the macro? From what I understand, most of the time sev => dsb before. I think we can hide it in sev() macro. Regards,
On Mon, 2014-03-17 at 14:43 +0000, Julien Grall wrote: > On 03/17/2014 02:39 PM, Ian Campbell wrote: > >>> The ARMv8 ARM recommends that sev() is usually accompanied by a dsb(), the > >>> only other uses are in the v7 spinlock code which includes a dsb() already. > >> > >> Would it be better to add dsb in the sev() macro? > > > > ARM32 has a dsb_sev macro, maybe we should do the same for ARM64. > > Do we really need to rename the macro? I think so. > From what I understand, most of > the time sev => dsb before. I think we can hide it in sev() macro. These macros are thin shims around the underlying instructions, having them do other stuff as well would be potentially misleading. The current macro has one callsight, I think we can live with the churn... Ian.
On Mon, 2014-03-17 at 13:52 +0000, Ian Campbell wrote: > This ensures that the writeq to the release address has occurred. > > In reality there is a dsb() in the iounmap() (in the eventual write_pte()) but > make it explicit. > > The ARMv8 ARM recommends that sev() is usually accompanied by a dsb(), the > only other uses are in the v7 spinlock code which includes a dsb() already. Thinking about this some more the dsb() in the ioumap is logical and it is reasonable to rely on it. This also satisfies the need for a dsb before the sev. So I intend to just drop this patch for the next iteration. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > v2: typos in the commit message > --- > xen/arch/arm/arm64/smpboot.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c > index 9146476..8e2ecf2 100644 > --- a/xen/arch/arm/arm64/smpboot.c > +++ b/xen/arch/arm/arm64/smpboot.c > @@ -36,6 +36,7 @@ static int __init smp_spin_table_cpu_up(int cpu) > > iounmap(release); > > + dsb(); /* Ensure that the write to cpu_release_addr has occurred. */ > sev(); > > return cpu_up_send_sgi(cpu);
diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c index 9146476..8e2ecf2 100644 --- a/xen/arch/arm/arm64/smpboot.c +++ b/xen/arch/arm/arm64/smpboot.c @@ -36,6 +36,7 @@ static int __init smp_spin_table_cpu_up(int cpu) iounmap(release); + dsb(); /* Ensure that the write to cpu_release_addr has occurred. */ sev(); return cpu_up_send_sgi(cpu);
This ensures that the writeq to the release address has occurred. In reality there is a dsb() in the iounmap() (in the eventual write_pte()) but make it explicit. The ARMv8 ARM recommends that sev() is usually accompanied by a dsb(), the only other uses are in the v7 spinlock code which includes a dsb() already. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- v2: typos in the commit message --- xen/arch/arm/arm64/smpboot.c | 1 + 1 file changed, 1 insertion(+)