Re: [PATCH 1/1] target/riscv: log guest errors when reserved bits are set in PTEs

[Daniel Henrique Barboza]

On 2/2/25 2:15 AM, julia wrote:
> For instance, QEMUs newer than b6ecc63c569bb88c0fcadf79fb92bf4b88aefea8
> would silently treat this akin to an unmapped page (as required by the
> RISC-V spec, admittedly). However, not all hardware platforms do (e.g.
> CVA6) which leads to an apparent QEMU bug.
>
> Instead, log a guest error so that in future, incorrectly set up page
> tables can be debugged without bisecting QEMU.
>
> Signed-off-by: julia <midnight@trainwit.ch>

Usually the author line consists of a full name. You can set the author name by
using 'git config --global user.name <full_name>'. To amend an existing patch
you can use:

git commit --amend --author="Full Author Name <author@email>"

> ---
>   target/riscv/cpu_helper.c | 27 ++++++++++++++++++++++++++-
>   1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index e1dfc4ecbf..87adf16a49 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1226,14 +1226,27 @@ restart:
>               ppn = pte >> PTE_PPN_SHIFT;
>           } else {
>               if (pte & PTE_RESERVED) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: reserved bits set in PTE: "
> +                              "addr: 0x%" HWADDR_PRIx " pte: 0x%" PRIx64 "\n",
> +                              __func__, pte_addr, pte);


This will fail to compile for riscv32 (riscv32-softmmu configure target) with 
errors
like this:

../target/riscv/cpu_helper.c: In function ‘get_physical_address’:
../target/riscv/cpu_helper.c:1230:48: error: format ‘%lx’ expects argument of 
type ‘long unsigned int’, but argument 4 has type ‘target_ulong’ {aka ‘unsigned 
int’} [-Werror=format=]
 1230 |                 qemu_log_mask(LOG_GUEST_ERROR, "%s: reserved bits set in 
PTE: "
      |                                                
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1231 |                               "addr: 0x%" HWADDR_PRIx " pte: 0x%" PRIx64 
"\n",
 1232 |                               __func__, pte_addr, pte);
      |                                                   ~~~
      |                                                   |
      |                                                   target_ulong {aka 
unsigned int}
/home/danielhb/work/qemu/include/qemu/log.h:57:22: note: in definition of macro 
‘qemu_log_mask’
   57 |             qemu_log(FMT, ## __VA_ARGS__);              \
      |                      ^~~

This happens because 'pte' is a 'target_ulong' type that, for riscv32, will be
interpreted as uint32_t while the FMT being used is PRIx64.

You can fix it by using TARGET_FMT_lx instead of PRIx64:

+++ b/target/riscv/cpu_helper.c
@@ -1228,7 +1228,7 @@ restart:
         } else {
             if (pte & PTE_RESERVED) {
                 qemu_log_mask(LOG_GUEST_ERROR, "%s: reserved bits set in PTE: "
-                              "addr: 0x%" HWADDR_PRIx " pte: 0x%" PRIx64 "\n",
+                              "addr: 0x%" HWADDR_PRIx " pte:" TARGET_FMT_lx 
"\n",
                               __func__, pte_addr, pte);
                 return TRANSLATE_FAIL;


This change is needed in all qemu_log_mask() entries added. Thanks,


Daniel



>                   return TRANSLATE_FAIL;
>               }
>   
>               if (!pbmte && (pte & PTE_PBMT)) {
> +                /* Reserved without Svpbmt. */
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: PBMT bits set in PTE, "
> +                              "and Svpbmt extension is disabled: "
> +                              "addr: 0x%" HWADDR_PRIx " pte: 0x%" PRIx64 "\n",
> +                              __func__, pte_addr, pte);
>                   return TRANSLATE_FAIL;
>               }
>   
>               if (!riscv_cpu_cfg(env)->ext_svnapot && (pte & PTE_N)) {
> +                /* Reserved without Svnapot extension */
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: N bit set in PTE, "
> +                              "and Svnapot extension is disabled: "
> +                              "addr: 0x%" HWADDR_PRIx " pte: 0x%" PRIx64 "\n",
> +                              __func__, pte_addr, pte);
>                   return TRANSLATE_FAIL;
>               }
>   
> @@ -1244,14 +1257,19 @@ restart:
>               /* Invalid PTE */
>               return TRANSLATE_FAIL;
>           }
> +
>           if (pte & (PTE_R | PTE_W | PTE_X)) {
>               goto leaf;
>           }
>   
> -        /* Inner PTE, continue walking */
>           if (pte & (PTE_D | PTE_A | PTE_U | PTE_ATTR)) {
> +            /* D, A, and U bits are reserved in non-leaf/inner PTEs */
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: D, A, or U bits set in non-leaf 
> PTE: "
> +                          "addr: 0x%" HWADDR_PRIx " pte: 0x%" PRIx64 "\n",
> +                          __func__, pte_addr, pte);
>               return TRANSLATE_FAIL;
>           }
> +        /* Inner PTE, continue walking */
>           base = ppn << PGSHIFT;
>       }
>   
> @@ -1261,10 +1279,17 @@ restart:
>    leaf:
>       if (ppn & ((1ULL << ptshift) - 1)) {
>           /* Misaligned PPN */
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: PPN bits in PTE is misaligned: "
> +                      "addr: 0x%" HWADDR_PRIx " pte: 0x%" PRIx64 "\n",
> +                      __func__, pte_addr, pte);
>           return TRANSLATE_FAIL;
>       }
>       if (!pbmte && (pte & PTE_PBMT)) {
>           /* Reserved without Svpbmt. */
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: PBMT bits set in PTE, "
> +                      "and Svpbmt extension is disabled: "
> +                      "addr: 0x%" HWADDR_PRIx " pte: 0x%" PRIx64 "\n",
> +                      __func__, pte_addr, pte);
>           return TRANSLATE_FAIL;
>       }
>