[PATCH 1/2] Fix the I-cache invalidation on ARMv6 and later CPUs

[PATCH 1/2] Fix the I-cache invalidation on ARMv6 and later CPUs

[Minchan Kim]

Sorry for the noise by old mailing list address.
Fix and resend.
Thanks.

On Tue, Nov 9, 2010 at 2:03 PM, Minchan Kim <minchan.kim@gmail.com> wrote:
> It's too old story.
> Sorry for the late question.
>
> On Fri, Jun 6, 2008 at 8:46 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> This patch adds the I-cache invalidation in update_mmu_cache if the
>> corresponding vma is marked as executable. It also invalidates the
>> I-cache if a thread migrates to a CPU it never ran on.
>
> The description says just two place in update_mmu_cache and CPU
> migration point.
> I agree on it.
>
>>
>> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
>> ---
>>
>> ?arch/arm/mm/fault-armv.c ? ? ?| ? ?4 ++++
>> ?arch/arm/mm/flush.c ? ? ? ? ? | ? ?2 ++
>> ?include/asm-arm/cacheflush.h ?| ? ?7 +++++++
>> ?include/asm-arm/mmu_context.h | ? ?5 +++++
>> ?4 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mm/fault-armv.c b/arch/arm/mm/fault-armv.c
>> index 44558d5..fbfa260 100644
>> --- a/arch/arm/mm/fault-armv.c
>> +++ b/arch/arm/mm/fault-armv.c
>> @@ -144,13 +144,17 @@ void update_mmu_cache(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
>> ? ? ? ?page = pfn_to_page(pfn);
>> ? ? ? ?mapping = page_mapping(page);
>> ? ? ? ?if (mapping) {
>> +#ifndef CONFIG_SMP
>> ? ? ? ? ? ? ? ?int dirty = test_and_clear_bit(PG_dcache_dirty, &page->flags);
>>
>> ? ? ? ? ? ? ? ?if (dirty)
>> ? ? ? ? ? ? ? ? ? ? ? ?__flush_dcache_page(mapping, page);
>> +#endif
>>
>> ? ? ? ? ? ? ? ?if (cache_is_vivt())
>> ? ? ? ? ? ? ? ? ? ? ? ?make_coherent(mapping, vma, addr, pfn);
>> + ? ? ? ? ? ? ? else if (vma->vm_flags & VM_EXEC)
>> + ? ? ? ? ? ? ? ? ? ? ? __flush_icache_all();
>> ? ? ? ?}
>> ?}
>>
>> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
>> index 9df507d..029ee65 100644
>> --- a/arch/arm/mm/flush.c
>> +++ b/arch/arm/mm/flush.c
>> @@ -199,6 +199,8 @@ void flush_dcache_page(struct page *page)
>> ? ? ? ? ? ? ? ?__flush_dcache_page(mapping, page);
>> ? ? ? ? ? ? ? ?if (mapping && cache_is_vivt())
>> ? ? ? ? ? ? ? ? ? ? ? ?__flush_dcache_aliases(mapping, page);
>> + ? ? ? ? ? ? ? else if (mapping)
>> + ? ? ? ? ? ? ? ? ? ? ? __flush_icache_all();
>> ? ? ? ?}
>
> Here, let me ask questions.
> 1. Why do we flush icache in ?flush_dcache_page? Does it really needed?
> 2. If we really need it, should we even flush it in !VM_EXEC?
>
> It was rather changed in 2.6.27-rc series. But still it flushes icache
> in flush_dcache_page regardless of executable permission.
>
> --
> Kind regards,
> Minchan Kim
>



-- 
Kind regards,
Minchan Kim

[PATCH 1/2] Fix the I-cache invalidation on ARMv6 and later CPUs

[Catalin Marinas]

> On Tue, Nov 9, 2010 at 2:03 PM, Minchan Kim <minchan.kim@gmail.com> wrote:
> > On Fri, Jun 6, 2008 at 8:46 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> >> This patch adds the I-cache invalidation in update_mmu_cache if the
> >> corresponding vma is marked as executable. It also invalidates the
> >> I-cache if a thread migrates to a CPU it never ran on.
> >
> > The description says just two place in update_mmu_cache and CPU
> > migration point.
> > I agree on it.
> >
> >>
> >> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> >> ---
> >>
> >>  arch/arm/mm/fault-armv.c      |    4 ++++
> >>  arch/arm/mm/flush.c           |    2 ++
> >>  include/asm-arm/cacheflush.h  |    7 +++++++
> >>  include/asm-arm/mmu_context.h |    5 +++++
> >>  4 files changed, 18 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/arm/mm/fault-armv.c b/arch/arm/mm/fault-armv.c
> >> index 44558d5..fbfa260 100644
> >> --- a/arch/arm/mm/fault-armv.c
> >> +++ b/arch/arm/mm/fault-armv.c
> >> @@ -144,13 +144,17 @@ void update_mmu_cache(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
> >>        page = pfn_to_page(pfn);
> >>        mapping = page_mapping(page);
> >>        if (mapping) {
> >> +#ifndef CONFIG_SMP
> >>                int dirty = test_and_clear_bit(PG_dcache_dirty, &page->flags);
> >>
> >>                if (dirty)
> >>                        __flush_dcache_page(mapping, page);
> >> +#endif
> >>
> >>                if (cache_is_vivt())
> >>                        make_coherent(mapping, vma, addr, pfn);
> >> +               else if (vma->vm_flags & VM_EXEC)
> >> +                       __flush_icache_all();
> >>        }
> >>  }
> >>
> >> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
> >> index 9df507d..029ee65 100644
> >> --- a/arch/arm/mm/flush.c
> >> +++ b/arch/arm/mm/flush.c
> >> @@ -199,6 +199,8 @@ void flush_dcache_page(struct page *page)
> >>                __flush_dcache_page(mapping, page);
> >>                if (mapping && cache_is_vivt())
> >>                        __flush_dcache_aliases(mapping, page);
> >> +               else if (mapping)
> >> +                       __flush_icache_all();
> >>        }
> >
> > Here, let me ask questions.
> > 1. Why do we flush icache in  flush_dcache_page? Does it really needed?

Note that these functions are changed slightly in 2.6.37-rc1, but we
still do the I-cache flushing.

There reason is that the page being modified may already be mapped in
user space and may be a code page as well. The kernel may call this
function after it modified the page and therefore we need to ensure the
I-D cache coherency.

IMHO, the API could be slightly improved since the kernel calls this
function even before reading a page cache page but we don't really need
to invalidate the I-cache at that point.

> > 2. If we really need it, should we even flush it in !VM_EXEC?

We don't have the vma in flush_dcache_page to be able to check for
VM_EXEC.

Catalin

[PATCH 1/2] Fix the I-cache invalidation on ARMv6 and later CPUs

[Minchan Kim]

On Tue, Nov 9, 2010 at 7:17 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> On Tue, Nov 9, 2010 at 2:03 PM, Minchan Kim <minchan.kim@gmail.com> wrote:
>> > On Fri, Jun 6, 2008 at 8:46 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> >> This patch adds the I-cache invalidation in update_mmu_cache if the
>> >> corresponding vma is marked as executable. It also invalidates the
>> >> I-cache if a thread migrates to a CPU it never ran on.
>> >
>> > The description says just two place in update_mmu_cache and CPU
>> > migration point.
>> > I agree on it.
>> >
>> >>
>> >> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
>> >> ---
>> >>
>> >> ?arch/arm/mm/fault-armv.c ? ? ?| ? ?4 ++++
>> >> ?arch/arm/mm/flush.c ? ? ? ? ? | ? ?2 ++
>> >> ?include/asm-arm/cacheflush.h ?| ? ?7 +++++++
>> >> ?include/asm-arm/mmu_context.h | ? ?5 +++++
>> >> ?4 files changed, 18 insertions(+), 0 deletions(-)
>> >>
>> >> diff --git a/arch/arm/mm/fault-armv.c b/arch/arm/mm/fault-armv.c
>> >> index 44558d5..fbfa260 100644
>> >> --- a/arch/arm/mm/fault-armv.c
>> >> +++ b/arch/arm/mm/fault-armv.c
>> >> @@ -144,13 +144,17 @@ void update_mmu_cache(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
>> >> ? ? ? ?page = pfn_to_page(pfn);
>> >> ? ? ? ?mapping = page_mapping(page);
>> >> ? ? ? ?if (mapping) {
>> >> +#ifndef CONFIG_SMP
>> >> ? ? ? ? ? ? ? ?int dirty = test_and_clear_bit(PG_dcache_dirty, &page->flags);
>> >>
>> >> ? ? ? ? ? ? ? ?if (dirty)
>> >> ? ? ? ? ? ? ? ? ? ? ? ?__flush_dcache_page(mapping, page);
>> >> +#endif
>> >>
>> >> ? ? ? ? ? ? ? ?if (cache_is_vivt())
>> >> ? ? ? ? ? ? ? ? ? ? ? ?make_coherent(mapping, vma, addr, pfn);
>> >> + ? ? ? ? ? ? ? else if (vma->vm_flags & VM_EXEC)
>> >> + ? ? ? ? ? ? ? ? ? ? ? __flush_icache_all();
>> >> ? ? ? ?}
>> >> ?}
>> >>
>> >> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
>> >> index 9df507d..029ee65 100644
>> >> --- a/arch/arm/mm/flush.c
>> >> +++ b/arch/arm/mm/flush.c
>> >> @@ -199,6 +199,8 @@ void flush_dcache_page(struct page *page)
>> >> ? ? ? ? ? ? ? ?__flush_dcache_page(mapping, page);
>> >> ? ? ? ? ? ? ? ?if (mapping && cache_is_vivt())
>> >> ? ? ? ? ? ? ? ? ? ? ? ?__flush_dcache_aliases(mapping, page);
>> >> + ? ? ? ? ? ? ? else if (mapping)
>> >> + ? ? ? ? ? ? ? ? ? ? ? __flush_icache_all();
>> >> ? ? ? ?}
>> >
>> > Here, let me ask questions.
>> > 1. Why do we flush icache in ?flush_dcache_page? Does it really needed?
>
> Note that these functions are changed slightly in 2.6.37-rc1, but we
> still do the I-cache flushing.
>
> There reason is that the page being modified may already be mapped in
> user space and may be a code page as well. The kernel may call this

Yes. That's my point. We can't know code or not in that context(ie,
flush_dcache_page).

> function after it modified the page and therefore we need to ensure the
> I-D cache coherency.

Yes. Only if the page is code page.
(I might miss something.That's why I ask a question).

>
> IMHO, the API could be slightly improved since the kernel calls this
> function even before reading a page cache page but we don't really need
> to invalidate the I-cache at that point.

Agree.

>
>> > 2. If we really need it, should we even flush it in !VM_EXEC?
>
> We don't have the vma in flush_dcache_page to be able to check for
> VM_EXEC.

Yes. We did it in update_mmu_cache through VM_EXEC check.

In 2.6.37-rc1, we flushes icache without VM_EXEC check in set_pte_at
which doesn't have VMA, either.
Could we move the icache flush to update_mmu_cache again?
I think it's right place to update_mmu_cache like old behavior.
What was the problem in there?

Thanks for the quick response, Catalin.


-- 
Kind regards,
Minchan Kim

[PATCH 1/2] Fix the I-cache invalidation on ARMv6 and later CPUs

[Russell King - ARM Linux]

On Tue, Nov 09, 2010 at 07:34:45PM +0900, Minchan Kim wrote:
> On Tue, Nov 9, 2010 at 7:17 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > Note that these functions are changed slightly in 2.6.37-rc1, but we
> > still do the I-cache flushing.
> >
> > There reason is that the page being modified may already be mapped in
> > user space and may be a code page as well. The kernel may call this
> 
> Yes. That's my point. We can't know code or not in that context(ie,
> flush_dcache_page).

And because we don't know, we have to assume that the page does contain
code, so we have to flush the I cache.

> In 2.6.37-rc1, we flushes icache without VM_EXEC check in set_pte_at
> which doesn't have VMA, either.
> Could we move the icache flush to update_mmu_cache again?

No.  We moved it to fix a race on SMP - between the PTE being established
and the caches being coherent.  Moving it later reintroduces that race
again.

[PATCH 1/2] Fix the I-cache invalidation on ARMv6 and later CPUs

[Minchan Kim]

On Tue, Nov 9, 2010 at 7:38 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Nov 09, 2010 at 07:34:45PM +0900, Minchan Kim wrote:
>> On Tue, Nov 9, 2010 at 7:17 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> > Note that these functions are changed slightly in 2.6.37-rc1, but we
>> > still do the I-cache flushing.
>> >
>> > There reason is that the page being modified may already be mapped in
>> > user space and may be a code page as well. The kernel may call this
>>
>> Yes. That's my point. We can't know code or not in that context(ie,
>> flush_dcache_page).
>
> And because we don't know, we have to assume that the page does contain
> code, so we have to flush the I cache.
>
>> In 2.6.37-rc1, we flushes icache without VM_EXEC check in set_pte_at
>> which doesn't have VMA, either.
>> Could we move the icache flush to update_mmu_cache again?
>
> No. ?We moved it to fix a race on SMP - between the PTE being established
> and the caches being coherent. ?Moving it later reintroduces that race
> again.
>

Thanks, I understand the problem by your kind explanation.
So, why don't we use flush_icache_page?

page fault
read
flush_icache_page <------ here
set_pte_at
update_mmu_cache


-- 
Kind regards,
Minchan Kim

[PATCH 1/2] Fix the I-cache invalidation on ARMv6 and later CPUs

[Russell King - ARM Linux]

On Tue, Nov 09, 2010 at 07:52:15PM +0900, Minchan Kim wrote:
> Thanks, I understand the problem by your kind explanation.
> So, why don't we use flush_icache_page?
> 
> page fault
> read
> flush_icache_page <------ here
> set_pte_at
> update_mmu_cache

Have you read Documentation/cachetlb.txt ?

[PATCH 1/2] Fix the I-cache invalidation on ARMv6 and later CPUs

[Minchan Kim]

On Tue, Nov 9, 2010 at 7:53 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Nov 09, 2010 at 07:52:15PM +0900, Minchan Kim wrote:
>> Thanks, I understand the problem by your kind explanation.
>> So, why don't we use flush_icache_page?
>>
>> page fault
>> read
>> flush_icache_page <------ here
>> set_pte_at
>> update_mmu_cache
>
> Have you read Documentation/cachetlb.txt ?
>

Thanks for the pointing me.
Maybe we need change it with following as.

  void flush_icache_page(struct vm_area_struct *vma, struct page *page)
        All the functionality of flush_icache_page can be implemented in
        flush_dcache_page or __sync_icache_dcache in set_pte_ext(ex
ARM), update_mmu_cache and set_pte_at. In 2.7 the hope is to
        remove this interface completely.




-- 
Kind regards,
Minchan Kim