* [PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode @ 2026-08-07 9:17 sesame_h 2026-08-11 13:17 ` Chinmay Rath 0 siblings, 1 reply; 5+ messages in thread From: sesame_h @ 2026-08-07 9:17 UTC (permalink / raw) To: qemu-devel Cc: qemu-ppc, npiggin, adityag, milesg, harshpb, rathc, Minhang Zhang From: Minhang Zhang <zhangminhang@kylinos.cn> ppc_store_sdr1() had validation for 64-bit SDR1 values but lacked corresponding checks for the 32-bit case. According to the Power ISA, in 32-bit mode SDR1 bits 16-22 are reserved (must be zero) and HTABMASK (bits 23-31) must consist of a consecutive string of 1-bits starting from the LSB, i.e., be of the form 2^n-1. Add checks to reject invalid HTABMASK values and log a guest error for non-zero reserved bits, following the same pattern used by the existing 64-bit validation. Signed-off-by: Minhang Zhang <zhangminhang@kylinos.cn> --- target/ppc/mmu_common.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c index 2499e61..59e8324 100644 --- a/target/ppc/mmu_common.c +++ b/target/ppc/mmu_common.c @@ -57,9 +57,23 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value) " stored in SDR1", htabsize); return; } - } + } else #endif /* defined(TARGET_PPC64) */ - /* FIXME: Should check for valid HTABMASK values in 32-bit case */ + { + target_ulong htabmask = value & SDR_32_HTABMASK; + if (value & 0x007F0000UL) { + qemu_log_mask(LOG_GUEST_ERROR, + "Invalid SDR1: reserved bits 0x" TARGET_FMT_lx + " set\n", value & 0x007F0000UL); + value &= ~0x007F0000UL; + } + if ((htabmask & (htabmask + 1)) != 0) { + qemu_log_mask(LOG_GUEST_ERROR, + "Invalid HTABMASK 0x" TARGET_FMT_lx + " in SDR1 (must be of form 2^n-1)\n", htabmask); + return; + } + } env->spr[SPR_SDR1] = value; } -- 2.43.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode 2026-08-07 9:17 [PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode sesame_h @ 2026-08-11 13:17 ` Chinmay Rath 2026-08-11 13:21 ` Chinmay Rath 0 siblings, 1 reply; 5+ messages in thread From: Chinmay Rath @ 2026-08-11 13:17 UTC (permalink / raw) To: sesame_h, qemu-devel Cc: qemu-ppc, npiggin, adityag, milesg, harshpb, Minhang Zhang Hi Minhang, Thanks for fixing this. Took me a while to get hold of the docs for 32 bit mmu, hence the late response. On 8/7/26 14:47, sesame_h@qq.com wrote: > From: Minhang Zhang <zhangminhang@kylinos.cn> > > ppc_store_sdr1() had validation for 64-bit SDR1 values but lacked > corresponding checks for the 32-bit case. According to the Power ISA, > in 32-bit mode SDR1 bits 16-22 are reserved (must be zero) and > HTABMASK (bits 23-31) must consist of a consecutive string of > 1-bits starting from the LSB, i.e., be of the form 2^n-1. > > Add checks to reject invalid HTABMASK values and log a guest error > for non-zero reserved bits, following the same pattern used by the > existing 64-bit validation. > > Signed-off-by: Minhang Zhang <zhangminhang@kylinos.cn> > --- > target/ppc/mmu_common.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c > index 2499e61..59e8324 100644 > --- a/target/ppc/mmu_common.c > +++ b/target/ppc/mmu_common.c > @@ -57,9 +57,23 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value) > " stored in SDR1", htabsize); > return; > } > - } > + } else > #endif /* defined(TARGET_PPC64) */ > - /* FIXME: Should check for valid HTABMASK values in 32-bit case */ > + { > + target_ulong htabmask = value & SDR_32_HTABMASK; > + if (value & 0x007F0000UL) { From what I see in the programming manual, shouldn't the bits of this flag be the other way around : 0x0000FE00 ? Nevertheless, it would be better to form the mask by ORing the SDR_32_HTABORG and SDR_32_HTABMASK fields from mmu-hash32.h, instead of hardcoding it; the way it is already done for 64 bits in the same function. Regards, Chinmay > + qemu_log_mask(LOG_GUEST_ERROR, > + "Invalid SDR1: reserved bits 0x" TARGET_FMT_lx > + " set\n", value & 0x007F0000UL); > + value &= ~0x007F0000UL; > + } > + if ((htabmask & (htabmask + 1)) != 0) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "Invalid HTABMASK 0x" TARGET_FMT_lx > + " in SDR1 (must be of form 2^n-1)\n", htabmask); > + return; > + } > + } > env->spr[SPR_SDR1] = value; > } > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode 2026-08-11 13:17 ` Chinmay Rath @ 2026-08-11 13:21 ` Chinmay Rath 2026-08-14 3:06 ` sesame_h 0 siblings, 1 reply; 5+ messages in thread From: Chinmay Rath @ 2026-08-11 13:21 UTC (permalink / raw) To: sesame_h, qemu-devel Cc: qemu-ppc, npiggin, adityag, milesg, harshpb, Minhang Zhang On 8/11/26 18:47, Chinmay Rath wrote: > Hi Minhang, > Thanks for fixing this. Took me a while to get hold of the docs for 32 > bit mmu, hence the late response. > > On 8/7/26 14:47, sesame_h@qq.com wrote: >> From: Minhang Zhang <zhangminhang@kylinos.cn> >> >> ppc_store_sdr1() had validation for 64-bit SDR1 values but lacked >> corresponding checks for the 32-bit case. According to the Power ISA, >> in 32-bit mode SDR1 bits 16-22 are reserved (must be zero) and >> HTABMASK (bits 23-31) must consist of a consecutive string of >> 1-bits starting from the LSB, i.e., be of the form 2^n-1. >> >> Add checks to reject invalid HTABMASK values and log a guest error >> for non-zero reserved bits, following the same pattern used by the >> existing 64-bit validation. >> >> Signed-off-by: Minhang Zhang <zhangminhang@kylinos.cn> >> --- >> target/ppc/mmu_common.c | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c >> index 2499e61..59e8324 100644 >> --- a/target/ppc/mmu_common.c >> +++ b/target/ppc/mmu_common.c >> @@ -57,9 +57,23 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong >> value) >> " stored in SDR1", htabsize); >> return; >> } >> - } >> + } else >> #endif /* defined(TARGET_PPC64) */ >> - /* FIXME: Should check for valid HTABMASK values in 32-bit case */ >> + { >> + target_ulong htabmask = value & SDR_32_HTABMASK; >> + if (value & 0x007F0000UL) { > From what I see in the programming manual, shouldn't the bits of this > flag be the other way around : 0x0000FE00 ? "bits of this mask", is what I meant. Not "flag", my bad. > Nevertheless, it would be better to form the mask by ORing the > SDR_32_HTABORG and SDR_32_HTABMASK fields from mmu-hash32.h, instead > of hardcoding it; the way it is already done for 64 bits in the same > function. > > Regards, > Chinmay >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "Invalid SDR1: reserved bits 0x" >> TARGET_FMT_lx >> + " set\n", value & 0x007F0000UL); >> + value &= ~0x007F0000UL; >> + } >> + if ((htabmask & (htabmask + 1)) != 0) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "Invalid HTABMASK 0x" TARGET_FMT_lx >> + " in SDR1 (must be of form 2^n-1)\n", >> htabmask); >> + return; >> + } >> + } >> env->spr[SPR_SDR1] = value; >> } ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode 2026-08-11 13:21 ` Chinmay Rath @ 2026-08-14 3:06 ` sesame_h 2026-08-14 8:29 ` Chinmay Rath 0 siblings, 1 reply; 5+ messages in thread From: sesame_h @ 2026-08-14 3:06 UTC (permalink / raw) To: qemu-devel, qemu-ppc; +Cc: npiggin, adityag, milesg, harshpb, Minhang Zhang From: Minhang Zhang <zhangminhang@kylinos.cn> ppc_store_sdr1() had validation for 64-bit SDR1 values but lacked corresponding checks for the 32-bit case. According to the Power ISA, in 32-bit mode SDR1 bits 16-22 are reserved (must be zero) and HTABMASK (bits 23-31) must consist of a consecutive string of 1-bits starting from the LSB, i.e., be of the form 2^n-1. Add checks to reject invalid HTABMASK values and log a guest error for non-zero reserved bits, following the same pattern used by the existing 64-bit validation. Signed-off-by: Minhang Zhang <zhangminhang@kylinos.cn> Hi Chinmay, Thanks a lot for your careful review and pointing out these issues. You are absolutely right, I messed up the reserved-bits mask. I misread the Power ISA bit numbering: the correct reserved-bits mask should be 0x0000FE00, not 0x007F0000. I also agree with your suggestion to avoid hard-coded magic numbers, so I constructed the mask using the existing SDR_32_HTABORG and SDR_32_HTABMASK macros from mmu-hash32.h, following the same pattern as the 64-bit implementation in ppc_store_sdr1(). Both issues are fixed in the v2 patch below. Regards, Minhang Zhang --- target/ppc/mmu_common.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c index 2499e61..31a221d 100644 --- a/target/ppc/mmu_common.c +++ b/target/ppc/mmu_common.c @@ -57,9 +57,25 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value) " stored in SDR1", htabsize); return; } - } + } else #endif /* defined(TARGET_PPC64) */ - /* FIXME: Should check for valid HTABMASK values in 32-bit case */ + { + target_ulong sdr_mask = SDR_32_HTABORG | SDR_32_HTABMASK; + target_ulong htabmask = value & SDR_32_HTABMASK; + + if (value & ~sdr_mask) { + qemu_log_mask(LOG_GUEST_ERROR, + "Invalid bits 0x" TARGET_FMT_lx + " set in SDR1\n", value & ~sdr_mask); + value &= sdr_mask; + } + if ((htabmask & (htabmask + 1)) != 0) { + qemu_log_mask(LOG_GUEST_ERROR, + "Invalid HTABMASK 0x" TARGET_FMT_lx + " in SDR1 (must be of form 2^n-1)\n", htabmask); + return; + } + } env->spr[SPR_SDR1] = value; } -- 2.43.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode 2026-08-14 3:06 ` sesame_h @ 2026-08-14 8:29 ` Chinmay Rath 0 siblings, 0 replies; 5+ messages in thread From: Chinmay Rath @ 2026-08-14 8:29 UTC (permalink / raw) To: sesame_h, qemu-devel, qemu-ppc Cc: npiggin, adityag, milesg, harshpb, Minhang Zhang, ritesh.list, dhruvb On 8/14/26 08:36, sesame_h@qq.com wrote: > From: Minhang Zhang <zhangminhang@kylinos.cn> > > ppc_store_sdr1() had validation for 64-bit SDR1 values but lacked > corresponding checks for the 32-bit case. According to the Power ISA, > in 32-bit mode SDR1 bits 16-22 are reserved (must be zero) and > HTABMASK (bits 23-31) must consist of a consecutive string of > 1-bits starting from the LSB, i.e., be of the form 2^n-1. > > Add checks to reject invalid HTABMASK values and log a guest error > for non-zero reserved bits, following the same pattern used by the > existing 64-bit validation. > > Signed-off-by: Minhang Zhang <zhangminhang@kylinos.cn> > > Hi Chinmay, > > Thanks a lot for your careful review and pointing out these issues. > > You are absolutely right, I messed up the reserved-bits mask. I misread > the Power ISA bit numbering: the correct reserved-bits mask should be > 0x0000FE00, not 0x007F0000. > > I also agree with your suggestion to avoid hard-coded magic numbers, so > I constructed the mask using the existing SDR_32_HTABORG and > SDR_32_HTABMASK macros from mmu-hash32.h, following the same pattern as > the 64-bit implementation in ppc_store_sdr1(). > > Both issues are fixed in the v2 patch below. Hi Minhang, Thanks for the v2. Could you please send this as a separate patch in the list rather than as a reply to this thread ? This will help the maintainer pull in the patch easily :) Plus I had a nit below, sorry I didn't notice it in the v1 : > > Regards, > Minhang Zhang > --- > target/ppc/mmu_common.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c > index 2499e61..31a221d 100644 > --- a/target/ppc/mmu_common.c > +++ b/target/ppc/mmu_common.c > @@ -57,9 +57,25 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value) > " stored in SDR1", htabsize); > return; > } > - } > + } else > #endif /* defined(TARGET_PPC64) */ > - /* FIXME: Should check for valid HTABMASK values in 32-bit case */ > + { Qemu coding style (https://qemu-project.gitlab.io/qemu/devel/style.html#block-structure) , discourages having '{' in the next line after the else. If you could fix that by using #elif instead of #endif here or keeping the entire #if defined(TARGET_PPC64)..#endif within the if (mmu_is_64bit(env->mmu_model)) block, that'd be great. Regards, Chinmay > + target_ulong sdr_mask = SDR_32_HTABORG | SDR_32_HTABMASK; > + target_ulong htabmask = value & SDR_32_HTABMASK; > + > + if (value & ~sdr_mask) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "Invalid bits 0x" TARGET_FMT_lx > + " set in SDR1\n", value & ~sdr_mask); > + value &= sdr_mask; > + } > + if ((htabmask & (htabmask + 1)) != 0) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "Invalid HTABMASK 0x" TARGET_FMT_lx > + " in SDR1 (must be of form 2^n-1)\n", htabmask); > + return; > + } > + } > env->spr[SPR_SDR1] = value; > } > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-08-14 8:30 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-07 9:17 [PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode sesame_h 2026-08-11 13:17 ` Chinmay Rath 2026-08-11 13:21 ` Chinmay Rath 2026-08-14 3:06 ` sesame_h 2026-08-14 8:29 ` Chinmay Rath
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.