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

[Weiwei Li <liweiwei@iscas.ac.cn>]

On 2023/4/18 12:47, LIU Zhiwei wrote:
>
> 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.

I have given an example for this bypass in the replied email of patch 1.

Regards,

Weiwei Li

>>
>> 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(-)
>>>>