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

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

Posted by sesame_h@qq.com 1 week, 5 days ago

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 | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
index 2499e61..2a36817 100644
--- a/target/ppc/mmu_common.c
+++ b/target/ppc/mmu_common.c
@@ -42,24 +42,39 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
     PowerPCCPU *cpu = env_archcpu(env);
     qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value);
     assert(!cpu->env.has_hv_mode || !cpu->vhyp);
-#if defined(TARGET_PPC64)
     if (mmu_is_64bit(env->mmu_model)) {
+#if defined(TARGET_PPC64)
         target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE;
         target_ulong htabsize = value & SDR_64_HTABSIZE;
 
         if (value & ~sdr_mask) {
             qemu_log_mask(LOG_GUEST_ERROR, "Invalid bits 0x"TARGET_FMT_lx
-                     " set in SDR1", value & ~sdr_mask);
+                     " set in SDR1\n", value & ~sdr_mask);
             value &= sdr_mask;
         }
         if (htabsize > 28) {
             qemu_log_mask(LOG_GUEST_ERROR, "Invalid HTABSIZE 0x" TARGET_FMT_lx
-                     " stored in SDR1", htabsize);
+                     " stored in SDR1\n", htabsize);
             return;
         }
-    }
 #endif /* defined(TARGET_PPC64) */
-    /* FIXME: Should check for valid HTABMASK values in 32-bit case */
+    } else {
+        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 v3] target/ppc: Validate HTABMASK and reserved bits in SDR1 for 32-bit mode

Posted by Chinmay Rath 1 week, 5 days ago

On 8/14/26 14:26, 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>
Thanks for making the changes.

Reviewed-by: Chinmay Rath <rathc@linux.ibm.com>
> ---
>   target/ppc/mmu_common.c | 25 ++++++++++++++++++++-----
>   1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
> index 2499e61..2a36817 100644
> --- a/target/ppc/mmu_common.c
> +++ b/target/ppc/mmu_common.c
> @@ -42,24 +42,39 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
>       PowerPCCPU *cpu = env_archcpu(env);
>       qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value);
>       assert(!cpu->env.has_hv_mode || !cpu->vhyp);
> -#if defined(TARGET_PPC64)
>       if (mmu_is_64bit(env->mmu_model)) {
> +#if defined(TARGET_PPC64)
>           target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE;
>           target_ulong htabsize = value & SDR_64_HTABSIZE;
>   
>           if (value & ~sdr_mask) {
>               qemu_log_mask(LOG_GUEST_ERROR, "Invalid bits 0x"TARGET_FMT_lx
> -                     " set in SDR1", value & ~sdr_mask);
> +                     " set in SDR1\n", value & ~sdr_mask);
>               value &= sdr_mask;
>           }
>           if (htabsize > 28) {
>               qemu_log_mask(LOG_GUEST_ERROR, "Invalid HTABSIZE 0x" TARGET_FMT_lx
> -                     " stored in SDR1", htabsize);
> +                     " stored in SDR1\n", htabsize);
>               return;
>           }
> -    }
>   #endif /* defined(TARGET_PPC64) */
> -    /* FIXME: Should check for valid HTABMASK values in 32-bit case */
> +    } else {
> +        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;
>   }
>