Re: [PATCH 0/6] target/riscv: Fix PMP related problem

[LIU Zhiwei]

On 2023/4/18 11:36, Weiwei Li wrote:
> On 2023/4/18 11:07, LIU Zhiwei wrote:
> > On 2023/4/13 17:01, Weiwei Li wrote:
> > > This patchset tries to fix the PMP bypass problem issue 
> > > https://gitlab.com/qemu-project/qemu/-/issues/1542
> >
> > Please add your analysis of this issue here.
> >
> > By the way, I think this problem is introduced by
> >
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg939331.html
>
> It seems have no relationship with this commit.
>
> I think there are several problems for this issue:
>
> 1. TLB will not be cached only when the access address have matched 
> PMP entry. 
TLB will be filled only when PMP check and PTW check pass.
> So the other address access  may hit the TLB(if first access of the 
> page didn't hit the PMP entry)
This page will not be filled to TLB if the first access of the page 
didn't pass the PMP check.
> and bypass the pmp check. This is fixed by patch 1.

Never it should be.

Zhiwei

> 2. Writing to pmpaddr  didn't trigger tlb flush. This is fixed by 
> patch 3.
>
> 3. The tb isn't flushed when PMP permission changes, so It also may 
> hit the tb and bypass the changed PMP check for instruction fetch. 
> This is fixed by patch 5.
>
> 4. We set the tlb_size to 1 to make the TLB_INVALID_MASK set. However 
> this flag will be cleared after fill_tlb, and this will make the host 
> address be cached, and let the following instruction fetch in the same 
> tb bypass the PMP check. This is fixed by patch 6.
>
> Regards,
>
> Weiwei Li
>
> > I have commented on how to correct this patch. But by accident, it 
> > has been merged.
> >
> > Zhiwei
> >
> > > The port is available here:
> > > https://github.com/plctlab/plct-qemu/tree/plct-pmp-fix
> > >
> > > Weiwei Li (6):
> > >    target/riscv: Update pmp_get_tlb_size()
> > >    target/riscv: Move pmp_get_tlb_size apart from
> > >      get_physical_address_pmp
> > >    target/riscv: flush tlb when pmpaddr is updated
> > >    target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
> > >    target/riscv: flush tb when PMP entry changes
> > >    accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is
> > >      re-filled
> > >
> > >   accel/tcg/cputlb.c        |  7 -----
> > >   target/riscv/cpu_helper.c | 19 ++++---------
> > >   target/riscv/pmp.c        | 60 
> > > ++++++++++++++++++++++++++-------------
> > >   target/riscv/pmp.h        |  3 +-
> > >   4 files changed, 47 insertions(+), 42 deletions(-)