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 <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 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;
-- 
2.24.0

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 <alistair.francis@wdc.com>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>  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.

On Mon, Jan 6, 2020 at 9:51 AM Palmer Dabbelt <palmerdabbelt@google.com> 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 <alistair.francis@wdc.com>
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >  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