[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 1/6] target/riscv: Don't set write permissions on dirty PTEs
From: |
Alistair Francis |
Subject: |
Re: [PULL 1/6] target/riscv: Don't set write permissions on dirty PTEs |
Date: |
Tue, 21 Apr 2020 12:20:58 -0700 |
On Tue, Apr 21, 2020 at 12:19 PM Palmer Dabbelt
<address@hidden> wrote:
>
> From: Alistair Francis <address@hidden>
>
> The RISC-V spec specifies that when a write happens and the D bit is
> clear the implementation will set the bit in the PTE. It does not
> describe that the PTE being dirty means that we should provide write
> access. This patch removes the write access granted to pages when the
> dirty bit is set.
>
> Following the prot variable we can see that it affects all of these
> functions:
> riscv_cpu_tlb_fill()
> tlb_set_page()
> tlb_set_page_with_attrs()
> address_space_translate_for_iotlb()
>
> Looking at the cputlb code (tlb_set_page_with_attrs() and
> address_space_translate_for_iotlb()) it looks like the main affect of
> setting write permissions is that the page can be marked as TLB_NOTDIRTY.
>
> I don't see any other impacts (related to the dirty bit) for giving a
> page write permissions.
>
> Setting write permission on dirty PTEs results in userspace inside a
> Hypervisor guest (VU) becoming corrupted. This appears to be because it
> ends up with write permission in the second stage translation in cases
> where we aren't doing a store.
>
> Signed-off-by: Alistair Francis <address@hidden>
> Reviewed-by: Bin Meng <address@hidden>
> Signed-off-by: Palmer Dabbelt <address@hidden>
This commit is wrong. Please do not apply this.
Alistair
> ---
> target/riscv/cpu_helper.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index d3ba9efb02..e2da2a4787 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -572,10 +572,8 @@ restart:
> if ((pte & PTE_X)) {
> *prot |= PAGE_EXEC;
> }
> - /* add write permission on stores or if the page is already
> dirty,
> - so that we TLB miss on later writes to update the dirty bit */
> - if ((pte & PTE_W) &&
> - (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
> + /* add write permission on stores */
> + if ((pte & PTE_W) && (access_type == MMU_DATA_STORE)) {
> *prot |= PAGE_WRITE;
> }
> return TRANSLATE_SUCCESS;
> --
> 2.26.1.301.g55bc3eb7cb9-goog
>
>
- [PULL] RISC-V Patches for 5.0-rc4, Palmer Dabbelt, 2020/04/21
- [PULL 1/6] target/riscv: Don't set write permissions on dirty PTEs, Palmer Dabbelt, 2020/04/21
- Re: [PULL 1/6] target/riscv: Don't set write permissions on dirty PTEs,
Alistair Francis <=
- [PULL 2/6] riscv: Don't use stage-2 PTE lookup protection flags, Palmer Dabbelt, 2020/04/21
- [PULL 4/6] riscv/sifive_u: Fix up file ordering, Palmer Dabbelt, 2020/04/21
- [PULL 6/6] riscv/sifive_u: Add a serial property to the sifive_u machine, Palmer Dabbelt, 2020/04/21
- [PULL 3/6] riscv: AND stage-1 and stage-2 protection flags, Palmer Dabbelt, 2020/04/21
- [PULL 5/6] riscv/sifive_u: Add a serial property to the sifive_u SoC, Palmer Dabbelt, 2020/04/21
- Re: [PULL] RISC-V Patches for 5.0-rc4, Peter Maydell, 2020/04/21