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

[Alistair Francis]

On Mon, Jan 6, 2020 at 9:51 AM Palmer Dabbelt <address@hidden> wrote:
>
> On Mon, 09 Dec 2019 10:10:45 PST (-0800), Alistair Francis wrote:
> > 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 767c8762ac..0de3a468eb 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -344,10 +344,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;
>
> This is at least the second time I've spent a day or two trying to figure out
> what the right thing to do here is, and once again I'm lost.  I think what's
> really getting me is the original comment: why would this cause us to TLB 
> miss,
> wouldn't it cause us to not TLB miss?
>
> Assuming that's the case, it seems to me more like there's some missing fence
> in whatever program is blowing up due to this -- maybe it's just reading from
> the page before marking it as read-only, then relying on writes to trap 
> without
> doing the requisite fence.

Ok, let's drop this for now then. I'll reinvestigate and if this is
still needed I'll add a more detailed explanation.

Alistair