Re: [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled

[Richard Henderson]

On 4/17/23 18:25, Daniel Henrique Barboza wrote:
> On 4/13/23 06:01, Weiwei Li wrote:
> > When PMP entry overlap part of the page, we'll set the tlb_size to 1, and
> > this will make the address set with TLB_INVALID_MASK to make the page
> > un-cached. However, if we clear TLB_INVALID_MASK when TLB is re-filled, then
> > the TLB host address will be cached, and the following instructions can use
> > this host address directly which may lead to the bypass of PMP related check.
> >
> > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> > ---
>
> For this commit I believe it's worth mentioning that it's partially reverting
> commit c3c8bf579b431b6b ("accel/tcg: Suppress auto-invalidate in
> probe_access_internal") that was made to handle a particularity/quirk that was
> present in s390x code.
>
> At first glance this patch seems benign but we must make sure that no other
> assumptions were made with this particular change in probe_access_internal().
>
>
> Thanks,
>
> Daniel
>
> >   accel/tcg/cputlb.c | 7 -------
> >   1 file changed, 7 deletions(-)
> >
> > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> > index e984a98dc4..d0bf996405 100644
> > --- a/accel/tcg/cputlb.c
> > +++ b/accel/tcg/cputlb.c
> > @@ -1563,13 +1563,6 @@ static int probe_access_internal(CPUArchState *env, target_ulong 
> > addr,
> >               /* TLB resize via tlb_fill may have moved the entry.  */
> >               index = tlb_index(env, mmu_idx, addr);
> >               entry = tlb_entry(env, mmu_idx, addr);
> > -
> > -            /*
> > -             * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately,
> > -             * to force the next access through tlb_fill.  We've just
> > -             * called tlb_fill, so we know that this entry *is* valid.
> > -             */
> > -            flags &= ~TLB_INVALID_MASK;


I missed the original patch, but this is definitely wrong.

Clearing this bit locally (!) is correct because we want to inform the caller of 
probe_access_* that the access is valid.  We know that it is valid because we have just 
queried tlb_fill (and thus for riscv, PMP).

Clearing the bit locally does *not* cause the tlb entry to be cached -- the INVALID bit is 
still set within the tlb entry.  The next access will again go through tlb_fill.

What is the original problem you are seeing?  The commit message does not say.


r~