diff mbox series

[v3,02/12] ppc: Remove stub of PPC970 HID4 implementation

Message ID 20200219005414.15635-3-david@gibson.dropbear.id.au
State New, archived
Headers show
Series target/ppc: Correct some errors with real mode handling | expand

Commit Message

David Gibson Feb. 19, 2020, 12:54 a.m. UTC
The PowerPC 970 CPU was a cut-down POWER4, which had hypervisor capability.
However, it can be (and often was) strapped into "Apple mode", where the
hypervisor capabilities were disabled (essentially putting it always in
hypervisor mode).

That's actually the only mode of the 970 we support in qemu, and we're
unlikely to change that any time soon.  However, we do have a partial
implementation of the 970's HID4 register which affects things only
relevant for hypervisor mode.

That stub is also really ugly, since it attempts to duplicate the effects
of HID4 by re-encoding it into the LPCR register used in newer CPUs, but
in a really confusing way.

Just get rid of it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 target/ppc/mmu-hash64.c         | 28 +---------------------------
 target/ppc/translate_init.inc.c | 17 ++++++-----------
 2 files changed, 7 insertions(+), 38 deletions(-)

Comments

BALATON Zoltan Feb. 19, 2020, 11:18 a.m. UTC | #1
On Wed, 19 Feb 2020, David Gibson wrote:
> The PowerPC 970 CPU was a cut-down POWER4, which had hypervisor capability.
> However, it can be (and often was) strapped into "Apple mode", where the
> hypervisor capabilities were disabled (essentially putting it always in
> hypervisor mode).
>
> That's actually the only mode of the 970 we support in qemu, and we're
> unlikely to change that any time soon.  However, we do have a partial
> implementation of the 970's HID4 register which affects things only
> relevant for hypervisor mode.
>
> That stub is also really ugly, since it attempts to duplicate the effects
> of HID4 by re-encoding it into the LPCR register used in newer CPUs, but
> in a really confusing way.
>
> Just get rid of it.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
> target/ppc/mmu-hash64.c         | 28 +---------------------------
> target/ppc/translate_init.inc.c | 17 ++++++-----------
> 2 files changed, 7 insertions(+), 38 deletions(-)
>
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index da8966ccf5..a881876647 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -1091,33 +1091,6 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>
>     /* Filter out bits */
>     switch (env->mmu_model) {
> -    case POWERPC_MMU_64B: /* 970 */
> -        if (val & 0x40) {
> -            lpcr |= LPCR_LPES0;
> -        }
> -        if (val & 0x8000000000000000ull) {
> -            lpcr |= LPCR_LPES1;
> -        }
> -        if (val & 0x20) {
> -            lpcr |= (0x4ull << LPCR_RMLS_SHIFT);
> -        }
> -        if (val & 0x4000000000000000ull) {
> -            lpcr |= (0x2ull << LPCR_RMLS_SHIFT);
> -        }
> -        if (val & 0x2000000000000000ull) {
> -            lpcr |= (0x1ull << LPCR_RMLS_SHIFT);
> -        }
> -        env->spr[SPR_RMOR] = ((lpcr >> 41) & 0xffffull) << 26;
> -
> -        /*
> -         * XXX We could also write LPID from HID4 here
> -         * but since we don't tag any translation on it
> -         * it doesn't actually matter
> -         *
> -         * XXX For proper emulation of 970 we also need
> -         * to dig HRMOR out of HID5
> -         */
> -        break;
>     case POWERPC_MMU_2_03: /* P5p */
>         lpcr = val & (LPCR_RMLS | LPCR_ILE |
>                       LPCR_LPES0 | LPCR_LPES1 |
> @@ -1154,6 +1127,7 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>         }
>         break;
>     default:
> +        g_assert_not_reached();
>         ;

Is this empty statement (lone semicolon) still needed now that you've 
added something to this case? Thought it was only there to be able to add 
a label to it so it could be removed now. (Does this count as a double ; 
that a recent patch was trying to fix?)

Regards,
BALATON Zoltan

>     }
>     env->spr[SPR_LPCR] = lpcr;
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index a0d0eaabf2..d7d4f012b8 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -7895,25 +7895,20 @@ static void spr_write_lpcr(DisasContext *ctx, int sprn, int gprn)
> {
>     gen_helper_store_lpcr(cpu_env, cpu_gpr[gprn]);
> }
> -
> -static void spr_write_970_hid4(DisasContext *ctx, int sprn, int gprn)
> -{
> -#if defined(TARGET_PPC64)
> -    spr_write_generic(ctx, sprn, gprn);
> -    gen_helper_store_lpcr(cpu_env, cpu_gpr[gprn]);
> -#endif
> -}
> -
> #endif /* !defined(CONFIG_USER_ONLY) */
>
> static void gen_spr_970_lpar(CPUPPCState *env)
> {
> #if !defined(CONFIG_USER_ONLY)
>     /* Logical partitionning */
> -    /* PPC970: HID4 is effectively the LPCR */
> +    /* PPC970: HID4 covers things later controlled by the LPCR and
> +     * RMOR in later CPUs, but with a different encoding.  We only
> +     * support the 970 in "Apple mode" which has all hypervisor
> +     * facilities disabled by strapping, so we can basically just
> +     * ignore it */
>     spr_register(env, SPR_970_HID4, "HID4",
>                  SPR_NOACCESS, SPR_NOACCESS,
> -                 &spr_read_generic, &spr_write_970_hid4,
> +                 &spr_read_generic, &spr_write_generic,
>                  0x00000000);
> #endif
> }
>
David Gibson Feb. 20, 2020, 12:36 a.m. UTC | #2
On Wed, Feb 19, 2020 at 12:18:34PM +0100, BALATON Zoltan wrote:
> On Wed, 19 Feb 2020, David Gibson wrote:
> > The PowerPC 970 CPU was a cut-down POWER4, which had hypervisor capability.
> > However, it can be (and often was) strapped into "Apple mode", where the
> > hypervisor capabilities were disabled (essentially putting it always in
> > hypervisor mode).
> > 
> > That's actually the only mode of the 970 we support in qemu, and we're
> > unlikely to change that any time soon.  However, we do have a partial
> > implementation of the 970's HID4 register which affects things only
> > relevant for hypervisor mode.
> > 
> > That stub is also really ugly, since it attempts to duplicate the effects
> > of HID4 by re-encoding it into the LPCR register used in newer CPUs, but
> > in a really confusing way.
> > 
> > Just get rid of it.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > ---
> > target/ppc/mmu-hash64.c         | 28 +---------------------------
> > target/ppc/translate_init.inc.c | 17 ++++++-----------
> > 2 files changed, 7 insertions(+), 38 deletions(-)
> > 
> > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > index da8966ccf5..a881876647 100644
> > --- a/target/ppc/mmu-hash64.c
> > +++ b/target/ppc/mmu-hash64.c
> > @@ -1091,33 +1091,6 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
> > 
> >     /* Filter out bits */
> >     switch (env->mmu_model) {
> > -    case POWERPC_MMU_64B: /* 970 */
> > -        if (val & 0x40) {
> > -            lpcr |= LPCR_LPES0;
> > -        }
> > -        if (val & 0x8000000000000000ull) {
> > -            lpcr |= LPCR_LPES1;
> > -        }
> > -        if (val & 0x20) {
> > -            lpcr |= (0x4ull << LPCR_RMLS_SHIFT);
> > -        }
> > -        if (val & 0x4000000000000000ull) {
> > -            lpcr |= (0x2ull << LPCR_RMLS_SHIFT);
> > -        }
> > -        if (val & 0x2000000000000000ull) {
> > -            lpcr |= (0x1ull << LPCR_RMLS_SHIFT);
> > -        }
> > -        env->spr[SPR_RMOR] = ((lpcr >> 41) & 0xffffull) << 26;
> > -
> > -        /*
> > -         * XXX We could also write LPID from HID4 here
> > -         * but since we don't tag any translation on it
> > -         * it doesn't actually matter
> > -         *
> > -         * XXX For proper emulation of 970 we also need
> > -         * to dig HRMOR out of HID5
> > -         */
> > -        break;
> >     case POWERPC_MMU_2_03: /* P5p */
> >         lpcr = val & (LPCR_RMLS | LPCR_ILE |
> >                       LPCR_LPES0 | LPCR_LPES1 |
> > @@ -1154,6 +1127,7 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
> >         }
> >         break;
> >     default:
> > +        g_assert_not_reached();
> >         ;
> 
> Is this empty statement (lone semicolon) still needed now that you've added
> something to this case? Thought it was only there to be able to add a label
> to it so it could be removed now. (Does this count as a double ; that a
> recent patch was trying to fix?)

The ; is redundant, but given this whole chunk of code is removed
later in the series, I don't think it's worth messing with.

> 
> Regards,
> BALATON Zoltan
> 
> >     }
> >     env->spr[SPR_LPCR] = lpcr;
> > diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> > index a0d0eaabf2..d7d4f012b8 100644
> > --- a/target/ppc/translate_init.inc.c
> > +++ b/target/ppc/translate_init.inc.c
> > @@ -7895,25 +7895,20 @@ static void spr_write_lpcr(DisasContext *ctx, int sprn, int gprn)
> > {
> >     gen_helper_store_lpcr(cpu_env, cpu_gpr[gprn]);
> > }
> > -
> > -static void spr_write_970_hid4(DisasContext *ctx, int sprn, int gprn)
> > -{
> > -#if defined(TARGET_PPC64)
> > -    spr_write_generic(ctx, sprn, gprn);
> > -    gen_helper_store_lpcr(cpu_env, cpu_gpr[gprn]);
> > -#endif
> > -}
> > -
> > #endif /* !defined(CONFIG_USER_ONLY) */
> > 
> > static void gen_spr_970_lpar(CPUPPCState *env)
> > {
> > #if !defined(CONFIG_USER_ONLY)
> >     /* Logical partitionning */
> > -    /* PPC970: HID4 is effectively the LPCR */
> > +    /* PPC970: HID4 covers things later controlled by the LPCR and
> > +     * RMOR in later CPUs, but with a different encoding.  We only
> > +     * support the 970 in "Apple mode" which has all hypervisor
> > +     * facilities disabled by strapping, so we can basically just
> > +     * ignore it */
> >     spr_register(env, SPR_970_HID4, "HID4",
> >                  SPR_NOACCESS, SPR_NOACCESS,
> > -                 &spr_read_generic, &spr_write_970_hid4,
> > +                 &spr_read_generic, &spr_write_generic,
> >                  0x00000000);
> > #endif
> > }
> >
diff mbox series

Patch

diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index da8966ccf5..a881876647 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -1091,33 +1091,6 @@  void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
 
     /* Filter out bits */
     switch (env->mmu_model) {
-    case POWERPC_MMU_64B: /* 970 */
-        if (val & 0x40) {
-            lpcr |= LPCR_LPES0;
-        }
-        if (val & 0x8000000000000000ull) {
-            lpcr |= LPCR_LPES1;
-        }
-        if (val & 0x20) {
-            lpcr |= (0x4ull << LPCR_RMLS_SHIFT);
-        }
-        if (val & 0x4000000000000000ull) {
-            lpcr |= (0x2ull << LPCR_RMLS_SHIFT);
-        }
-        if (val & 0x2000000000000000ull) {
-            lpcr |= (0x1ull << LPCR_RMLS_SHIFT);
-        }
-        env->spr[SPR_RMOR] = ((lpcr >> 41) & 0xffffull) << 26;
-
-        /*
-         * XXX We could also write LPID from HID4 here
-         * but since we don't tag any translation on it
-         * it doesn't actually matter
-         *
-         * XXX For proper emulation of 970 we also need
-         * to dig HRMOR out of HID5
-         */
-        break;
     case POWERPC_MMU_2_03: /* P5p */
         lpcr = val & (LPCR_RMLS | LPCR_ILE |
                       LPCR_LPES0 | LPCR_LPES1 |
@@ -1154,6 +1127,7 @@  void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
         }
         break;
     default:
+        g_assert_not_reached();
         ;
     }
     env->spr[SPR_LPCR] = lpcr;
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index a0d0eaabf2..d7d4f012b8 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -7895,25 +7895,20 @@  static void spr_write_lpcr(DisasContext *ctx, int sprn, int gprn)
 {
     gen_helper_store_lpcr(cpu_env, cpu_gpr[gprn]);
 }
-
-static void spr_write_970_hid4(DisasContext *ctx, int sprn, int gprn)
-{
-#if defined(TARGET_PPC64)
-    spr_write_generic(ctx, sprn, gprn);
-    gen_helper_store_lpcr(cpu_env, cpu_gpr[gprn]);
-#endif
-}
-
 #endif /* !defined(CONFIG_USER_ONLY) */
 
 static void gen_spr_970_lpar(CPUPPCState *env)
 {
 #if !defined(CONFIG_USER_ONLY)
     /* Logical partitionning */
-    /* PPC970: HID4 is effectively the LPCR */
+    /* PPC970: HID4 covers things later controlled by the LPCR and
+     * RMOR in later CPUs, but with a different encoding.  We only
+     * support the 970 in "Apple mode" which has all hypervisor
+     * facilities disabled by strapping, so we can basically just
+     * ignore it */
     spr_register(env, SPR_970_HID4, "HID4",
                  SPR_NOACCESS, SPR_NOACCESS,
-                 &spr_read_generic, &spr_write_970_hid4,
+                 &spr_read_generic, &spr_write_generic,
                  0x00000000);
 #endif
 }