Re: [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs

[Alistair Francis]

On Wed, Mar 18, 2020 at 9:52 PM Palmer Dabbelt <address@hidden> wrote:
>
> On Tue, 03 Mar 2020 17:16:59 PST (-0800), Alistair Francis wrote:
> > 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>
> > ---
> >  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 5ea5d133aa..cc9f20b471 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;
>
> I remember having seen this patch before and having some objections, but I 
> feel
> like I mistakenly had this backwards before or something because it makes 
> sense
> now.

Ha, we have come full circle because I think this is wrong.

This is an optimisation which from what I can tell (and talking to
Richard) is correct.

In saying that this patch is the only thing that I have found that
fixes Hypervisor guest userspace. It shouldn't be applied though.

Alistair

>
> Thanks!