On Wed, Mar 18, 2020 at 9:52 PM Palmer Dabbelt <pal...@dabbelt.com> 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 <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 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!
