| Message ID | 20230619170115.81398-5-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Add some missing ISBs after updating the PTEs | expand |
Hi Julien, > -----Original Message----- > Subject: [PATCH 4/7] xen/arm: page: Consolidate write_pte() and clarify the > documentation > > From: Julien Grall <jgrall@amazon.com> > > The implementation of write_pte() is pretty much the same on arm32 > and arm64. So it would be good to consolidate it as this would help > to clarify the requirements when using the helper. > > Take the opportunity to switch from assembly to call macros. Note there > is a difference on arm32 because the option was not specified. But this > meant 'sy' (system wide). > > Futhermore, the requirements for the ISB is incomplete. Per the Arm Arm, > (Armv7 DDI406C.d A3.8.3 and Armv8 DDI 0487J.a B2.3.12), DSB will only > affect explicit accesses. So an ISB is necessary after DSB to ensure > the completion. Having an ISB after each update to the page-tables is > probably too much, so let the caller add the instruction when it is > convenient. > > Lastly, the barrier in write_pte() may be too restrictive but I haven't > yet find the proper section(s) in the Arm Arm. > > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Henry Wang <Henry.Wang@arm.com> I've also tested this patch on top of today's staging by our internal CI, and this patch looks good, so: Tested-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
Hi Julien, > On 19 Jun 2023, at 19:01, Julien Grall <julien@xen.org> wrote: > > From: Julien Grall <jgrall@amazon.com> > > The implementation of write_pte() is pretty much the same on arm32 > and arm64. So it would be good to consolidate it as this would help > to clarify the requirements when using the helper. > > Take the opportunity to switch from assembly to call macros. Note there > is a difference on arm32 because the option was not specified. But this > meant 'sy' (system wide). > > Futhermore, the requirements for the ISB is incomplete. Per the Arm Arm, > (Armv7 DDI406C.d A3.8.3 and Armv8 DDI 0487J.a B2.3.12), DSB will only > affect explicit accesses. So an ISB is necessary after DSB to ensure > the completion. Having an ISB after each update to the page-tables is > probably too much, so let the caller add the instruction when it is > convenient. > > Lastly, the barrier in write_pte() may be too restrictive but I haven't > yet find the proper section(s) in the Arm Arm. I do not think we will find any proper section for that in the Arm Arm as it depends on use cases. In ours we might have the page table shared among cores so I think system is the way to go. > > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > ---- > > I am a bit split on whether we should add an ISB in write_pte(). It > would make easier for the developper, but would likely force a pipeline > flush too often. > > It might also be possible to drop the ISB (and even DSB) when updating > stage-2 PTE (Linux already does it, see 120798d2e7d1). But I am not sure > this is worth it in Xen. > --- > xen/arch/arm/include/asm/arm32/page.h | 16 ---------------- > xen/arch/arm/include/asm/arm64/page.h | 11 ----------- > xen/arch/arm/include/asm/page.h | 17 +++++++++++++++++ > 3 files changed, 17 insertions(+), 27 deletions(-) > > diff --git a/xen/arch/arm/include/asm/arm32/page.h b/xen/arch/arm/include/asm/arm32/page.h > index 715a9e4fef48..6d1ff0636ce3 100644 > --- a/xen/arch/arm/include/asm/arm32/page.h > +++ b/xen/arch/arm/include/asm/arm32/page.h > @@ -3,22 +3,6 @@ > > #ifndef __ASSEMBLY__ > > -/* Write a pagetable entry. > - * > - * If the table entry is changing a text mapping, it is responsibility > - * of the caller to issue an ISB after write_pte. > - */ > -static inline void write_pte(lpae_t *p, lpae_t pte) > -{ > - asm volatile ( > - /* Ensure any writes have completed with the old mappings. */ > - "dsb;" > - /* Safely write the entry (STRD is atomic on CPUs that support LPAE) */ > - "strd %0, %H0, [%1];" > - "dsb;" > - : : "r" (pte.bits), "r" (p) : "memory"); > -} > - > /* Inline ASM to invalidate dcache on register R (may be an inline asm operand) */ > #define __invalidate_dcache_one(R) STORE_CP32(R, DCIMVAC) > > diff --git a/xen/arch/arm/include/asm/arm64/page.h b/xen/arch/arm/include/asm/arm64/page.h > index 0cba2663733b..4f58c0382adc 100644 > --- a/xen/arch/arm/include/asm/arm64/page.h > +++ b/xen/arch/arm/include/asm/arm64/page.h > @@ -5,17 +5,6 @@ > > #include <asm/alternative.h> > > -/* Write a pagetable entry */ > -static inline void write_pte(lpae_t *p, lpae_t pte) > -{ > - asm volatile ( > - /* Ensure any writes have completed with the old mappings. */ > - "dsb sy;" > - "str %0, [%1];" /* Write the entry */ > - "dsb sy;" > - : : "r" (pte.bits), "r" (p) : "memory"); > -} > - > /* Inline ASM to invalidate dcache on register R (may be an inline asm operand) */ > #define __invalidate_dcache_one(R) "dc ivac, %" #R ";" > > diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h > index e7cd62190c7f..ea96983ab976 100644 > --- a/xen/arch/arm/include/asm/page.h > +++ b/xen/arch/arm/include/asm/page.h > @@ -126,6 +126,7 @@ > #include <xen/errno.h> > #include <xen/types.h> > #include <xen/lib.h> > +#include <asm/atomic.h> > #include <asm/system.h> > > #if defined(CONFIG_ARM_32) > @@ -237,6 +238,22 @@ static inline int clean_and_invalidate_dcache_va_range > : : "r" (_p), "m" (*_p)); \ > } while (0) > > +/* > + * Write a pagetable entry. > + * > + * It is the responsibility of the caller to issue an ISB (if a new entry) > + * or a TLB flush (if modified or removed) after write_pte(). > + */ > +static inline void write_pte(lpae_t *p, lpae_t pte) > +{ > + /* Ensure any writes have completed with the old mappings. */ > + dsb(sy); > + /* Safely write the entry. This should always be an atomic write. */ > + write_atomic(p, pte); > + dsb(sy); > +} > + > + > /* Flush the dcache for an entire page. */ > void flush_page_to_ram(unsigned long mfn, bool sync_icache); > > -- > 2.40.1 >
Hi Bertrand, On 04/07/2023 16:06, Bertrand Marquis wrote: >> On 19 Jun 2023, at 19:01, Julien Grall <julien@xen.org> wrote: >> >> From: Julien Grall <jgrall@amazon.com> >> >> The implementation of write_pte() is pretty much the same on arm32 >> and arm64. So it would be good to consolidate it as this would help >> to clarify the requirements when using the helper. >> >> Take the opportunity to switch from assembly to call macros. Note there >> is a difference on arm32 because the option was not specified. But this >> meant 'sy' (system wide). >> >> Futhermore, the requirements for the ISB is incomplete. Per the Arm Arm, >> (Armv7 DDI406C.d A3.8.3 and Armv8 DDI 0487J.a B2.3.12), DSB will only >> affect explicit accesses. So an ISB is necessary after DSB to ensure >> the completion. Having an ISB after each update to the page-tables is >> probably too much, so let the caller add the instruction when it is >> convenient. >> >> Lastly, the barrier in write_pte() may be too restrictive but I haven't >> yet find the proper section(s) in the Arm Arm. > > I do not think we will find any proper section for that in the Arm Arm as it > depends on use cases. > In ours we might have the page table shared among cores so I think system > is the way to go. For SMP systems, like Linux, Xen is assuming that all the cores are in the same inner-shareable domain. So 'ish' would be sufficient based on just that. However, I haven't made the change because I am not sure about the interaction when the modified entry modified to device memory and we have an access after/before. > >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> > > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Thanks! Cheers,
diff --git a/xen/arch/arm/include/asm/arm32/page.h b/xen/arch/arm/include/asm/arm32/page.h index 715a9e4fef48..6d1ff0636ce3 100644 --- a/xen/arch/arm/include/asm/arm32/page.h +++ b/xen/arch/arm/include/asm/arm32/page.h @@ -3,22 +3,6 @@ #ifndef __ASSEMBLY__ -/* Write a pagetable entry. - * - * If the table entry is changing a text mapping, it is responsibility - * of the caller to issue an ISB after write_pte. - */ -static inline void write_pte(lpae_t *p, lpae_t pte) -{ - asm volatile ( - /* Ensure any writes have completed with the old mappings. */ - "dsb;" - /* Safely write the entry (STRD is atomic on CPUs that support LPAE) */ - "strd %0, %H0, [%1];" - "dsb;" - : : "r" (pte.bits), "r" (p) : "memory"); -} - /* Inline ASM to invalidate dcache on register R (may be an inline asm operand) */ #define __invalidate_dcache_one(R) STORE_CP32(R, DCIMVAC) diff --git a/xen/arch/arm/include/asm/arm64/page.h b/xen/arch/arm/include/asm/arm64/page.h index 0cba2663733b..4f58c0382adc 100644 --- a/xen/arch/arm/include/asm/arm64/page.h +++ b/xen/arch/arm/include/asm/arm64/page.h @@ -5,17 +5,6 @@ #include <asm/alternative.h> -/* Write a pagetable entry */ -static inline void write_pte(lpae_t *p, lpae_t pte) -{ - asm volatile ( - /* Ensure any writes have completed with the old mappings. */ - "dsb sy;" - "str %0, [%1];" /* Write the entry */ - "dsb sy;" - : : "r" (pte.bits), "r" (p) : "memory"); -} - /* Inline ASM to invalidate dcache on register R (may be an inline asm operand) */ #define __invalidate_dcache_one(R) "dc ivac, %" #R ";" diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h index e7cd62190c7f..ea96983ab976 100644 --- a/xen/arch/arm/include/asm/page.h +++ b/xen/arch/arm/include/asm/page.h @@ -126,6 +126,7 @@ #include <xen/errno.h> #include <xen/types.h> #include <xen/lib.h> +#include <asm/atomic.h> #include <asm/system.h> #if defined(CONFIG_ARM_32) @@ -237,6 +238,22 @@ static inline int clean_and_invalidate_dcache_va_range : : "r" (_p), "m" (*_p)); \ } while (0) +/* + * Write a pagetable entry. + * + * It is the responsibility of the caller to issue an ISB (if a new entry) + * or a TLB flush (if modified or removed) after write_pte(). + */ +static inline void write_pte(lpae_t *p, lpae_t pte) +{ + /* Ensure any writes have completed with the old mappings. */ + dsb(sy); + /* Safely write the entry. This should always be an atomic write. */ + write_atomic(p, pte); + dsb(sy); +} + + /* Flush the dcache for an entire page. */ void flush_page_to_ram(unsigned long mfn, bool sync_icache);