[PATCH V2] arm64: add DSB after icache flush in __flush_icache_all()

[PATCH V2] arm64: add DSB after icache flush in __flush_icache_all()

[Vinayak Kale]

Add DSB after icache flush. It's needed to complete the cache maintenance
operation. The function __flush_icache_all() is used only for user space
mappings and an ISB is not required because of an exception return before
executing user instructions. An exception return would behave like an ISB.

This patch also uses 'memory' clobber for flush operation instruction to 
prevent instruction re-ordering by compiler. 

Signed-off-by: Vinayak Kale <vkale@apm.com>
---

V2: - Add more desciption in the commit message as suggested by Catalin & Will
    - Use 'memory' clobber for flush instruction as suggested by Will
    

 arch/arm64/include/asm/cacheflush.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h
index fea9ee3..bd30f42 100644
--- a/arch/arm64/include/asm/cacheflush.h
+++ b/arch/arm64/include/asm/cacheflush.h
@@ -115,7 +115,8 @@ extern void flush_dcache_page(struct page *);
 
 static inline void __flush_icache_all(void)
 {
-	asm("ic	ialluis");
+	asm volatile("ic ialluis" : : : "memory");
+	dsb();
 }
 
 #define flush_dcache_mmap_lock(mapping) \
-- 
1.8.2.1

[PATCH V2] arm64: add DSB after icache flush in __flush_icache_all()

[Will Deacon]

On Tue, Jan 28, 2014 at 07:06:53AM +0000, Vinayak Kale wrote:
> Add DSB after icache flush. It's needed to complete the cache maintenance
> operation. The function __flush_icache_all() is used only for user space
> mappings and an ISB is not required because of an exception return before
> executing user instructions. An exception return would behave like an ISB.
> 
> This patch also uses 'memory' clobber for flush operation instruction to 
> prevent instruction re-ordering by compiler. 
> 
> Signed-off-by: Vinayak Kale <vkale@apm.com>
> ---
> 
> V2: - Add more desciption in the commit message as suggested by Catalin & Will
>     - Use 'memory' clobber for flush instruction as suggested by Will

Please can you check and fix other occurrences of this bug too, as I asked
in v1? For example, a 2 second grep shows problems with data-cache
maintenance in kvm. I can also see the same problem for system register
writes followed up with isb.

I also don't buy you not being able to test AArch32 kernels. Does KVM not
work for you?

Will

> 
>  arch/arm64/include/asm/cacheflush.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h
> index fea9ee3..bd30f42 100644
> --- a/arch/arm64/include/asm/cacheflush.h
> +++ b/arch/arm64/include/asm/cacheflush.h
> @@ -115,7 +115,8 @@ extern void flush_dcache_page(struct page *);
>  
>  static inline void __flush_icache_all(void)
>  {
> -	asm("ic	ialluis");
> +	asm volatile("ic ialluis" : : : "memory");
> +	dsb();
>  }
>  
>  #define flush_dcache_mmap_lock(mapping) \
> -- 
> 1.8.2.1
> 
> 

[PATCH V2] arm64: add DSB after icache flush in __flush_icache_all()

[Vinayak Kale]

Hi Will,

On Tue, Jan 28, 2014 at 9:44 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jan 28, 2014 at 07:06:53AM +0000, Vinayak Kale wrote:
>> Add DSB after icache flush. It's needed to complete the cache maintenance
>> operation. The function __flush_icache_all() is used only for user space
>> mappings and an ISB is not required because of an exception return before
>> executing user instructions. An exception return would behave like an ISB.
>>
>> This patch also uses 'memory' clobber for flush operation instruction to
>> prevent instruction re-ordering by compiler.
>>
>> Signed-off-by: Vinayak Kale <vkale@apm.com>
>> ---
>>
>> V2: - Add more desciption in the commit message as suggested by Catalin & Will
>>     - Use 'memory' clobber for flush instruction as suggested by Will
>
> Please can you check and fix other occurrences of this bug too, as I asked
> in v1? For example, a 2 second grep shows problems with data-cache
> maintenance in kvm. I can also see the same problem for system register
> writes followed up with isb.
Can you please elaborate whether you are referring to lack of memory
clobber or missing barriers?
>
> I also don't buy you not being able to test AArch32 kernels. Does KVM not
> work for you?
Let me see what I can do to address this.
>
> Will
>
>>
>>  arch/arm64/include/asm/cacheflush.h | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h
>> index fea9ee3..bd30f42 100644
>> --- a/arch/arm64/include/asm/cacheflush.h
>> +++ b/arch/arm64/include/asm/cacheflush.h
>> @@ -115,7 +115,8 @@ extern void flush_dcache_page(struct page *);
>>
>>  static inline void __flush_icache_all(void)
>>  {
>> -     asm("ic ialluis");
>> +     asm volatile("ic ialluis" : : : "memory");
>> +     dsb();
>>  }
>>
>>  #define flush_dcache_mmap_lock(mapping) \
>> --
>> 1.8.2.1
>>
>>

[PATCH V2] arm64: add DSB after icache flush in __flush_icache_all()

[Will Deacon]

On Thu, Jan 30, 2014 at 06:04:43AM +0000, Vinayak Kale wrote:
> On Tue, Jan 28, 2014 at 9:44 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Jan 28, 2014 at 07:06:53AM +0000, Vinayak Kale wrote:
> >> V2: - Add more desciption in the commit message as suggested by Catalin & Will
> >>     - Use 'memory' clobber for flush instruction as suggested by Will
> >
> > Please can you check and fix other occurrences of this bug too, as I asked
> > in v1? For example, a 2 second grep shows problems with data-cache
> > maintenance in kvm. I can also see the same problem for system register
> > writes followed up with isb.
> Can you please elaborate whether you are referring to lack of memory
> clobber or missing barriers?

The clobbers. For example:

arch/arm64/kvm/sys_regs.c:

        /* Make sure noone else changes CSSELR during this! */
        local_irq_disable();
        /* Put value into CSSELR */
        asm volatile("msr csselr_el1, %x0" : : "r" (csselr));
        isb();
        /* Read result out of CCSIDR */
        asm volatile("mrs %0, ccsidr_el1" : "=r" (ccsidr));
        local_irq_enable();

Just about everything can be re-ordered in that block, because the asm
volatile statements don't have "memory" clobbers.

I think it's worth checking the rest of the kernel, too.

Will

[PATCH V2] arm64: add DSB after icache flush in __flush_icache_all()

[Nicolas Pitre]

On Thu, 30 Jan 2014, Will Deacon wrote:

> On Thu, Jan 30, 2014 at 06:04:43AM +0000, Vinayak Kale wrote:
> > On Tue, Jan 28, 2014 at 9:44 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > On Tue, Jan 28, 2014 at 07:06:53AM +0000, Vinayak Kale wrote:
> > >> V2: - Add more desciption in the commit message as suggested by Catalin & Will
> > >>     - Use 'memory' clobber for flush instruction as suggested by Will
> > >
> > > Please can you check and fix other occurrences of this bug too, as I asked
> > > in v1? For example, a 2 second grep shows problems with data-cache
> > > maintenance in kvm. I can also see the same problem for system register
> > > writes followed up with isb.
> > Can you please elaborate whether you are referring to lack of memory
> > clobber or missing barriers?
> 
> The clobbers. For example:
> 
> arch/arm64/kvm/sys_regs.c:
> 
>         /* Make sure noone else changes CSSELR during this! */
>         local_irq_disable();
>         /* Put value into CSSELR */
>         asm volatile("msr csselr_el1, %x0" : : "r" (csselr));
>         isb();
>         /* Read result out of CCSIDR */
>         asm volatile("mrs %0, ccsidr_el1" : "=r" (ccsidr));
>         local_irq_enable();
> 
> Just about everything can be re-ordered in that block, because the asm
> volatile statements don't have "memory" clobbers.

I don't think they would be reordered at all with the 
volatile qualifiers.


Nicolas

[PATCH V2] arm64: add DSB after icache flush in __flush_icache_all()

[Will Deacon]

Hi Nico,

On Thu, Jan 30, 2014 at 09:42:29PM +0000, Nicolas Pitre wrote:
> On Thu, 30 Jan 2014, Will Deacon wrote:
> > On Thu, Jan 30, 2014 at 06:04:43AM +0000, Vinayak Kale wrote:
> > > Can you please elaborate whether you are referring to lack of memory
> > > clobber or missing barriers?
> > 
> > The clobbers. For example:
> > 
> > arch/arm64/kvm/sys_regs.c:
> > 
> >         /* Make sure noone else changes CSSELR during this! */
> >         local_irq_disable();
> >         /* Put value into CSSELR */
> >         asm volatile("msr csselr_el1, %x0" : : "r" (csselr));
> >         isb();
> >         /* Read result out of CCSIDR */
> >         asm volatile("mrs %0, ccsidr_el1" : "=r" (ccsidr));
> >         local_irq_enable();
> > 
> > Just about everything can be re-ordered in that block, because the asm
> > volatile statements don't have "memory" clobbers.
> 
> I don't think they would be reordered at all with the 
> volatile qualifiers.

Whilst that may be the case in current compilers (i.e. I've not actually
seen the above sequence get re-ordered), the GCC documentation states that:

  Similarly, you can't expect a sequence of volatile asm instructions to remain
  perfectly consecutive. If you want consecutive output, use a single asm. Also,
  GCC performs some optimizations across a volatile asm instruction; GCC does not
  `forget everything' when it encounters a volatile asm instruction the way some
  other compilers do.

so I really think that the "memory" clobbers are needed to ensure strict
ordering. This matches my understanding from discussions with the compiler
engineers at ARM.

Will

[PATCH V2] arm64: add DSB after icache flush in __flush_icache_all()

[Nicolas Pitre]

On Fri, 31 Jan 2014, Will Deacon wrote:

> Hi Nico,
> 
> On Thu, Jan 30, 2014 at 09:42:29PM +0000, Nicolas Pitre wrote:
> > On Thu, 30 Jan 2014, Will Deacon wrote:
> > > On Thu, Jan 30, 2014 at 06:04:43AM +0000, Vinayak Kale wrote:
> > > > Can you please elaborate whether you are referring to lack of memory
> > > > clobber or missing barriers?
> > > 
> > > The clobbers. For example:
> > > 
> > > arch/arm64/kvm/sys_regs.c:
> > > 
> > >         /* Make sure noone else changes CSSELR during this! */
> > >         local_irq_disable();
> > >         /* Put value into CSSELR */
> > >         asm volatile("msr csselr_el1, %x0" : : "r" (csselr));
> > >         isb();
> > >         /* Read result out of CCSIDR */
> > >         asm volatile("mrs %0, ccsidr_el1" : "=r" (ccsidr));
> > >         local_irq_enable();
> > > 
> > > Just about everything can be re-ordered in that block, because the asm
> > > volatile statements don't have "memory" clobbers.
> > 
> > I don't think they would be reordered at all with the 
> > volatile qualifiers.
> 
> Whilst that may be the case in current compilers (i.e. I've not actually
> seen the above sequence get re-ordered), the GCC documentation states that:
> 
>   Similarly, you can't expect a sequence of volatile asm instructions to remain
>   perfectly consecutive. If you want consecutive output, use a single asm. Also,
>   GCC performs some optimizations across a volatile asm instruction; GCC does not
>   `forget everything' when it encounters a volatile asm instruction the way some
>   other compilers do.
> 
> so I really think that the "memory" clobbers are needed to ensure strict
> ordering. This matches my understanding from discussions with the compiler
> engineers at ARM.

Well, the key sentence above is: "you can't expect a sequence of 
volatile asm instructions to remain perfectly consecutive."  That 
doesn't mean the "ordering" won't be respected.

If you need a certain ordering, then a volatile is all that you need.  
If nothing else should happen in between, then just use a single asm 
statement as suggested.

Only if you need ordering with regards to other memory accesses as well 
then yes you need a memory clobber in that case.


Nicolas

[PATCH V2] arm64: add DSB after icache flush in __flush_icache_all()

[Russell King - ARM Linux]

On Fri, Jan 31, 2014 at 12:16:48AM +0000, Will Deacon wrote:
> Hi Nico,
> 
> On Thu, Jan 30, 2014 at 09:42:29PM +0000, Nicolas Pitre wrote:
> > On Thu, 30 Jan 2014, Will Deacon wrote:
> > > On Thu, Jan 30, 2014 at 06:04:43AM +0000, Vinayak Kale wrote:
> > > > Can you please elaborate whether you are referring to lack of memory
> > > > clobber or missing barriers?
> > > 
> > > The clobbers. For example:
> > > 
> > > arch/arm64/kvm/sys_regs.c:
> > > 
> > >         /* Make sure noone else changes CSSELR during this! */
> > >         local_irq_disable();
> > >         /* Put value into CSSELR */
> > >         asm volatile("msr csselr_el1, %x0" : : "r" (csselr));
> > >         isb();
> > >         /* Read result out of CCSIDR */
> > >         asm volatile("mrs %0, ccsidr_el1" : "=r" (ccsidr));
> > >         local_irq_enable();
> > > 
> > > Just about everything can be re-ordered in that block, because the asm
> > > volatile statements don't have "memory" clobbers.
> > 
> > I don't think they would be reordered at all with the 
> > volatile qualifiers.
> 
> Whilst that may be the case in current compilers (i.e. I've not actually
> seen the above sequence get re-ordered), the GCC documentation states that:
> 
>   Similarly, you can't expect a sequence of volatile asm instructions to remain
>   perfectly consecutive. If you want consecutive output, use a single asm. Also,
>   GCC performs some optimizations across a volatile asm instruction; GCC does not
>   `forget everything' when it encounters a volatile asm instruction the way some
>   other compilers do.
> 
> so I really think that the "memory" clobbers are needed to ensure strict
> ordering. This matches my understanding from discussions with the compiler
> engineers at ARM.

What it means is that the compiler may introduce additional instructions
between your consecutive asm() statements.  So there's no guarantee that
the ISB will immediately follow the MSR instruction - there may be other
instructions which the compiler may decide to schedule between the two.

For example, instructions to load the address of the variable(s) may be
inserted between the assembly specified in the asm() statements which
may involve loading from a literal pool.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH V2] arm64: add DSB after icache flush in __flush_icache_all()

[Will Deacon]

Hi Nico, Russell,

Thanks for the replies.

On Fri, Jan 31, 2014 at 10:48:50AM +0000, Russell King - ARM Linux wrote:
> On Fri, Jan 31, 2014 at 12:16:48AM +0000, Will Deacon wrote:
> > On Thu, Jan 30, 2014 at 09:42:29PM +0000, Nicolas Pitre wrote:
> > > I don't think they would be reordered at all with the 
> > > volatile qualifiers.
> > 
> > Whilst that may be the case in current compilers (i.e. I've not actually
> > seen the above sequence get re-ordered), the GCC documentation states that:
> > 
> >   Similarly, you can't expect a sequence of volatile asm instructions to remain
> >   perfectly consecutive. If you want consecutive output, use a single asm. Also,
> >   GCC performs some optimizations across a volatile asm instruction; GCC does not
> >   `forget everything' when it encounters a volatile asm instruction the way some
> >   other compilers do.
> > 
> > so I really think that the "memory" clobbers are needed to ensure strict
> > ordering. This matches my understanding from discussions with the compiler
> > engineers at ARM.
> 
> What it means is that the compiler may introduce additional instructions
> between your consecutive asm() statements.  So there's no guarantee that
> the ISB will immediately follow the MSR instruction - there may be other
> instructions which the compiler may decide to schedule between the two.
> 
> For example, instructions to load the address of the variable(s) may be
> inserted between the assembly specified in the asm() statements which
> may involve loading from a literal pool.

That matches what Nicolas said and, to be honest, makes a lot of sense. I'm
just slightly concerned that it doesn't match the explanation I received
from some compiler guys, but I can chase that down separately.

Vinayak: sorry for leading you down the garden path on this. Please can you
stick with your original patch, but adding something equivalent for
arch/arm?

Cheers,

Will

[PATCH V2] arm64: add DSB after icache flush in __flush_icache_all()

[Vinayak Kale]

On Mon, Feb 3, 2014 at 4:47 PM, Will Deacon <will.deacon@arm.com> wrote:
> Hi Nico, Russell,
>
> Thanks for the replies.
>
> On Fri, Jan 31, 2014 at 10:48:50AM +0000, Russell King - ARM Linux wrote:
>> On Fri, Jan 31, 2014 at 12:16:48AM +0000, Will Deacon wrote:
>> > On Thu, Jan 30, 2014 at 09:42:29PM +0000, Nicolas Pitre wrote:
>> > > I don't think they would be reordered at all with the
>> > > volatile qualifiers.
>> >
>> > Whilst that may be the case in current compilers (i.e. I've not actually
>> > seen the above sequence get re-ordered), the GCC documentation states that:
>> >
>> >   Similarly, you can't expect a sequence of volatile asm instructions to remain
>> >   perfectly consecutive. If you want consecutive output, use a single asm. Also,
>> >   GCC performs some optimizations across a volatile asm instruction; GCC does not
>> >   `forget everything' when it encounters a volatile asm instruction the way some
>> >   other compilers do.
>> >
>> > so I really think that the "memory" clobbers are needed to ensure strict
>> > ordering. This matches my understanding from discussions with the compiler
>> > engineers at ARM.
>>
>> What it means is that the compiler may introduce additional instructions
>> between your consecutive asm() statements.  So there's no guarantee that
>> the ISB will immediately follow the MSR instruction - there may be other
>> instructions which the compiler may decide to schedule between the two.
>>
>> For example, instructions to load the address of the variable(s) may be
>> inserted between the assembly specified in the asm() statements which
>> may involve loading from a literal pool.
>
> That matches what Nicolas said and, to be honest, makes a lot of sense. I'm
> just slightly concerned that it doesn't match the explanation I received
> from some compiler guys, but I can chase that down separately.
>
> Vinayak: sorry for leading you down the garden path on this. Please can you
> stick with your original patch, but adding something equivalent for
> arch/arm?
I have posted a patch for arch/arm, have tested it for ARM-v7.
>
> Cheers,
>
> Will