[PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode

[PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode

[sesame_h]

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

Re: [PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode

[Chinmay Rath]

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

Re: [PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode

[Chinmay Rath]

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

[PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode

[sesame_h]

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

Re: [PATCH] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode

[Chinmay Rath]

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