| Message ID | 20250202051505.1846933-2-midnight@trainwit.ch (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | -----BEGIN SSH SIGNATURE----- | expand |
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; > } >
> 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
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
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
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 >
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; }
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(-)