[PATCH] target/ppc: Fix test on mmu_model in hreg_compute_hflags_value()

Cédric Le Goater posted 1 patch 2 years, 2 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220124081609.3672341-1-clg@kaod.org
Maintainers: David Gibson <david@gibson.dropbear.id.au>, Daniel Henrique Barboza <danielhb413@gmail.com>, Greg Kurz <groug@kaod.org>, "Cédric Le Goater" <clg@kaod.org>
target/ppc/helper_regs.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
[PATCH] target/ppc: Fix test on mmu_model in hreg_compute_hflags_value()
Posted by Cédric Le Goater 2 years, 2 months ago
POWERPC_MMU_BOOKE is not a mask and should not be tested with a
bitwise AND operator.

It went unnoticed because it only impacts the 601 CPU implementation
for which we don't have a known firmware image.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 target/ppc/helper_regs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index 7dca585dddeb..5b12cb03c961 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -156,7 +156,8 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
      */
     unsigned immu_idx, dmmu_idx;
     dmmu_idx = msr & (1 << MSR_PR) ? 0 : 1;
-    if (env->mmu_model & POWERPC_MMU_BOOKE) {
+    if (env->mmu_model == POWERPC_MMU_BOOKE ||
+        env->mmu_model == POWERPC_MMU_BOOKE206) {
         dmmu_idx |= msr & (1 << MSR_GS) ? 4 : 0;
         immu_idx = dmmu_idx;
         immu_idx |= msr & (1 << MSR_IS) ? 2 : 0;
@@ -237,7 +238,8 @@ int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv)
         ((value >> MSR_DR) & 1) != msr_dr) {
         cpu_interrupt_exittb(cs);
     }
-    if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
+    if ((env->mmu_model == POWERPC_MMU_BOOKE ||
+         env->mmu_model == POWERPC_MMU_BOOKE206) &&
         ((value >> MSR_GS) & 1) != msr_gs) {
         cpu_interrupt_exittb(cs);
     }
-- 
2.31.1


Re: [PATCH] target/ppc: Fix test on mmu_model in hreg_compute_hflags_value()
Posted by Cédric Le Goater 2 years, 2 months ago
On 1/24/22 09:16, Cédric Le Goater wrote:
> POWERPC_MMU_BOOKE is not a mask and should not be tested with a
> bitwise AND operator.
> 
> It went unnoticed because it only impacts the 601 CPU implementation
> for which we don't have a known firmware image.

I forgot to change that. There is one here :

   https://github.com/artyom-tarasenko/openfirmware

image :

   https://github.com/artyom-tarasenko/openfirmware/releases/download/40p-20190413/q40pofw-serial.rom

Thanks,

C.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   target/ppc/helper_regs.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 7dca585dddeb..5b12cb03c961 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -156,7 +156,8 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>        */
>       unsigned immu_idx, dmmu_idx;
>       dmmu_idx = msr & (1 << MSR_PR) ? 0 : 1;
> -    if (env->mmu_model & POWERPC_MMU_BOOKE) {
> +    if (env->mmu_model == POWERPC_MMU_BOOKE ||
> +        env->mmu_model == POWERPC_MMU_BOOKE206) {
>           dmmu_idx |= msr & (1 << MSR_GS) ? 4 : 0;
>           immu_idx = dmmu_idx;
>           immu_idx |= msr & (1 << MSR_IS) ? 2 : 0;
> @@ -237,7 +238,8 @@ int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv)
>           ((value >> MSR_DR) & 1) != msr_dr) {
>           cpu_interrupt_exittb(cs);
>       }
> -    if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
> +    if ((env->mmu_model == POWERPC_MMU_BOOKE ||
> +         env->mmu_model == POWERPC_MMU_BOOKE206) &&
>           ((value >> MSR_GS) & 1) != msr_gs) {
>           cpu_interrupt_exittb(cs);
>       }
> 


Re: [PATCH] target/ppc: Fix test on mmu_model in hreg_compute_hflags_value()
Posted by David Gibson 2 years, 2 months ago
On Mon, Jan 24, 2022 at 09:16:09AM +0100, Cédric le Goater wrote:
> POWERPC_MMU_BOOKE is not a mask and should not be tested with a
> bitwise AND operator.
> 
> It went unnoticed because it only impacts the 601 CPU implementation
> for which we don't have a known firmware image.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

The longer term fix, of course, is to entirely get rid of the unholy
mix of enum and bitmask that is mmu_model.

> ---
>  target/ppc/helper_regs.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 7dca585dddeb..5b12cb03c961 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -156,7 +156,8 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>       */
>      unsigned immu_idx, dmmu_idx;
>      dmmu_idx = msr & (1 << MSR_PR) ? 0 : 1;
> -    if (env->mmu_model & POWERPC_MMU_BOOKE) {
> +    if (env->mmu_model == POWERPC_MMU_BOOKE ||
> +        env->mmu_model == POWERPC_MMU_BOOKE206) {
>          dmmu_idx |= msr & (1 << MSR_GS) ? 4 : 0;
>          immu_idx = dmmu_idx;
>          immu_idx |= msr & (1 << MSR_IS) ? 2 : 0;
> @@ -237,7 +238,8 @@ int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv)
>          ((value >> MSR_DR) & 1) != msr_dr) {
>          cpu_interrupt_exittb(cs);
>      }
> -    if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
> +    if ((env->mmu_model == POWERPC_MMU_BOOKE ||
> +         env->mmu_model == POWERPC_MMU_BOOKE206) &&
>          ((value >> MSR_GS) & 1) != msr_gs) {
>          cpu_interrupt_exittb(cs);
>      }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson