On Mon, Jan 6, 2020 at 9:51 AM Palmer Dabbelt <palmerdabb...@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.fran...@wdc.com>
> > Reviewed-by: Bin Meng <bmeng...@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
