All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] riscv: Add sfence.vma after page table changed
@ 2020-08-04  3:02 ` Greentime Hu
  0 siblings, 0 replies; 12+ messages in thread
From: Greentime Hu @ 2020-08-04  3:02 UTC (permalink / raw)
  To: greentime.hu, linux-riscv, linux-kernel, aou, palmer,
	paul.walmsley, syven.wang

This patch addes local_flush_tlb_page(addr) to use sfence.vma after the
page table changed. That address will be used immediately in
memset(nextp, 0, PAGE_SIZE) to cause this issue so we should add the
sfence.vma before we use it.

Fixes: f2c17aabc917 ("RISC-V: Implement compile-time fixed mappings")

Reported-by: Syven Wang <syven.wang@sifive.com>
Signed-off-by: Syven Wang <syven.wang@sifive.com>
Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
---
 arch/riscv/mm/init.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index f4adb3684f3d..29b0f7108054 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -202,12 +202,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
 
 	ptep = &fixmap_pte[pte_index(addr)];
 
-	if (pgprot_val(prot)) {
+	if (pgprot_val(prot))
 		set_pte(ptep, pfn_pte(phys >> PAGE_SHIFT, prot));
-	} else {
+	else
 		pte_clear(&init_mm, addr, ptep);
-		local_flush_tlb_page(addr);
-	}
+	local_flush_tlb_page(addr);
 }
 
 static pte_t *__init get_pte_virt(phys_addr_t pa)
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH] riscv: Add sfence.vma after page table changed
@ 2020-08-04  3:02 ` Greentime Hu
  0 siblings, 0 replies; 12+ messages in thread
From: Greentime Hu @ 2020-08-04  3:02 UTC (permalink / raw)
  To: greentime.hu, linux-riscv, linux-kernel, aou, palmer,
	paul.walmsley, syven.wang

This patch addes local_flush_tlb_page(addr) to use sfence.vma after the
page table changed. That address will be used immediately in
memset(nextp, 0, PAGE_SIZE) to cause this issue so we should add the
sfence.vma before we use it.

Fixes: f2c17aabc917 ("RISC-V: Implement compile-time fixed mappings")

Reported-by: Syven Wang <syven.wang@sifive.com>
Signed-off-by: Syven Wang <syven.wang@sifive.com>
Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
---
 arch/riscv/mm/init.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index f4adb3684f3d..29b0f7108054 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -202,12 +202,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
 
 	ptep = &fixmap_pte[pte_index(addr)];
 
-	if (pgprot_val(prot)) {
+	if (pgprot_val(prot))
 		set_pte(ptep, pfn_pte(phys >> PAGE_SHIFT, prot));
-	} else {
+	else
 		pte_clear(&init_mm, addr, ptep);
-		local_flush_tlb_page(addr);
-	}
+	local_flush_tlb_page(addr);
 }
 
 static pte_t *__init get_pte_virt(phys_addr_t pa)
-- 
2.28.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: Add sfence.vma after page table changed
  2020-08-04  3:02 ` Greentime Hu
@ 2020-08-04  3:29   ` Anup Patel
  -1 siblings, 0 replies; 12+ messages in thread
From: Anup Patel @ 2020-08-04  3:29 UTC (permalink / raw)
  To: Greentime Hu
  Cc: linux-riscv, linux-kernel@vger.kernel.org List, Albert Ou,
	Palmer Dabbelt, Paul Walmsley, syven.wang

On Tue, Aug 4, 2020 at 8:32 AM Greentime Hu <greentime.hu@sifive.com> wrote:
>
> This patch addes local_flush_tlb_page(addr) to use sfence.vma after the

s/addes/adds

> page table changed. That address will be used immediately in
> memset(nextp, 0, PAGE_SIZE) to cause this issue so we should add the
> sfence.vma before we use it.

Alternate version of this commit description can be:

Invalidate local TLB after both set_pet() and clear_pte() because the
address can be used immediately after page table change.

>
> Fixes: f2c17aabc917 ("RISC-V: Implement compile-time fixed mappings")
>
> Reported-by: Syven Wang <syven.wang@sifive.com>
> Signed-off-by: Syven Wang <syven.wang@sifive.com>
> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> ---
>  arch/riscv/mm/init.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index f4adb3684f3d..29b0f7108054 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -202,12 +202,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
>
>         ptep = &fixmap_pte[pte_index(addr)];
>
> -       if (pgprot_val(prot)) {
> +       if (pgprot_val(prot))
>                 set_pte(ptep, pfn_pte(phys >> PAGE_SHIFT, prot));
> -       } else {
> +       else
>                 pte_clear(&init_mm, addr, ptep);
> -               local_flush_tlb_page(addr);
> -       }
> +       local_flush_tlb_page(addr);
>  }
>
>  static pte_t *__init get_pte_virt(phys_addr_t pa)
> --
> 2.28.0
>

Otherwise looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: Add sfence.vma after page table changed
@ 2020-08-04  3:29   ` Anup Patel
  0 siblings, 0 replies; 12+ messages in thread
From: Anup Patel @ 2020-08-04  3:29 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Albert Ou, syven.wang, linux-kernel@vger.kernel.org List,
	Palmer Dabbelt, Paul Walmsley, linux-riscv

On Tue, Aug 4, 2020 at 8:32 AM Greentime Hu <greentime.hu@sifive.com> wrote:
>
> This patch addes local_flush_tlb_page(addr) to use sfence.vma after the

s/addes/adds

> page table changed. That address will be used immediately in
> memset(nextp, 0, PAGE_SIZE) to cause this issue so we should add the
> sfence.vma before we use it.

Alternate version of this commit description can be:

Invalidate local TLB after both set_pet() and clear_pte() because the
address can be used immediately after page table change.

>
> Fixes: f2c17aabc917 ("RISC-V: Implement compile-time fixed mappings")
>
> Reported-by: Syven Wang <syven.wang@sifive.com>
> Signed-off-by: Syven Wang <syven.wang@sifive.com>
> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> ---
>  arch/riscv/mm/init.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index f4adb3684f3d..29b0f7108054 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -202,12 +202,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
>
>         ptep = &fixmap_pte[pte_index(addr)];
>
> -       if (pgprot_val(prot)) {
> +       if (pgprot_val(prot))
>                 set_pte(ptep, pfn_pte(phys >> PAGE_SHIFT, prot));
> -       } else {
> +       else
>                 pte_clear(&init_mm, addr, ptep);
> -               local_flush_tlb_page(addr);
> -       }
> +       local_flush_tlb_page(addr);
>  }
>
>  static pte_t *__init get_pte_virt(phys_addr_t pa)
> --
> 2.28.0
>

Otherwise looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: Add sfence.vma after page table changed
  2020-08-04  3:29   ` Anup Patel
@ 2020-08-05  2:03     ` Palmer Dabbelt
  -1 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2020-08-05  2:03 UTC (permalink / raw)
  To: catalin.marinas, Will Deacon, Marc Zyngier, mark.rutland, anup
  Cc: greentime.hu, linux-riscv, linux-kernel, aou, Paul Walmsley, syven.wang

On Mon, 03 Aug 2020 20:29:32 PDT (-0700), anup@brainfault.org wrote:
> On Tue, Aug 4, 2020 at 8:32 AM Greentime Hu <greentime.hu@sifive.com> wrote:
>>
>> This patch addes local_flush_tlb_page(addr) to use sfence.vma after the
>
> s/addes/adds
>
>> page table changed. That address will be used immediately in
>> memset(nextp, 0, PAGE_SIZE) to cause this issue so we should add the
>> sfence.vma before we use it.
>
> Alternate version of this commit description can be:
>
> Invalidate local TLB after both set_pet() and clear_pte() because the
> address can be used immediately after page table change.
>
>> Fixes: f2c17aabc917 ("RISC-V: Implement compile-time fixed mappings")
>>
>> Reported-by: Syven Wang <syven.wang@sifive.com>
>> Signed-off-by: Syven Wang <syven.wang@sifive.com>
>> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
>> ---
>>  arch/riscv/mm/init.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>> index f4adb3684f3d..29b0f7108054 100644
>> --- a/arch/riscv/mm/init.c
>> +++ b/arch/riscv/mm/init.c
>> @@ -202,12 +202,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
>>
>>         ptep = &fixmap_pte[pte_index(addr)];
>>
>> -       if (pgprot_val(prot)) {
>> +       if (pgprot_val(prot))
>>                 set_pte(ptep, pfn_pte(phys >> PAGE_SHIFT, prot));
>> -       } else {
>> +       else
>>                 pte_clear(&init_mm, addr, ptep);
>> -               local_flush_tlb_page(addr);
>> -       }
>> +       local_flush_tlb_page(addr);
>>  }

arm64 appears to be upgrading all set_pte()s on valid kernel mappings to
include the fence.  It looks like the message from 7f0b1bf04511 ("arm64: Fix
barriers used for page table modifications") is out of date, as I can't find
create_mapping() any more.  If that was some generic kernel thing then we
should probably upgrade ours as well, but if it was arch/arm64/ code then this
approach seems fine as __set_fixmap() isn't on the hot path -- I guess this is
fine either way, but there may be other issues that the arm64 approach fixes.

Do you guys happen to remember what was going on here?

>>
>>  static pte_t *__init get_pte_virt(phys_addr_t pa)
>> --
>> 2.28.0
>>
>
> Otherwise looks good to me.
>
> Reviewed-by: Anup Patel <anup@brainfault.org>
>
> Regards,
> Anup

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: Add sfence.vma after page table changed
@ 2020-08-05  2:03     ` Palmer Dabbelt
  0 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2020-08-05  2:03 UTC (permalink / raw)
  To: catalin.marinas, Will Deacon, Marc Zyngier, mark.rutland, anup
  Cc: aou, syven.wang, linux-kernel, Paul Walmsley, greentime.hu, linux-riscv

On Mon, 03 Aug 2020 20:29:32 PDT (-0700), anup@brainfault.org wrote:
> On Tue, Aug 4, 2020 at 8:32 AM Greentime Hu <greentime.hu@sifive.com> wrote:
>>
>> This patch addes local_flush_tlb_page(addr) to use sfence.vma after the
>
> s/addes/adds
>
>> page table changed. That address will be used immediately in
>> memset(nextp, 0, PAGE_SIZE) to cause this issue so we should add the
>> sfence.vma before we use it.
>
> Alternate version of this commit description can be:
>
> Invalidate local TLB after both set_pet() and clear_pte() because the
> address can be used immediately after page table change.
>
>> Fixes: f2c17aabc917 ("RISC-V: Implement compile-time fixed mappings")
>>
>> Reported-by: Syven Wang <syven.wang@sifive.com>
>> Signed-off-by: Syven Wang <syven.wang@sifive.com>
>> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
>> ---
>>  arch/riscv/mm/init.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>> index f4adb3684f3d..29b0f7108054 100644
>> --- a/arch/riscv/mm/init.c
>> +++ b/arch/riscv/mm/init.c
>> @@ -202,12 +202,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
>>
>>         ptep = &fixmap_pte[pte_index(addr)];
>>
>> -       if (pgprot_val(prot)) {
>> +       if (pgprot_val(prot))
>>                 set_pte(ptep, pfn_pte(phys >> PAGE_SHIFT, prot));
>> -       } else {
>> +       else
>>                 pte_clear(&init_mm, addr, ptep);
>> -               local_flush_tlb_page(addr);
>> -       }
>> +       local_flush_tlb_page(addr);
>>  }

arm64 appears to be upgrading all set_pte()s on valid kernel mappings to
include the fence.  It looks like the message from 7f0b1bf04511 ("arm64: Fix
barriers used for page table modifications") is out of date, as I can't find
create_mapping() any more.  If that was some generic kernel thing then we
should probably upgrade ours as well, but if it was arch/arm64/ code then this
approach seems fine as __set_fixmap() isn't on the hot path -- I guess this is
fine either way, but there may be other issues that the arm64 approach fixes.

Do you guys happen to remember what was going on here?

>>
>>  static pte_t *__init get_pte_virt(phys_addr_t pa)
>> --
>> 2.28.0
>>
>
> Otherwise looks good to me.
>
> Reviewed-by: Anup Patel <anup@brainfault.org>
>
> Regards,
> Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: Add sfence.vma after page table changed
  2020-08-05  2:03     ` Palmer Dabbelt
@ 2020-08-05  8:12       ` Will Deacon
  -1 siblings, 0 replies; 12+ messages in thread
From: Will Deacon @ 2020-08-05  8:12 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: catalin.marinas, Marc Zyngier, mark.rutland, anup, greentime.hu,
	linux-riscv, linux-kernel, aou, Paul Walmsley, syven.wang

On Tue, Aug 04, 2020 at 07:03:06PM -0700, Palmer Dabbelt wrote:
> > On Tue, Aug 4, 2020 at 8:32 AM Greentime Hu <greentime.hu@sifive.com> wrote:
> > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > index f4adb3684f3d..29b0f7108054 100644
> > > --- a/arch/riscv/mm/init.c
> > > +++ b/arch/riscv/mm/init.c
> > > @@ -202,12 +202,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
> > > 
> > >         ptep = &fixmap_pte[pte_index(addr)];
> > > 
> > > -       if (pgprot_val(prot)) {
> > > +       if (pgprot_val(prot))
> > >                 set_pte(ptep, pfn_pte(phys >> PAGE_SHIFT, prot));
> > > -       } else {
> > > +       else
> > >                 pte_clear(&init_mm, addr, ptep);
> > > -               local_flush_tlb_page(addr);
> > > -       }
> > > +       local_flush_tlb_page(addr);
> > >  }
> 
> arm64 appears to be upgrading all set_pte()s on valid kernel mappings to
> include the fence.  It looks like the message from 7f0b1bf04511 ("arm64: Fix
> barriers used for page table modifications") is out of date, as I can't find
> create_mapping() any more.  If that was some generic kernel thing then we
> should probably upgrade ours as well, but if it was arch/arm64/ code then this
> approach seems fine as __set_fixmap() isn't on the hot path -- I guess this is
> fine either way, but there may be other issues that the arm64 approach fixes.
> 
> Do you guys happen to remember what was going on here?

Basically, the architecture says we need these fences in order to guarantee
that the page-table walker sees the updated entry. Since we couldn't
tolerate spurious faults on kernel mappings, we had to add them (for
userspace we don't bother because in reality we don't tend to take the
spurious fault, and it's harmless if we do).

But having said all that, check out 42f91093b043 ("arm64: mm: Ignore spurious
translation faults taken from the kernel").

Will

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: Add sfence.vma after page table changed
@ 2020-08-05  8:12       ` Will Deacon
  0 siblings, 0 replies; 12+ messages in thread
From: Will Deacon @ 2020-08-05  8:12 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: mark.rutland, Marc Zyngier, aou, syven.wang, catalin.marinas,
	Paul Walmsley, linux-kernel, anup, greentime.hu, linux-riscv

On Tue, Aug 04, 2020 at 07:03:06PM -0700, Palmer Dabbelt wrote:
> > On Tue, Aug 4, 2020 at 8:32 AM Greentime Hu <greentime.hu@sifive.com> wrote:
> > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > index f4adb3684f3d..29b0f7108054 100644
> > > --- a/arch/riscv/mm/init.c
> > > +++ b/arch/riscv/mm/init.c
> > > @@ -202,12 +202,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
> > > 
> > >         ptep = &fixmap_pte[pte_index(addr)];
> > > 
> > > -       if (pgprot_val(prot)) {
> > > +       if (pgprot_val(prot))
> > >                 set_pte(ptep, pfn_pte(phys >> PAGE_SHIFT, prot));
> > > -       } else {
> > > +       else
> > >                 pte_clear(&init_mm, addr, ptep);
> > > -               local_flush_tlb_page(addr);
> > > -       }
> > > +       local_flush_tlb_page(addr);
> > >  }
> 
> arm64 appears to be upgrading all set_pte()s on valid kernel mappings to
> include the fence.  It looks like the message from 7f0b1bf04511 ("arm64: Fix
> barriers used for page table modifications") is out of date, as I can't find
> create_mapping() any more.  If that was some generic kernel thing then we
> should probably upgrade ours as well, but if it was arch/arm64/ code then this
> approach seems fine as __set_fixmap() isn't on the hot path -- I guess this is
> fine either way, but there may be other issues that the arm64 approach fixes.
> 
> Do you guys happen to remember what was going on here?

Basically, the architecture says we need these fences in order to guarantee
that the page-table walker sees the updated entry. Since we couldn't
tolerate spurious faults on kernel mappings, we had to add them (for
userspace we don't bother because in reality we don't tend to take the
spurious fault, and it's harmless if we do).

But having said all that, check out 42f91093b043 ("arm64: mm: Ignore spurious
translation faults taken from the kernel").

Will

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: Add sfence.vma after page table changed
  2020-08-05  2:03     ` Palmer Dabbelt
@ 2020-09-15  3:58       ` Greentime Hu
  -1 siblings, 0 replies; 12+ messages in thread
From: Greentime Hu @ 2020-09-15  3:58 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Catalin Marinas, Will Deacon, Marc Zyngier, Mark Rutland,
	Anup Patel, linux-riscv, Linux Kernel Mailing List, Albert Ou,
	Paul Walmsley, Syven Wang

Palmer Dabbelt <palmer@dabbelt.com> 於 2020年8月5日 週三 上午10:03寫道:
>
> On Mon, 03 Aug 2020 20:29:32 PDT (-0700), anup@brainfault.org wrote:
> > On Tue, Aug 4, 2020 at 8:32 AM Greentime Hu <greentime.hu@sifive.com> wrote:
> >>
> >> This patch addes local_flush_tlb_page(addr) to use sfence.vma after the
> >
> > s/addes/adds
> >
> >> page table changed. That address will be used immediately in
> >> memset(nextp, 0, PAGE_SIZE) to cause this issue so we should add the
> >> sfence.vma before we use it.
> >
> > Alternate version of this commit description can be:
> >
> > Invalidate local TLB after both set_pet() and clear_pte() because the
> > address can be used immediately after page table change.
> >
> >> Fixes: f2c17aabc917 ("RISC-V: Implement compile-time fixed mappings")
> >>
> >> Reported-by: Syven Wang <syven.wang@sifive.com>
> >> Signed-off-by: Syven Wang <syven.wang@sifive.com>
> >> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> >> ---
> >>  arch/riscv/mm/init.c | 7 +++----
> >>  1 file changed, 3 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> >> index f4adb3684f3d..29b0f7108054 100644
> >> --- a/arch/riscv/mm/init.c
> >> +++ b/arch/riscv/mm/init.c
> >> @@ -202,12 +202,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
> >>
> >>         ptep = &fixmap_pte[pte_index(addr)];
> >>
> >> -       if (pgprot_val(prot)) {
> >> +       if (pgprot_val(prot))
> >>                 set_pte(ptep, pfn_pte(phys >> PAGE_SHIFT, prot));
> >> -       } else {
> >> +       else
> >>                 pte_clear(&init_mm, addr, ptep);
> >> -               local_flush_tlb_page(addr);
> >> -       }
> >> +       local_flush_tlb_page(addr);
> >>  }
>
> arm64 appears to be upgrading all set_pte()s on valid kernel mappings to
> include the fence.  It looks like the message from 7f0b1bf04511 ("arm64: Fix
> barriers used for page table modifications") is out of date, as I can't find
> create_mapping() any more.  If that was some generic kernel thing then we
> should probably upgrade ours as well, but if it was arch/arm64/ code then this
> approach seems fine as __set_fixmap() isn't on the hot path -- I guess this is
> fine either way, but there may be other issues that the arm64 approach fixes.
>
> Do you guys happen to remember what was going on here?

Hi Palmer,

Some architectures add cache writeback in set_pte(), just like nds32, csky.

https://github.com/torvalds/linux/blob/master/arch/nds32/include/asm/pgtable.h#L213
https://github.com/torvalds/linux/blob/master/arch/csky/include/asm/pgtable.h#L104

Would you like to pick this patch or should I send another patch to
implement it in set_pte()?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: Add sfence.vma after page table changed
@ 2020-09-15  3:58       ` Greentime Hu
  0 siblings, 0 replies; 12+ messages in thread
From: Greentime Hu @ 2020-09-15  3:58 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Mark Rutland, Marc Zyngier, Albert Ou, Syven Wang, Will Deacon,
	Catalin Marinas, Paul Walmsley, Linux Kernel Mailing List,
	Anup Patel, linux-riscv

Palmer Dabbelt <palmer@dabbelt.com> 於 2020年8月5日 週三 上午10:03寫道:
>
> On Mon, 03 Aug 2020 20:29:32 PDT (-0700), anup@brainfault.org wrote:
> > On Tue, Aug 4, 2020 at 8:32 AM Greentime Hu <greentime.hu@sifive.com> wrote:
> >>
> >> This patch addes local_flush_tlb_page(addr) to use sfence.vma after the
> >
> > s/addes/adds
> >
> >> page table changed. That address will be used immediately in
> >> memset(nextp, 0, PAGE_SIZE) to cause this issue so we should add the
> >> sfence.vma before we use it.
> >
> > Alternate version of this commit description can be:
> >
> > Invalidate local TLB after both set_pet() and clear_pte() because the
> > address can be used immediately after page table change.
> >
> >> Fixes: f2c17aabc917 ("RISC-V: Implement compile-time fixed mappings")
> >>
> >> Reported-by: Syven Wang <syven.wang@sifive.com>
> >> Signed-off-by: Syven Wang <syven.wang@sifive.com>
> >> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> >> ---
> >>  arch/riscv/mm/init.c | 7 +++----
> >>  1 file changed, 3 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> >> index f4adb3684f3d..29b0f7108054 100644
> >> --- a/arch/riscv/mm/init.c
> >> +++ b/arch/riscv/mm/init.c
> >> @@ -202,12 +202,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
> >>
> >>         ptep = &fixmap_pte[pte_index(addr)];
> >>
> >> -       if (pgprot_val(prot)) {
> >> +       if (pgprot_val(prot))
> >>                 set_pte(ptep, pfn_pte(phys >> PAGE_SHIFT, prot));
> >> -       } else {
> >> +       else
> >>                 pte_clear(&init_mm, addr, ptep);
> >> -               local_flush_tlb_page(addr);
> >> -       }
> >> +       local_flush_tlb_page(addr);
> >>  }
>
> arm64 appears to be upgrading all set_pte()s on valid kernel mappings to
> include the fence.  It looks like the message from 7f0b1bf04511 ("arm64: Fix
> barriers used for page table modifications") is out of date, as I can't find
> create_mapping() any more.  If that was some generic kernel thing then we
> should probably upgrade ours as well, but if it was arch/arm64/ code then this
> approach seems fine as __set_fixmap() isn't on the hot path -- I guess this is
> fine either way, but there may be other issues that the arm64 approach fixes.
>
> Do you guys happen to remember what was going on here?

Hi Palmer,

Some architectures add cache writeback in set_pte(), just like nds32, csky.

https://github.com/torvalds/linux/blob/master/arch/nds32/include/asm/pgtable.h#L213
https://github.com/torvalds/linux/blob/master/arch/csky/include/asm/pgtable.h#L104

Would you like to pick this patch or should I send another patch to
implement it in set_pte()?

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: Add sfence.vma after page table changed
  2020-09-15  3:58       ` Greentime Hu
@ 2020-09-19  4:41         ` Palmer Dabbelt
  -1 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2020-09-19  4:41 UTC (permalink / raw)
  To: greentime.hu
  Cc: catalin.marinas, Will Deacon, Marc Zyngier, mark.rutland, anup,
	linux-riscv, linux-kernel, aou, Paul Walmsley, syven.wang

On Mon, 14 Sep 2020 20:58:13 PDT (-0700), greentime.hu@sifive.com wrote:
> Palmer Dabbelt <palmer@dabbelt.com> 於 2020年8月5日 週三 上午10:03寫道:
>>
>> On Mon, 03 Aug 2020 20:29:32 PDT (-0700), anup@brainfault.org wrote:
>> > On Tue, Aug 4, 2020 at 8:32 AM Greentime Hu <greentime.hu@sifive.com> wrote:
>> >>
>> >> This patch addes local_flush_tlb_page(addr) to use sfence.vma after the
>> >
>> > s/addes/adds
>> >
>> >> page table changed. That address will be used immediately in
>> >> memset(nextp, 0, PAGE_SIZE) to cause this issue so we should add the
>> >> sfence.vma before we use it.
>> >
>> > Alternate version of this commit description can be:
>> >
>> > Invalidate local TLB after both set_pet() and clear_pte() because the
>> > address can be used immediately after page table change.
>> >
>> >> Fixes: f2c17aabc917 ("RISC-V: Implement compile-time fixed mappings")
>> >>
>> >> Reported-by: Syven Wang <syven.wang@sifive.com>
>> >> Signed-off-by: Syven Wang <syven.wang@sifive.com>
>> >> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
>> >> ---
>> >>  arch/riscv/mm/init.c | 7 +++----
>> >>  1 file changed, 3 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>> >> index f4adb3684f3d..29b0f7108054 100644
>> >> --- a/arch/riscv/mm/init.c
>> >> +++ b/arch/riscv/mm/init.c
>> >> @@ -202,12 +202,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
>> >>
>> >>         ptep = &fixmap_pte[pte_index(addr)];
>> >>
>> >> -       if (pgprot_val(prot)) {
>> >> +       if (pgprot_val(prot))
>> >>                 set_pte(ptep, pfn_pte(phys >> PAGE_SHIFT, prot));
>> >> -       } else {
>> >> +       else
>> >>                 pte_clear(&init_mm, addr, ptep);
>> >> -               local_flush_tlb_page(addr);
>> >> -       }
>> >> +       local_flush_tlb_page(addr);
>> >>  }
>>
>> arm64 appears to be upgrading all set_pte()s on valid kernel mappings to
>> include the fence.  It looks like the message from 7f0b1bf04511 ("arm64: Fix
>> barriers used for page table modifications") is out of date, as I can't find
>> create_mapping() any more.  If that was some generic kernel thing then we
>> should probably upgrade ours as well, but if it was arch/arm64/ code then this
>> approach seems fine as __set_fixmap() isn't on the hot path -- I guess this is
>> fine either way, but there may be other issues that the arm64 approach fixes.
>>
>> Do you guys happen to remember what was going on here?
>
> Hi Palmer,
>
> Some architectures add cache writeback in set_pte(), just like nds32, csky.
>
> https://github.com/torvalds/linux/blob/master/arch/nds32/include/asm/pgtable.h#L213
> https://github.com/torvalds/linux/blob/master/arch/csky/include/asm/pgtable.h#L104
>
> Would you like to pick this patch or should I send another patch to
> implement it in set_pte()?

Sorry, I forgot about this.  I'm picking up this patch on fixes, as it clearly
fixes a bug.  We might also have uncovered another bug, but there's no sense in
letting this one hang around until we figure that out.

I've cleaned up the comment text a bit, as Anup pointed out some issuses.

Thanks!

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] riscv: Add sfence.vma after page table changed
@ 2020-09-19  4:41         ` Palmer Dabbelt
  0 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2020-09-19  4:41 UTC (permalink / raw)
  To: greentime.hu
  Cc: mark.rutland, Marc Zyngier, aou, syven.wang, Will Deacon,
	catalin.marinas, Paul Walmsley, linux-kernel, anup, linux-riscv

On Mon, 14 Sep 2020 20:58:13 PDT (-0700), greentime.hu@sifive.com wrote:
> Palmer Dabbelt <palmer@dabbelt.com> 於 2020年8月5日 週三 上午10:03寫道:
>>
>> On Mon, 03 Aug 2020 20:29:32 PDT (-0700), anup@brainfault.org wrote:
>> > On Tue, Aug 4, 2020 at 8:32 AM Greentime Hu <greentime.hu@sifive.com> wrote:
>> >>
>> >> This patch addes local_flush_tlb_page(addr) to use sfence.vma after the
>> >
>> > s/addes/adds
>> >
>> >> page table changed. That address will be used immediately in
>> >> memset(nextp, 0, PAGE_SIZE) to cause this issue so we should add the
>> >> sfence.vma before we use it.
>> >
>> > Alternate version of this commit description can be:
>> >
>> > Invalidate local TLB after both set_pet() and clear_pte() because the
>> > address can be used immediately after page table change.
>> >
>> >> Fixes: f2c17aabc917 ("RISC-V: Implement compile-time fixed mappings")
>> >>
>> >> Reported-by: Syven Wang <syven.wang@sifive.com>
>> >> Signed-off-by: Syven Wang <syven.wang@sifive.com>
>> >> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
>> >> ---
>> >>  arch/riscv/mm/init.c | 7 +++----
>> >>  1 file changed, 3 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>> >> index f4adb3684f3d..29b0f7108054 100644
>> >> --- a/arch/riscv/mm/init.c
>> >> +++ b/arch/riscv/mm/init.c
>> >> @@ -202,12 +202,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
>> >>
>> >>         ptep = &fixmap_pte[pte_index(addr)];
>> >>
>> >> -       if (pgprot_val(prot)) {
>> >> +       if (pgprot_val(prot))
>> >>                 set_pte(ptep, pfn_pte(phys >> PAGE_SHIFT, prot));
>> >> -       } else {
>> >> +       else
>> >>                 pte_clear(&init_mm, addr, ptep);
>> >> -               local_flush_tlb_page(addr);
>> >> -       }
>> >> +       local_flush_tlb_page(addr);
>> >>  }
>>
>> arm64 appears to be upgrading all set_pte()s on valid kernel mappings to
>> include the fence.  It looks like the message from 7f0b1bf04511 ("arm64: Fix
>> barriers used for page table modifications") is out of date, as I can't find
>> create_mapping() any more.  If that was some generic kernel thing then we
>> should probably upgrade ours as well, but if it was arch/arm64/ code then this
>> approach seems fine as __set_fixmap() isn't on the hot path -- I guess this is
>> fine either way, but there may be other issues that the arm64 approach fixes.
>>
>> Do you guys happen to remember what was going on here?
>
> Hi Palmer,
>
> Some architectures add cache writeback in set_pte(), just like nds32, csky.
>
> https://github.com/torvalds/linux/blob/master/arch/nds32/include/asm/pgtable.h#L213
> https://github.com/torvalds/linux/blob/master/arch/csky/include/asm/pgtable.h#L104
>
> Would you like to pick this patch or should I send another patch to
> implement it in set_pte()?

Sorry, I forgot about this.  I'm picking up this patch on fixes, as it clearly
fixes a bug.  We might also have uncovered another bug, but there's no sense in
letting this one hang around until we figure that out.

I've cleaned up the comment text a bit, as Anup pointed out some issuses.

Thanks!

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-09-19  4:41 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-04  3:02 [PATCH] riscv: Add sfence.vma after page table changed Greentime Hu
2020-08-04  3:02 ` Greentime Hu
2020-08-04  3:29 ` Anup Patel
2020-08-04  3:29   ` Anup Patel
2020-08-05  2:03   ` Palmer Dabbelt
2020-08-05  2:03     ` Palmer Dabbelt
2020-08-05  8:12     ` Will Deacon
2020-08-05  8:12       ` Will Deacon
2020-09-15  3:58     ` Greentime Hu
2020-09-15  3:58       ` Greentime Hu
2020-09-19  4:41       ` Palmer Dabbelt
2020-09-19  4:41         ` Palmer Dabbelt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.