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

Weiwei Li posted 6 patches 1 year ago
Failed in applying to current master (apply log)
There is a newer version of this series
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(-)
[PATCH 0/6] target/riscv: Fix PMP related problem
Posted by Weiwei Li 1 year ago
This patchset tries to fix the PMP bypass problem issue https://gitlab.com/qemu-project/qemu/-/issues/1542

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

-- 
2.25.1
Re: [PATCH 0/6] target/riscv: Fix PMP related problem
Posted by LIU Zhiwei 1 year ago
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

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(-)
>
Re: [PATCH 0/6] target/riscv: Fix PMP related problem
Posted by Weiwei Li 1 year ago
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.  So the other address access  may hit the TLB(if first access of 
the page didn't hit the PMP entry)

and bypass the pmp check. This is fixed by patch 1.

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


Re: [PATCH 0/6] target/riscv: Fix PMP related problem
Posted by LIU Zhiwei 1 year ago
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(-)
>>>

Re: [PATCH 0/6] target/riscv: Fix PMP related problem
Posted by Weiwei Li 1 year ago
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(-)
>>>>