[PATCH 0/1] -----BEGIN SSH SIGNATURE-----

[PATCH 0/1] -----BEGIN SSH SIGNATURE-----

[julia]

fY3jfRbpdO9l1l2wwAAAADZ2l0AAAAAAAAAAZzaGE1MTIAAABTAAAAC3NzaC1lZDI1NTE5
AAAAQP6c2B82m4kq6h046Ou/LV6c9I/D/uUtUlivmbvR/lSdCWOiPIYnpK5HPtvhcgVYoQ
8X1k8kKjplch4iy6JnNgU=
-----END SSH SIGNATURE-----

julia (1):
  target/riscv: log guest errors when reserved bits are set in PTEs

 target/riscv/cpu_helper.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

-- 
2.47.0

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

[julia]

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>
---
 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);
                 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;
     }
 
-- 
2.47.0

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;
>       }
>   

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

[Julia]

> 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:
>

I've sent a follow-up patch fixing these build errors, it builds on 32 & 64 bit on my system. Cheers 

As for the Signed-off-by, I'd rather not, and the contributing guide does not require it.

Regards,
Julia

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

[Alex Bennée]

Julia <midnight@trainwit.ch> writes:

>> 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:
>>
>
> I've sent a follow-up patch fixing these build errors, it builds on 32 & 64 bit on my system. Cheers 
>
> As for the Signed-off-by, I'd rather not, and the contributing guide
> does not require it.

It absolutely does, as listed in the minimal checklist:

  https://qemu.readthedocs.io/en/master/devel/submitting-a-patch.html#id30

>
> Regards,
> Julia

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Alex Bennée]

Julia <midnight@trainwit.ch> writes:

>> 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:
>>
>
> I've sent a follow-up patch fixing these build errors, it builds on 32 & 64 bit on my system. Cheers 
>
> As for the Signed-off-by, I'd rather not, and the contributing guide
> does not require it.

Apologies - I missed the context.

  The name used with “Signed-off-by” does not need to be your legal name,
  nor birth name, nor appear on any government ID. It is the identity you
  choose to be known by in the community, but should not be anonymous, nor
  misrepresent whom you are.

>
> Regards,
> Julia

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Daniel Henrique Barboza]

On 2/3/25 11:15 AM, Alex Bennée wrote:
> Julia <midnight@trainwit.ch> writes:
> 
>>> 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:
>>>
>>
>> I've sent a follow-up patch fixing these build errors, it builds on 32 & 64 bit on my system. Cheers
>>
>> As for the Signed-off-by, I'd rather not, and the contributing guide
>> does not require it.
> 
> Apologies - I missed the context.
> 
>    The name used with “Signed-off-by” does not need to be your legal name,
>    nor birth name, nor appear on any government ID. It is the identity you
>    choose to be known by in the community, but should not be anonymous, nor
>    misrepresent whom you are.

That's correct.

The reason I mentioned about the "git" commands in my reply is that newcomers
aren't necessarily well versed with git and creating a patch without doing
a prior setup will, in Linux, use login_name@hostname as author. I saw the
Signed-off-by of this patch and I figured that could be the case. Apparently
I was wrong and the git identity used is intention, so we're good.

By the way I wonder if we could amend docs/devel/submitting-a-patch.rst with
a handful of lines about how to set author in git. We have docs about how to
send git send-mail, format-patch and so on, but apparently nothing about how
to set the author.


Thanks,

Daniel

> 
>>
>> Regards,
>> Julia
>