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

[Richard Henderson <richard.henderson@linaro.org>]

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~