| Message ID | 1380835081-12129-7-git-send-email-santosh.shilimkar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 3 Oct 2013, Santosh Shilimkar wrote: > From: Sricharan R <r.sricharan@ti.com> > > As per the arm ARMv7 manual, the sequence of TLB maintenance > operations after making changes to the translation table is > to clean the dcache first, then invalidate the TLB. With > the current sequence we see cache corruption when the > flush_cache_all is called after tlb_flush_all. > > STR rx, [Translation table entry] > ; write new entry to the translation table > Clean cache line [Translation table entry] > DSB > ; ensures visibility of the data cleaned from the D Cache > Invalidate TLB entry by MVA (and ASID if non-global) [page address] > Invalidate BTC > DSB > ; ensure completion of the Invalidate TLB operation > ISB > ; ensure table changes visible to instruction fetch > > The issue is seen only with LPAE + THUMB BUILT KERNEL + 64BIT patching, > which is little bit weird. > > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Nicolas Pitre <nicolas.pitre@linaro.org> > Cc: Russell King - ARM Linux <linux@arm.linux.org.uk> > > Signed-off-by: Sricharan R <r.sricharan@ti.com> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> Acked-by: Nicolas Pitre <nico@linaro.org> > --- > arch/arm/mm/mmu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c > index 47c7497..49cba8a 100644 > --- a/arch/arm/mm/mmu.c > +++ b/arch/arm/mm/mmu.c > @@ -1280,8 +1280,8 @@ static void __init devicemaps_init(const struct machine_desc *mdesc) > * any write-allocated cache lines in the vector page are written > * back. After this point, we can start to touch devices again. > */ > - local_flush_tlb_all(); > flush_cache_all(); > + local_flush_tlb_all(); > } > > static void __init kmap_init(void) > -- > 1.7.9.5 >
On Thu, Oct 03, 2013 at 05:18:00PM -0400, Santosh Shilimkar wrote: > From: Sricharan R <r.sricharan@ti.com> > > As per the arm ARMv7 manual, the sequence of TLB maintenance > operations after making changes to the translation table is > to clean the dcache first, then invalidate the TLB. With > the current sequence we see cache corruption when the > flush_cache_all is called after tlb_flush_all. This needs testing on ARMv4 CPUs which don't have a way to flush the cache except by reading memory - hence they need the new page table entries to be visible to the MMU before calling flush_cache_all().
On Fri, 4 Oct 2013, Russell King - ARM Linux wrote: > On Thu, Oct 03, 2013 at 05:18:00PM -0400, Santosh Shilimkar wrote: > > From: Sricharan R <r.sricharan@ti.com> > > > > As per the arm ARMv7 manual, the sequence of TLB maintenance > > operations after making changes to the translation table is > > to clean the dcache first, then invalidate the TLB. With > > the current sequence we see cache corruption when the > > flush_cache_all is called after tlb_flush_all. > > This needs testing on ARMv4 CPUs which don't have a way to flush the > cache except by reading memory - hence they need the new page table > entries to be visible to the MMU before calling flush_cache_all(). I suspect you might be one of the few individuals still having the ability to test new kernels on ARMv4 CPUs. Nicolas
On Friday 04 October 2013 09:14 AM, Nicolas Pitre wrote: > On Fri, 4 Oct 2013, Russell King - ARM Linux wrote: > >> On Thu, Oct 03, 2013 at 05:18:00PM -0400, Santosh Shilimkar wrote: >>> From: Sricharan R <r.sricharan@ti.com> >>> >>> As per the arm ARMv7 manual, the sequence of TLB maintenance >>> operations after making changes to the translation table is >>> to clean the dcache first, then invalidate the TLB. With >>> the current sequence we see cache corruption when the >>> flush_cache_all is called after tlb_flush_all. >> >> This needs testing on ARMv4 CPUs which don't have a way to flush the >> cache except by reading memory - hence they need the new page table >> entries to be visible to the MMU before calling flush_cache_all(). > > I suspect you might be one of the few individuals still having the > ability to test new kernels on ARMv4 CPUs. > At least I don't have any ARMv4 based system to validate it. Regards, Santosh
On Thu, Oct 03, 2013 at 10:18:00PM +0100, Santosh Shilimkar wrote: > From: Sricharan R <r.sricharan@ti.com> > > As per the arm ARMv7 manual, the sequence of TLB maintenance > operations after making changes to the translation table is > to clean the dcache first, then invalidate the TLB. With > the current sequence we see cache corruption when the > flush_cache_all is called after tlb_flush_all. > > STR rx, [Translation table entry] > ; write new entry to the translation table > Clean cache line [Translation table entry] > DSB > ; ensures visibility of the data cleaned from the D Cache > Invalidate TLB entry by MVA (and ASID if non-global) [page address] > Invalidate BTC > DSB > ; ensure completion of the Invalidate TLB operation > ISB > ; ensure table changes visible to instruction fetch > > The issue is seen only with LPAE + THUMB BUILT KERNEL + 64BIT patching, > which is little bit weird. NAK. I don't buy your reasoning. All current LPAE implementations also implement the multi-processing extensions, meaning that the cache flush isn't required to make the PTEs visible to the table walker. The dsb from the TLB_WB flag is sufficient, so I think you still have some debugging to do as this change is likely masking a problem elsewhere. On top of that, create_mapping does all the flushing you need (for the !SMP case) when the tables are initialised, so this code doesn't need changing. Will
On Friday 04 October 2013 11:52 AM, Will Deacon wrote: > On Thu, Oct 03, 2013 at 10:18:00PM +0100, Santosh Shilimkar wrote: >> From: Sricharan R <r.sricharan@ti.com> >> >> As per the arm ARMv7 manual, the sequence of TLB maintenance >> operations after making changes to the translation table is >> to clean the dcache first, then invalidate the TLB. With >> the current sequence we see cache corruption when the >> flush_cache_all is called after tlb_flush_all. >> >> STR rx, [Translation table entry] >> ; write new entry to the translation table >> Clean cache line [Translation table entry] >> DSB >> ; ensures visibility of the data cleaned from the D Cache >> Invalidate TLB entry by MVA (and ASID if non-global) [page address] >> Invalidate BTC >> DSB >> ; ensure completion of the Invalidate TLB operation >> ISB >> ; ensure table changes visible to instruction fetch >> >> The issue is seen only with LPAE + THUMB BUILT KERNEL + 64BIT patching, >> which is little bit weird. > > NAK. > > I don't buy your reasoning. All current LPAE implementations also implement > the multi-processing extensions, meaning that the cache flush isn't required > to make the PTEs visible to the table walker. The dsb from the TLB_WB flag > is sufficient, so I think you still have some debugging to do as this change > is likely masking a problem elsewhere. > > On top of that, create_mapping does all the flushing you need (for the !SMP > case) when the tables are initialised, so this code doesn't need changing. > Fair enough. We will drop this patch from this series and continue to look at the issue further. As such the patch has no hard dependency with rest of the series. Regards, Santosh
Will, Will, On Friday 04 October 2013 12:03 PM, Santosh Shilimkar wrote: > On Friday 04 October 2013 11:52 AM, Will Deacon wrote: >> On Thu, Oct 03, 2013 at 10:18:00PM +0100, Santosh Shilimkar wrote: >>> From: Sricharan R <r.sricharan@ti.com> >>> >>> As per the arm ARMv7 manual, the sequence of TLB maintenance >>> operations after making changes to the translation table is >>> to clean the dcache first, then invalidate the TLB. With >>> the current sequence we see cache corruption when the >>> flush_cache_all is called after tlb_flush_all. >>> >>> STR rx, [Translation table entry] >>> ; write new entry to the translation table >>> Clean cache line [Translation table entry] >>> DSB >>> ; ensures visibility of the data cleaned from the D Cache >>> Invalidate TLB entry by MVA (and ASID if non-global) [page address] >>> Invalidate BTC >>> DSB >>> ; ensure completion of the Invalidate TLB operation >>> ISB >>> ; ensure table changes visible to instruction fetch >>> >>> The issue is seen only with LPAE + THUMB BUILT KERNEL + 64BIT patching, >>> which is little bit weird. >> >> NAK. >> >> I don't buy your reasoning. All current LPAE implementations also implement >> the multi-processing extensions, meaning that the cache flush isn't required >> to make the PTEs visible to the table walker. The dsb from the TLB_WB flag >> is sufficient, so I think you still have some debugging to do as this change >> is likely masking a problem elsewhere. >> >> On top of that, create_mapping does all the flushing you need (for the !SMP >> case) when the tables are initialised, so this code doesn't need changing. >> > Fair enough. We will drop this patch from this series and continue to look > at the issue further. As such the patch has no hard dependency with rest of > the series. > Just to update the thread, Sricharan tracked down this issue now and the 64 bit patch is fixed. Thanks for NAK ;) Regards, Santosh
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c index 47c7497..49cba8a 100644 --- a/arch/arm/mm/mmu.c +++ b/arch/arm/mm/mmu.c @@ -1280,8 +1280,8 @@ static void __init devicemaps_init(const struct machine_desc *mdesc) * any write-allocated cache lines in the vector page are written * back. After this point, we can start to touch devices again. */ - local_flush_tlb_all(); flush_cache_all(); + local_flush_tlb_all(); } static void __init kmap_init(void)