Re: [PATCH] riscv: Add sfence.vma after page table changed

From: Palmer Dabbelt
Date: Sat Sep 19 2020 - 00:43:06 EST


On Mon, 14 Sep 2020 20:58:13 PDT (-0700), greentime.hu@xxxxxxxxxx wrote:
Palmer Dabbelt <palmer@xxxxxxxxxxx> 於 2020年8月5日 週三 上午10:03寫道:

On Mon, 03 Aug 2020 20:29:32 PDT (-0700), anup@xxxxxxxxxxxxxx wrote:
> On Tue, Aug 4, 2020 at 8:32 AM Greentime Hu <greentime.hu@xxxxxxxxxx> 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@xxxxxxxxxx>
>> Signed-off-by: Syven Wang <syven.wang@xxxxxxxxxx>
>> Signed-off-by: Greentime Hu <greentime.hu@xxxxxxxxxx>
>> ---
>> 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!