[v2] target/riscv: raise access fault for Zicbom MMIO-like accesses

[PATCH v2] target/riscv: raise access fault for Zicbom MMIO-like accesses

Posted by ZhengXiang Qin 1 month, 1 week ago

check_zicbom_access() currently treats any probe_access_flags() result
other than TLB_INVALID_MASK as a successful access. However, a result
with TLB_MMIO means that the target is MMIO-like and should not be
treated as a normal cache block management target.

Raise a store/AMO access fault for Zicbom accesses to MMIO-like regions
so that cbo.clean, cbo.flush and cbo.inval do not silently succeed on
non-cacheable/MMIO-like memory.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3501
Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
Reviewed-by: Chao Liu <chao.liu.zevorn@gmail.com>
Signed-off-by: ZhengXiang Qin <qinzhengxiang@foxmail.com>
---
Changes since v1:
- Use a bit test for TLB_MMIO since probe_access_flags() returns a bitmask.
- Keep Reviewed-by tags from v1.

 target/riscv/op_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 81873014cb..f23b060585 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -218,6 +218,7 @@ static void check_zicbom_access(CPURISCVState *env,
     void *phost;
     int ret;
 
+    target_ulong fault_addr = address;
     /* Mask off low-bits to align-down to the cache-block. */
     address &= ~(cbomlen - 1);
 
@@ -235,6 +236,10 @@ static void check_zicbom_access(CPURISCVState *env,
      */
     ret = probe_access_flags(env, address, cbomlen, MMU_DATA_LOAD,
                              mmu_idx, true, &phost, ra);
+    if (ret & TLB_MMIO) {
+        env->badaddr = fault_addr;
+        riscv_raise_exception(env, RISCV_EXCP_STORE_AMO_ACCESS_FAULT, ra);
+    }
     if (ret != TLB_INVALID_MASK) {
         /* Success: readable */
         return;
-- 
2.43.0

Re: [PATCH v2] target/riscv: raise access fault for Zicbom MMIO-like accesses

Posted by Philippe Mathieu-Daudé 1 month, 1 week ago

On 18/6/26 16:06, ZhengXiang Qin wrote:
> check_zicbom_access() currently treats any probe_access_flags() result
> other than TLB_INVALID_MASK as a successful access. However, a result
> with TLB_MMIO means that the target is MMIO-like and should not be
> treated as a normal cache block management target.
> 
> Raise a store/AMO access fault for Zicbom accesses to MMIO-like regions
> so that cbo.clean, cbo.flush and cbo.inval do not silently succeed on
> non-cacheable/MMIO-like memory.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3501
> Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
> Reviewed-by: Chao Liu <chao.liu.zevorn@gmail.com>
> Signed-off-by: ZhengXiang Qin <qinzhengxiang@foxmail.com>
> ---
> Changes since v1:
> - Use a bit test for TLB_MMIO since probe_access_flags() returns a bitmask.
> - Keep Reviewed-by tags from v1.
> 
>   target/riscv/op_helper.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 81873014cb..f23b060585 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -218,6 +218,7 @@ static void check_zicbom_access(CPURISCVState *env,
>       void *phost;
>       int ret;
>   
> +    target_ulong fault_addr = address;

CPURISCVState::badaddr is of type uint64_t, can we use that here instead
of target_ulong?

>       /* Mask off low-bits to align-down to the cache-block. */
>       address &= ~(cbomlen - 1);
>   
> @@ -235,6 +236,10 @@ static void check_zicbom_access(CPURISCVState *env,
>        */
>       ret = probe_access_flags(env, address, cbomlen, MMU_DATA_LOAD,
>                                mmu_idx, true, &phost, ra);
> +    if (ret & TLB_MMIO) {
> +        env->badaddr = fault_addr;
> +        riscv_raise_exception(env, RISCV_EXCP_STORE_AMO_ACCESS_FAULT, ra);
> +    }

Maybe adding 'else' make this code path more readable.

>       if (ret != TLB_INVALID_MASK) {
>           /* Success: readable */
>           return;

Feel free to ignore these nitpicking comments.

Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>