[PATCH v2 00/10] target/ppc: Correct some errors with real mode handling

[PATCH v2 00/10] target/ppc: Correct some errors with real mode handling

[David Gibson]

POWER "book S" (server class) cpus have a concept of "real mode" where
MMU translation is disabled... sort of.  In fact this can mean a bunch
of slightly different things when hypervisor mode and other
considerations are present.

We had some errors in edge cases here, so clean some things up and
correct them.

Changes since RFCv1:
 * Add a number of extra patches taking advantage of the initial
   cleanups

David Gibson (10):
  ppc: Drop PPC_EMULATE_32BITS_HYPV stub
  ppc: Remove stub of PPC970 HID4 implementation
  target/ppc: Correct handling of real mode accesses with vhyp on hash
    MMU
  target/ppc: Introduce ppc_hash64_use_vrma() helper
  spapr, ppc: Remove VPM0/RMLS hacks for POWER9
  target/ppc: Remove RMOR register from POWER9 & POWER10
  target/ppc: Use class fields to simplify LPCR masking
  target/ppc: Streamline calculation of RMA limit from LPCR[RMLS]
  target/ppc: Correct RMLS table
  target/ppc: Only calculate RMLS derived RMA limit on demand

 hw/ppc/spapr_cpu_core.c         |   6 +-
 target/ppc/cpu-qom.h            |   1 +
 target/ppc/cpu.h                |   8 --
 target/ppc/mmu-hash64.c         | 241 ++++++++++++--------------------
 target/ppc/translate_init.inc.c |  54 ++++---
 5 files changed, 130 insertions(+), 180 deletions(-)

-- 
2.24.1

[PATCH v2 01/10] ppc: Drop PPC_EMULATE_32BITS_HYPV stub

[David Gibson]

The only effect of the PPC_EMULATE_32BITS_HYPV define is to change how
MSR_HVB is defined.  This appears to be something to handle hypervisor mode
for 32-bit CPUs, but there's really no other code to handle this.  The
MSR_THV bit, which it uses is implemented for no cpus we model.

It's unlikely anyone is going to implement this any time soon, so just drop
it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/cpu.h | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 103bfe9dc2..2de9e2fa2b 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -26,8 +26,6 @@
 #include "exec/cpu-defs.h"
 #include "cpu-qom.h"
 
-/* #define PPC_EMULATE_32BITS_HYPV */
-
 #define TCG_GUEST_DEFAULT_MO 0
 
 #define TARGET_PAGE_BITS_64K 16
@@ -450,14 +448,9 @@ typedef struct ppc_v3_pate_t {
 #define MSR_HVB (1ULL << MSR_SHV)
 #define msr_hv  msr_shv
 #else
-#if defined(PPC_EMULATE_32BITS_HYPV)
-#define MSR_HVB (1ULL << MSR_THV)
-#define msr_hv  msr_thv
-#else
 #define MSR_HVB (0ULL)
 #define msr_hv  (0)
 #endif
-#endif
 
 /* DSISR */
 #define DSISR_NOPTE              0x40000000
-- 
2.24.1

Re: [PATCH v2 01/10] ppc: Drop PPC_EMULATE_32BITS_HYPV stub

[Cédric Le Goater]

On 1/7/20 5:48 AM, David Gibson wrote:
> The only effect of the PPC_EMULATE_32BITS_HYPV define is to change how
> MSR_HVB is defined.  This appears to be something to handle hypervisor mode
> for 32-bit CPUs, but there's really no other code to handle this.  The
> MSR_THV bit, which it uses is implemented for no cpus we model.
> 
> It's unlikely anyone is going to implement this any time soon, so just drop
> it.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  target/ppc/cpu.h | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 103bfe9dc2..2de9e2fa2b 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -26,8 +26,6 @@
>  #include "exec/cpu-defs.h"
>  #include "cpu-qom.h"
>  
> -/* #define PPC_EMULATE_32BITS_HYPV */
> -
>  #define TCG_GUEST_DEFAULT_MO 0
>  
>  #define TARGET_PAGE_BITS_64K 16

btw, this define ^ is unused.

> @@ -450,14 +448,9 @@ typedef struct ppc_v3_pate_t {
>  #define MSR_HVB (1ULL << MSR_SHV)
>  #define msr_hv  msr_shv
>  #else
> -#if defined(PPC_EMULATE_32BITS_HYPV)
> -#define MSR_HVB (1ULL << MSR_THV)
> -#define msr_hv  msr_thv
> -#else
>  #define MSR_HVB (0ULL)
>  #define msr_hv  (0)
>  #endif
> -#endif
>  
>  /* DSISR */
>  #define DSISR_NOPTE              0x40000000
> 

Re: [PATCH v2 01/10] ppc: Drop PPC_EMULATE_32BITS_HYPV stub

[Greg Kurz]

On Tue,  7 Jan 2020 15:48:18 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> The only effect of the PPC_EMULATE_32BITS_HYPV define is to change how
> MSR_HVB is defined.  This appears to be something to handle hypervisor mode
> for 32-bit CPUs, but there's really no other code to handle this.  The
> MSR_THV bit, which it uses is implemented for no cpus we model.
> 
> It's unlikely anyone is going to implement this any time soon, so just drop
> it.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/cpu.h | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 103bfe9dc2..2de9e2fa2b 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -26,8 +26,6 @@
>  #include "exec/cpu-defs.h"
>  #include "cpu-qom.h"
>  
> -/* #define PPC_EMULATE_32BITS_HYPV */
> -
>  #define TCG_GUEST_DEFAULT_MO 0
>  
>  #define TARGET_PAGE_BITS_64K 16
> @@ -450,14 +448,9 @@ typedef struct ppc_v3_pate_t {
>  #define MSR_HVB (1ULL << MSR_SHV)
>  #define msr_hv  msr_shv
>  #else
> -#if defined(PPC_EMULATE_32BITS_HYPV)
> -#define MSR_HVB (1ULL << MSR_THV)
> -#define msr_hv  msr_thv
> -#else

These lines come from commit a4f30719a8cd, which also introduced msr_thv
and msr_shv to differentiate the 32 and 64 bits HV mode. Maybe you can go
one step further: drop msr_thv and msr_shv and revert to msr_hv only, like
before a4f30719a8cd ?

>  #define MSR_HVB (0ULL)
>  #define msr_hv  (0)
>  #endif
> -#endif
>  
>  /* DSISR */
>  #define DSISR_NOPTE              0x40000000

[PATCH v2 02/10] ppc: Remove stub of PPC970 HID4 implementation

[David Gibson]

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>
---
 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();
         ;
     }
     env->spr[SPR_LPCR] = lpcr;
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index d33d65dff7..436d0d5a51 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -7884,25 +7884,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
 }
-- 
2.24.1

Re: [PATCH v2 02/10] ppc: Remove stub of PPC970 HID4 implementation

[Cédric Le Goater]

On 1/7/20 5:48 AM, 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>


> ---
>  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();
>          ;
>      }
>      env->spr[SPR_LPCR] = lpcr;
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index d33d65dff7..436d0d5a51 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -7884,25 +7884,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
>  }
> 

Re: [PATCH v2 02/10] ppc: Remove stub of PPC970 HID4 implementation

[Greg Kurz]

On Tue,  7 Jan 2020 15:48:19 +1100
David Gibson <david@gibson.dropbear.id.au> 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: 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();
>          ;
>      }
>      env->spr[SPR_LPCR] = lpcr;
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index d33d65dff7..436d0d5a51 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -7884,25 +7884,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
>  }

Re: [PATCH v2 02/10] ppc: Remove stub of PPC970 HID4 implementation

[Greg Kurz]

On Tue, 7 Jan 2020 18:32:15 +0100
Greg Kurz <groug@kaod.org> wrote:

> On Tue,  7 Jan 2020 15:48:19 +1100
> David Gibson <david@gibson.dropbear.id.au> 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).

Isn't it supervisor mode instead of 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: 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();
> >          ;
> >      }
> >      env->spr[SPR_LPCR] = lpcr;
> > diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> > index d33d65dff7..436d0d5a51 100644
> > --- a/target/ppc/translate_init.inc.c
> > +++ b/target/ppc/translate_init.inc.c
> > @@ -7884,25 +7884,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
> >  }
> 
> 

Re: [PATCH v2 02/10] ppc: Remove stub of PPC970 HID4 implementation

[BALATON Zoltan]

On Tue, 7 Jan 2020, Greg Kurz wrote:
> On Tue, 7 Jan 2020 18:32:15 +0100
> Greg Kurz <groug@kaod.org> wrote:
>
>> On Tue,  7 Jan 2020 15:48:19 +1100
>> David Gibson <david@gibson.dropbear.id.au> 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).
>
> Isn't it supervisor mode instead of hypervisor mode ?

By the way, do you know if this strapping is hardware or software based? 
So is it the firmware that disables it on Apple hardware or is it some CPU 
pin connected somewhere on the motherboard or it's within the CPU and 
cannot be changed? I wonder if it's theoretically possible to re-enable it 
on an Apple G5 or we would likely never see a PowerPC 970 with HV enabled?

Regards,
BALATON Zoltan

Re: [PATCH v2 02/10] ppc: Remove stub of PPC970 HID4 implementation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

On Tue, Jan 07, 2020 at 07:05:41PM +0100, BALATON Zoltan wrote:
> On Tue, 7 Jan 2020, Greg Kurz wrote:
> > On Tue, 7 Jan 2020 18:32:15 +0100
> > Greg Kurz <groug@kaod.org> wrote:
> > 
> > > On Tue,  7 Jan 2020 15:48:19 +1100
> > > David Gibson <david@gibson.dropbear.id.au> 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).
> > 
> > Isn't it supervisor mode instead of hypervisor mode ?
> 
> By the way, do you know if this strapping is hardware or software based? So
> is it the firmware that disables it on Apple hardware or is it some CPU pin
> connected somewhere on the motherboard or it's within the CPU and cannot be
> changed? I wonder if it's theoretically possible to re-enable it on an Apple
> G5 or we would likely never see a PowerPC 970 with HV enabled?

I don't know, sorry.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 02/10] ppc: Remove stub of PPC970 HID4 implementation

[Thomas Huth]

On 07/01/2020 19.05, BALATON Zoltan wrote:
> On Tue, 7 Jan 2020, Greg Kurz wrote:
>> On Tue, 7 Jan 2020 18:32:15 +0100
>> Greg Kurz <groug@kaod.org> wrote:
>>
>>> On Tue,  7 Jan 2020 15:48:19 +1100
>>> David Gibson <david@gibson.dropbear.id.au> 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).
>>
>> Isn't it supervisor mode instead of hypervisor mode ?
> 
> By the way, do you know if this strapping is hardware or software based?
> So is it the firmware that disables it on Apple hardware or is it some
> CPU pin connected somewhere on the motherboard or it's within the CPU
> and cannot be changed? I wonder if it's theoretically possible to
> re-enable it on an Apple G5 or we would likely never see a PowerPC 970
> with HV enabled?

I don't know how Apple disabled it, but you can buy a used Terrasoft YDL
PowerStation if you want to see it in action.

 Thomas

Re: [PATCH v2 02/10] ppc: Remove stub of PPC970 HID4 implementation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5307 bytes --]

On Tue, Jan 07, 2020 at 06:36:38PM +0100, Greg Kurz wrote:
> On Tue, 7 Jan 2020 18:32:15 +0100
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Tue,  7 Jan 2020 15:48:19 +1100
> > David Gibson <david@gibson.dropbear.id.au> 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).
> 
> Isn't it supervisor mode instead of hypervisor mode ?

No; hypervisor is correct.  If the cpu was always in supervisor mode,
the boot OS couldn't access the hypervisor privileged registers that
are needed for basic setup (e.g. SDR1).  "Apple mode" means the cpu
doesn't have a supervisor mode that _isn't_ hypervisor privileged and
hence, can't run guests.

> 
> > > 
> > > 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: 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();
> > >          ;
> > >      }
> > >      env->spr[SPR_LPCR] = lpcr;
> > > diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> > > index d33d65dff7..436d0d5a51 100644
> > > --- a/target/ppc/translate_init.inc.c
> > > +++ b/target/ppc/translate_init.inc.c
> > > @@ -7884,25 +7884,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			| 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 02/10] ppc: Remove stub of PPC970 HID4 implementation

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 5498 bytes --]

On Wed, 8 Jan 2020 12:08:50 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jan 07, 2020 at 06:36:38PM +0100, Greg Kurz wrote:
> > On Tue, 7 Jan 2020 18:32:15 +0100
> > Greg Kurz <groug@kaod.org> wrote:
> > 
> > > On Tue,  7 Jan 2020 15:48:19 +1100
> > > David Gibson <david@gibson.dropbear.id.au> 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).
> > 
> > Isn't it supervisor mode instead of hypervisor mode ?
> 
> No; hypervisor is correct.  If the cpu was always in supervisor mode,
> the boot OS couldn't access the hypervisor privileged registers that
> are needed for basic setup (e.g. SDR1).  "Apple mode" means the cpu
> doesn't have a supervisor mode that _isn't_ hypervisor privileged and
> hence, can't run guests.
> 

Ok, thanks for the clarification.

> > 
> > > > 
> > > > 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: 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();
> > > >          ;
> > > >      }
> > > >      env->spr[SPR_LPCR] = lpcr;
> > > > diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> > > > index d33d65dff7..436d0d5a51 100644
> > > > --- a/target/ppc/translate_init.inc.c
> > > > +++ b/target/ppc/translate_init.inc.c
> > > > @@ -7884,25 +7884,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
> > > >  }
> > > 
> > > 
> > 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 02/10] ppc: Remove stub of PPC970 HID4 implementation

[Paul Mackerras]

On Tue, Jan 07, 2020 at 06:36:38PM +0100, Greg Kurz wrote:
> On Tue, 7 Jan 2020 18:32:15 +0100
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Tue,  7 Jan 2020 15:48:19 +1100
> > David Gibson <david@gibson.dropbear.id.au> 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).
> 
> Isn't it supervisor mode instead of hypervisor mode ?

No, it's hypervisor mode.  MSR[HV] always reads as 1.

Paul.

Re: [PATCH v2 02/10] ppc: Remove stub of PPC970 HID4 implementation

[Greg Kurz]

On Tue,  7 Jan 2020 15:48:19 +1100
David Gibson <david@gibson.dropbear.id.au> 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.
> 

FWIW, it is currently _possible_ to boot an RHEL 6.1 guest with a 970mp
CPU under TCG (newer guests like RHEL 7.5 don't work because the kernel
seems to have instructions not supported on 970). Of course this no longer
works with this patch, but I guess we don't really care for such an old
setup, do we ?

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

Just one cosmetic nit I hadn't spotted before...

>  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();
>          ;

... remove the line with the semi-colon ?
 
>      }
>      env->spr[SPR_LPCR] = lpcr;
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index d33d65dff7..436d0d5a51 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -7884,25 +7884,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
>  }

[PATCH v2 03/10] target/ppc: Correct handling of real mode accesses with vhyp on hash MMU

[David Gibson]

On ppc we have the concept of virtual hypervisor ("vhyp") mode, where we
only model the non-hypervisor-privileged parts of the cpu.  Essentially we
model the hypervisor's behaviour from the point of view of a guest OS, but
we don't model the hypervisor's execution.

In particular, in this mode, qemu's notion of target physical address is
a guest physical address from the vcpu's point of view.  So accesses in
guest real mode don't require translation.  If we were modelling the
hypervisor mode, we'd need to translate the guest physical address into
a host physical address.

Currently, we handle this sloppily: we rely on setting up the virtual LPCR
and RMOR registers so that GPAs are simply HPAs plus an offset, which we
set to zero.  This is already conceptually dubious, since the LPCR and RMOR
registers don't exist in the non-hypervisor portion of the CPU.  It gets
worse with POWER9, where RMOR and LPCR[VPM0] no longer exist at all.

Clean this up by explicitly handling the vhyp case.  While we're there,
remove some unnecessary nesting of if statements that made the logic to
select the correct real mode behaviour a bit less clear than it could be.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/mmu-hash64.c | 60 ++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index a881876647..5fabd93c92 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -789,27 +789,30 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
          */
         raddr = eaddr & 0x0FFFFFFFFFFFFFFFULL;
 
-        /* In HV mode, add HRMOR if top EA bit is clear */
-        if (msr_hv || !env->has_hv_mode) {
+        if (cpu->vhyp) {
+            /*
+             * In virtual hypervisor mode, there's nothing to do:
+             *   EA == GPA == qemu guest address
+             */
+        } else if (msr_hv || !env->has_hv_mode) {
+            /* In HV mode, add HRMOR if top EA bit is clear */
             if (!(eaddr >> 63)) {
                 raddr |= env->spr[SPR_HRMOR];
             }
-        } else {
-            /* Otherwise, check VPM for RMA vs VRMA */
-            if (env->spr[SPR_LPCR] & LPCR_VPM0) {
-                slb = &env->vrma_slb;
-                if (slb->sps) {
-                    goto skip_slb_search;
-                }
-                /* Not much else to do here */
+        } else if (env->spr[SPR_LPCR] & LPCR_VPM0) {
+            /* Emulated VRMA mode */
+            slb = &env->vrma_slb;
+            if (!slb->sps) {
+                /* Invalid VRMA setup, machine check */
                 cs->exception_index = POWERPC_EXCP_MCHECK;
                 env->error_code = 0;
                 return 1;
-            } else if (raddr < env->rmls) {
-                /* RMA. Check bounds in RMLS */
-                raddr |= env->spr[SPR_RMOR];
-            } else {
-                /* The access failed, generate the approriate interrupt */
+            }
+
+            goto skip_slb_search;
+        } else {
+            /* Emulated old-style RMO mode, bounds check against RMLS */
+            if (raddr >= env->rmls) {
                 if (rwx == 2) {
                     ppc_hash64_set_isi(cs, SRR1_PROTFAULT);
                 } else {
@@ -821,6 +824,8 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
                 }
                 return 1;
             }
+
+            raddr |= env->spr[SPR_RMOR];
         }
         tlb_set_page(cs, eaddr & TARGET_PAGE_MASK, raddr & TARGET_PAGE_MASK,
                      PAGE_READ | PAGE_WRITE | PAGE_EXEC, mmu_idx,
@@ -953,22 +958,27 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
         /* In real mode the top 4 effective address bits are ignored */
         raddr = addr & 0x0FFFFFFFFFFFFFFFULL;
 
-        /* In HV mode, add HRMOR if top EA bit is clear */
-        if ((msr_hv || !env->has_hv_mode) && !(addr >> 63)) {
+        if (cpu->vhyp) {
+            /*
+             * In virtual hypervisor mode, there's nothing to do:
+             *   EA == GPA == qemu guest address
+             */
+            return raddr;
+        } else if ((msr_hv || !env->has_hv_mode) && !(addr >> 63)) {
+            /* In HV mode, add HRMOR if top EA bit is clear */
             return raddr | env->spr[SPR_HRMOR];
-        }
-
-        /* Otherwise, check VPM for RMA vs VRMA */
-        if (env->spr[SPR_LPCR] & LPCR_VPM0) {
+        } else if (env->spr[SPR_LPCR] & LPCR_VPM0) {
+            /* Emulated VRMA mode */
             slb = &env->vrma_slb;
             if (!slb->sps) {
                 return -1;
             }
-        } else if (raddr < env->rmls) {
-            /* RMA. Check bounds in RMLS */
-            return raddr | env->spr[SPR_RMOR];
         } else {
-            return -1;
+            /* Emulated old-style RMO mode, bounds check against RMLS */
+            if (raddr >= env->rmls) {
+                return -1;
+            }
+            return raddr | env->spr[SPR_RMOR];
         }
     } else {
         slb = slb_lookup(cpu, addr);
-- 
2.24.1

Re: [PATCH v2 03/10] target/ppc: Correct handling of real mode accesses with vhyp on hash MMU

[Cédric Le Goater]

On 1/7/20 5:48 AM, David Gibson wrote:
> On ppc we have the concept of virtual hypervisor ("vhyp") mode, where we
> only model the non-hypervisor-privileged parts of the cpu.  Essentially we
> model the hypervisor's behaviour from the point of view of a guest OS, but
> we don't model the hypervisor's execution.
> 
> In particular, in this mode, qemu's notion of target physical address is
> a guest physical address from the vcpu's point of view.  So accesses in
> guest real mode don't require translation.  If we were modelling the
> hypervisor mode, we'd need to translate the guest physical address into
> a host physical address.
> 
> Currently, we handle this sloppily: we rely on setting up the virtual LPCR
> and RMOR registers so that GPAs are simply HPAs plus an offset, which we
> set to zero.  This is already conceptually dubious, since the LPCR and RMOR
> registers don't exist in the non-hypervisor portion of the CPU.  It gets
> worse with POWER9, where RMOR and LPCR[VPM0] no longer exist at all.
> 
> Clean this up by explicitly handling the vhyp case.  While we're there,
> remove some unnecessary nesting of if statements that made the logic to
> select the correct real mode behaviour a bit less clear than it could be.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

I went through the changes and they look correct to me.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

C.

> ---
>  target/ppc/mmu-hash64.c | 60 ++++++++++++++++++++++++-----------------
>  1 file changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index a881876647..5fabd93c92 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -789,27 +789,30 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
>           */
>          raddr = eaddr & 0x0FFFFFFFFFFFFFFFULL;
>  
> -        /* In HV mode, add HRMOR if top EA bit is clear */
> -        if (msr_hv || !env->has_hv_mode) {
> +        if (cpu->vhyp) {
> +            /*
> +             * In virtual hypervisor mode, there's nothing to do:
> +             *   EA == GPA == qemu guest address
> +             */
> +        } else if (msr_hv || !env->has_hv_mode) {
> +            /* In HV mode, add HRMOR if top EA bit is clear */
>              if (!(eaddr >> 63)) {
>                  raddr |= env->spr[SPR_HRMOR];
>              }
> -        } else {
> -            /* Otherwise, check VPM for RMA vs VRMA */
> -            if (env->spr[SPR_LPCR] & LPCR_VPM0) {
> -                slb = &env->vrma_slb;
> -                if (slb->sps) {
> -                    goto skip_slb_search;
> -                }
> -                /* Not much else to do here */
> +        } else if (env->spr[SPR_LPCR] & LPCR_VPM0) {
> +            /* Emulated VRMA mode */
> +            slb = &env->vrma_slb;
> +            if (!slb->sps) {
> +                /* Invalid VRMA setup, machine check */
>                  cs->exception_index = POWERPC_EXCP_MCHECK;
>                  env->error_code = 0;
>                  return 1;
> -            } else if (raddr < env->rmls) {
> -                /* RMA. Check bounds in RMLS */
> -                raddr |= env->spr[SPR_RMOR];
> -            } else {
> -                /* The access failed, generate the approriate interrupt */
> +            }
> +
> +            goto skip_slb_search;
> +        } else {
> +            /* Emulated old-style RMO mode, bounds check against RMLS */
> +            if (raddr >= env->rmls) {
>                  if (rwx == 2) {
>                      ppc_hash64_set_isi(cs, SRR1_PROTFAULT);
>                  } else {
> @@ -821,6 +824,8 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
>                  }
>                  return 1;
>              }
> +
> +            raddr |= env->spr[SPR_RMOR];
>          }
>          tlb_set_page(cs, eaddr & TARGET_PAGE_MASK, raddr & TARGET_PAGE_MASK,
>                       PAGE_READ | PAGE_WRITE | PAGE_EXEC, mmu_idx,
> @@ -953,22 +958,27 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
>          /* In real mode the top 4 effective address bits are ignored */
>          raddr = addr & 0x0FFFFFFFFFFFFFFFULL;
>  
> -        /* In HV mode, add HRMOR if top EA bit is clear */
> -        if ((msr_hv || !env->has_hv_mode) && !(addr >> 63)) {
> +        if (cpu->vhyp) {
> +            /*
> +             * In virtual hypervisor mode, there's nothing to do:
> +             *   EA == GPA == qemu guest address
> +             */
> +            return raddr;
> +        } else if ((msr_hv || !env->has_hv_mode) && !(addr >> 63)) {
> +            /* In HV mode, add HRMOR if top EA bit is clear */
>              return raddr | env->spr[SPR_HRMOR];
> -        }
> -
> -        /* Otherwise, check VPM for RMA vs VRMA */
> -        if (env->spr[SPR_LPCR] & LPCR_VPM0) {
> +        } else if (env->spr[SPR_LPCR] & LPCR_VPM0) {
> +            /* Emulated VRMA mode */
>              slb = &env->vrma_slb;
>              if (!slb->sps) {
>                  return -1;
>              }
> -        } else if (raddr < env->rmls) {
> -            /* RMA. Check bounds in RMLS */
> -            return raddr | env->spr[SPR_RMOR];
>          } else {
> -            return -1;
> +            /* Emulated old-style RMO mode, bounds check against RMLS */
> +            if (raddr >= env->rmls) {
> +                return -1;
> +            }
> +            return raddr | env->spr[SPR_RMOR];
>          }
>      } else {
>          slb = slb_lookup(cpu, addr);
> 

[PATCH v2 04/10] target/ppc: Introduce ppc_hash64_use_vrma() helper

[David Gibson]

When running guests under a hypervisor, the hypervisor obviously needs to
be protected from guest accesses even if those are in what the guest
considers real mode (translation off).  The POWER hardware provides two
ways of doing that: The old way has guest real mode accesses simply offset
and bounds checked into host addresses.  It works, but requires that a
significant chunk of the guest's memory - the RMA - be physically
contiguous in the host, which is pretty inconvenient.  The new way, known
as VRMA, has guest real mode accesses translated in roughly the normal way
but with some special parameters.

In POWER7 and POWER8 the LPCR[VPM0] bit selected between the two modes, but
in POWER9 only VRMA mode is supported and LPCR[VPM0] no longer exists.  We
handle that difference in behaviour in ppc_hash64_set_isi().. but not in
other places that we blindly check LPCR[VPM0].

Correct those instances with a new helper to tell if we should be in VRMA
mode.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/mmu-hash64.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 5fabd93c92..d878180df5 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -668,6 +668,19 @@ unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
     return 0;
 }
 
+static bool ppc_hash64_use_vrma(CPUPPCState *env)
+{
+    switch (env->mmu_model) {
+    case POWERPC_MMU_3_00:
+        /* ISAv3.0 (POWER9) always uses VRMA, the VPM0 field and RMOR
+         * register no longer exist */
+        return true;
+
+    default:
+        return !!(env->spr[SPR_LPCR] & LPCR_VPM0);
+    }
+}
+
 static void ppc_hash64_set_isi(CPUState *cs, uint64_t error_code)
 {
     CPUPPCState *env = &POWERPC_CPU(cs)->env;
@@ -676,15 +689,7 @@ static void ppc_hash64_set_isi(CPUState *cs, uint64_t error_code)
     if (msr_ir) {
         vpm = !!(env->spr[SPR_LPCR] & LPCR_VPM1);
     } else {
-        switch (env->mmu_model) {
-        case POWERPC_MMU_3_00:
-            /* Field deprecated in ISAv3.00 - interrupts always go to hyperv */
-            vpm = true;
-            break;
-        default:
-            vpm = !!(env->spr[SPR_LPCR] & LPCR_VPM0);
-            break;
-        }
+        vpm = ppc_hash64_use_vrma(env);
     }
     if (vpm && !msr_hv) {
         cs->exception_index = POWERPC_EXCP_HISI;
@@ -702,15 +707,7 @@ static void ppc_hash64_set_dsi(CPUState *cs, uint64_t dar, uint64_t dsisr)
     if (msr_dr) {
         vpm = !!(env->spr[SPR_LPCR] & LPCR_VPM1);
     } else {
-        switch (env->mmu_model) {
-        case POWERPC_MMU_3_00:
-            /* Field deprecated in ISAv3.00 - interrupts always go to hyperv */
-            vpm = true;
-            break;
-        default:
-            vpm = !!(env->spr[SPR_LPCR] & LPCR_VPM0);
-            break;
-        }
+        vpm = ppc_hash64_use_vrma(env);
     }
     if (vpm && !msr_hv) {
         cs->exception_index = POWERPC_EXCP_HDSI;
@@ -799,7 +796,7 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
             if (!(eaddr >> 63)) {
                 raddr |= env->spr[SPR_HRMOR];
             }
-        } else if (env->spr[SPR_LPCR] & LPCR_VPM0) {
+        } else if (ppc_hash64_use_vrma(env)) {
             /* Emulated VRMA mode */
             slb = &env->vrma_slb;
             if (!slb->sps) {
@@ -967,7 +964,7 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
         } else if ((msr_hv || !env->has_hv_mode) && !(addr >> 63)) {
             /* In HV mode, add HRMOR if top EA bit is clear */
             return raddr | env->spr[SPR_HRMOR];
-        } else if (env->spr[SPR_LPCR] & LPCR_VPM0) {
+        } else if (ppc_hash64_use_vrma(env)) {
             /* Emulated VRMA mode */
             slb = &env->vrma_slb;
             if (!slb->sps) {
@@ -1056,8 +1053,7 @@ static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
     slb->sps = NULL;
 
     /* Is VRMA enabled ? */
-    lpcr = env->spr[SPR_LPCR];
-    if (!(lpcr & LPCR_VPM0)) {
+    if (ppc_hash64_use_vrma(env)) {
         return;
     }
 
@@ -1065,6 +1061,7 @@ static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
      * Make one up. Mostly ignore the ESID which will not be needed
      * for translation
      */
+    lpcr = env->spr[SPR_LPCR];
     vsid = SLB_VSID_VRMA;
     vrmasd = (lpcr & LPCR_VRMASD) >> LPCR_VRMASD_SHIFT;
     vsid |= (vrmasd << 4) & (SLB_VSID_L | SLB_VSID_LP);
-- 
2.24.1

Re: [PATCH v2 04/10] target/ppc: Introduce ppc_hash64_use_vrma() helper

[Cédric Le Goater]

On 1/7/20 5:48 AM, David Gibson wrote:
> When running guests under a hypervisor, the hypervisor obviously needs to
> be protected from guest accesses even if those are in what the guest
> considers real mode (translation off).  The POWER hardware provides two
> ways of doing that: The old way has guest real mode accesses simply offset
> and bounds checked into host addresses.  It works, but requires that a
> significant chunk of the guest's memory - the RMA - be physically
> contiguous in the host, which is pretty inconvenient.  The new way, known
> as VRMA, has guest real mode accesses translated in roughly the normal way
> but with some special parameters.
> 
> In POWER7 and POWER8 the LPCR[VPM0] bit selected between the two modes, but
> in POWER9 only VRMA mode is supported and LPCR[VPM0] no longer exists.  We
> handle that difference in behaviour in ppc_hash64_set_isi().. but not in
> other places that we blindly check LPCR[VPM0].
> 
> Correct those instances with a new helper to tell if we should be in VRMA
> mode.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  target/ppc/mmu-hash64.c | 41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 5fabd93c92..d878180df5 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -668,6 +668,19 @@ unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
>      return 0;
>  }
>  
> +static bool ppc_hash64_use_vrma(CPUPPCState *env)
> +{
> +    switch (env->mmu_model) {
> +    case POWERPC_MMU_3_00:
> +        /* ISAv3.0 (POWER9) always uses VRMA, the VPM0 field and RMOR
> +         * register no longer exist */
> +        return true;
> +
> +    default:
> +        return !!(env->spr[SPR_LPCR] & LPCR_VPM0);
> +    }
> +}
> +
>  static void ppc_hash64_set_isi(CPUState *cs, uint64_t error_code)
>  {
>      CPUPPCState *env = &POWERPC_CPU(cs)->env;
> @@ -676,15 +689,7 @@ static void ppc_hash64_set_isi(CPUState *cs, uint64_t error_code)
>      if (msr_ir) {
>          vpm = !!(env->spr[SPR_LPCR] & LPCR_VPM1);
>      } else {
> -        switch (env->mmu_model) {
> -        case POWERPC_MMU_3_00:
> -            /* Field deprecated in ISAv3.00 - interrupts always go to hyperv */
> -            vpm = true;
> -            break;
> -        default:
> -            vpm = !!(env->spr[SPR_LPCR] & LPCR_VPM0);
> -            break;
> -        }
> +        vpm = ppc_hash64_use_vrma(env);
>      }
>      if (vpm && !msr_hv) {
>          cs->exception_index = POWERPC_EXCP_HISI;
> @@ -702,15 +707,7 @@ static void ppc_hash64_set_dsi(CPUState *cs, uint64_t dar, uint64_t dsisr)
>      if (msr_dr) {
>          vpm = !!(env->spr[SPR_LPCR] & LPCR_VPM1);
>      } else {
> -        switch (env->mmu_model) {
> -        case POWERPC_MMU_3_00:
> -            /* Field deprecated in ISAv3.00 - interrupts always go to hyperv */
> -            vpm = true;
> -            break;
> -        default:
> -            vpm = !!(env->spr[SPR_LPCR] & LPCR_VPM0);
> -            break;
> -        }
> +        vpm = ppc_hash64_use_vrma(env);
>      }
>      if (vpm && !msr_hv) {
>          cs->exception_index = POWERPC_EXCP_HDSI;
> @@ -799,7 +796,7 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
>              if (!(eaddr >> 63)) {
>                  raddr |= env->spr[SPR_HRMOR];
>              }
> -        } else if (env->spr[SPR_LPCR] & LPCR_VPM0) {
> +        } else if (ppc_hash64_use_vrma(env)) {
>              /* Emulated VRMA mode */
>              slb = &env->vrma_slb;
>              if (!slb->sps) {
> @@ -967,7 +964,7 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
>          } else if ((msr_hv || !env->has_hv_mode) && !(addr >> 63)) {
>              /* In HV mode, add HRMOR if top EA bit is clear */
>              return raddr | env->spr[SPR_HRMOR];
> -        } else if (env->spr[SPR_LPCR] & LPCR_VPM0) {
> +        } else if (ppc_hash64_use_vrma(env)) {
>              /* Emulated VRMA mode */
>              slb = &env->vrma_slb;
>              if (!slb->sps) {
> @@ -1056,8 +1053,7 @@ static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
>      slb->sps = NULL;
>  
>      /* Is VRMA enabled ? */
> -    lpcr = env->spr[SPR_LPCR];
> -    if (!(lpcr & LPCR_VPM0)) {
> +    if (ppc_hash64_use_vrma(env)) {
>          return;
>      }
>  
> @@ -1065,6 +1061,7 @@ static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
>       * Make one up. Mostly ignore the ESID which will not be needed
>       * for translation
>       */
> +    lpcr = env->spr[SPR_LPCR];
>      vsid = SLB_VSID_VRMA;
>      vrmasd = (lpcr & LPCR_VRMASD) >> LPCR_VRMASD_SHIFT;
>      vsid |= (vrmasd << 4) & (SLB_VSID_L | SLB_VSID_LP);
> 

[PATCH v2 05/10] spapr, ppc: Remove VPM0/RMLS hacks for POWER9

[David Gibson]

For the "pseries" machine, we use "virtual hypervisor" mode where we only
model the CPU in non-hypervisor privileged mode.  This means that we need
guest physical addresses within the modelled within the cpu to be treated
as absolute physical addresses.

We used to do that by clearing LPCR[VPM0] and setting LPCR[RMLS] to a high
limit so that the old offset based translation for guest mode applied,
which does what we need.  However, POWER9 has removed support for that
translation mode, which meant we had some ugly hacks to keep it working.

We now explicitly handle this sort of translation for virtual hypervisor
mode, so the hacks aren't necessary.  We don't need to set VPM0 and RMLS
from the machine type code - they're now ignored in vhyp mode.  On the cpu
side we don't need to allow LPCR[RMLS] to be set on POWER9 in vhyp mode -
that was only there to allow the hack on the machine side.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_cpu_core.c | 6 +-----
 target/ppc/mmu-hash64.c | 8 --------
 2 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 8339c4c0f8..daebcd9f38 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -58,14 +58,10 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
      * we don't get spurious wakups before an RTAS start-cpu call.
      * For the same reason, set PSSCR_EC.
      */
-    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV | pcc->lpcr_pm);
+    lpcr &= ~(LPCR_VPM1 | LPCR_ISL | LPCR_KBV | pcc->lpcr_pm);
     lpcr |= LPCR_LPES0 | LPCR_LPES1;
     env->spr[SPR_PSSCR] |= PSSCR_EC;
 
-    /* Set RMLS to the max (ie, 16G) */
-    lpcr &= ~LPCR_RMLS;
-    lpcr |= 1ull << LPCR_RMLS_SHIFT;
-
     ppc_store_lpcr(cpu, lpcr);
 
     /* Set a full AMOR so guest can use the AMR as it sees fit */
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index d878180df5..d7f9933e6d 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -1124,14 +1124,6 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
                       (LPCR_PECE_L_MASK & (LPCR_PDEE | LPCR_HDEE | LPCR_EEE |
                       LPCR_DEE | LPCR_OEE)) | LPCR_MER | LPCR_GTSE | LPCR_TC |
                       LPCR_HEIC | LPCR_LPES0 | LPCR_HVICE | LPCR_HDICE);
-        /*
-         * If we have a virtual hypervisor, we need to bring back RMLS. It
-         * doesn't exist on an actual P9 but that's all we know how to
-         * configure with softmmu at the moment
-         */
-        if (cpu->vhyp) {
-            lpcr |= (val & LPCR_RMLS);
-        }
         break;
     default:
         g_assert_not_reached();
-- 
2.24.1

Re: [PATCH v2 05/10] spapr, ppc: Remove VPM0/RMLS hacks for POWER9

[Cédric Le Goater]

On 1/7/20 5:48 AM, David Gibson wrote:
> For the "pseries" machine, we use "virtual hypervisor" mode where we only
> model the CPU in non-hypervisor privileged mode.  This means that we need
> guest physical addresses within the modelled within the cpu to be treated
> as absolute physical addresses.
> 
> We used to do that by clearing LPCR[VPM0] and setting LPCR[RMLS] to a high
> limit so that the old offset based translation for guest mode applied,
> which does what we need.  However, POWER9 has removed support for that
> translation mode, which meant we had some ugly hacks to keep it working.
> 
> We now explicitly handle this sort of translation for virtual hypervisor
> mode, so the hacks aren't necessary.  We don't need to set VPM0 and RMLS
> from the machine type code - they're now ignored in vhyp mode.  On the cpu
> side we don't need to allow LPCR[RMLS] to be set on POWER9 in vhyp mode -
> that was only there to allow the hack on the machine side.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/spapr_cpu_core.c | 6 +-----
>  target/ppc/mmu-hash64.c | 8 --------
>  2 files changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 8339c4c0f8..daebcd9f38 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -58,14 +58,10 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>       * we don't get spurious wakups before an RTAS start-cpu call.
>       * For the same reason, set PSSCR_EC.
>       */
> -    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV | pcc->lpcr_pm);
> +    lpcr &= ~(LPCR_VPM1 | LPCR_ISL | LPCR_KBV | pcc->lpcr_pm);
>      lpcr |= LPCR_LPES0 | LPCR_LPES1;
>      env->spr[SPR_PSSCR] |= PSSCR_EC;
>  
> -    /* Set RMLS to the max (ie, 16G) */
> -    lpcr &= ~LPCR_RMLS;
> -    lpcr |= 1ull << LPCR_RMLS_SHIFT;
> -
>      ppc_store_lpcr(cpu, lpcr);
>  
>      /* Set a full AMOR so guest can use the AMR as it sees fit */
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index d878180df5..d7f9933e6d 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -1124,14 +1124,6 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>                        (LPCR_PECE_L_MASK & (LPCR_PDEE | LPCR_HDEE | LPCR_EEE |
>                        LPCR_DEE | LPCR_OEE)) | LPCR_MER | LPCR_GTSE | LPCR_TC |
>                        LPCR_HEIC | LPCR_LPES0 | LPCR_HVICE | LPCR_HDICE);
> -        /*
> -         * If we have a virtual hypervisor, we need to bring back RMLS. It
> -         * doesn't exist on an actual P9 but that's all we know how to
> -         * configure with softmmu at the moment
> -         */
> -        if (cpu->vhyp) {
> -            lpcr |= (val & LPCR_RMLS);
> -        }
>          break;
>      default:
>          g_assert_not_reached();
> 

Re: [PATCH v2 05/10] spapr, ppc: Remove VPM0/RMLS hacks for POWER9

[Alexey Kardashevskiy]

On 07/01/2020 15:48, David Gibson wrote:
> For the "pseries" machine, we use "virtual hypervisor" mode where we only
> model the CPU in non-hypervisor privileged mode.  This means that we need
> guest physical addresses within the modelled within the cpu to be treated
> as absolute physical addresses.


s/within the modelled within the cpu/within the modelled cpu/ ? Thanks,


> We used to do that by clearing LPCR[VPM0] and setting LPCR[RMLS] to a high
> limit so that the old offset based translation for guest mode applied,
> which does what we need.  However, POWER9 has removed support for that
> translation mode, which meant we had some ugly hacks to keep it working.
> 
> We now explicitly handle this sort of translation for virtual hypervisor
> mode, so the hacks aren't necessary.  We don't need to set VPM0 and RMLS
> from the machine type code - they're now ignored in vhyp mode.  On the cpu
> side we don't need to allow LPCR[RMLS] to be set on POWER9 in vhyp mode -
> that was only there to allow the hack on the machine side.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_cpu_core.c | 6 +-----
>  target/ppc/mmu-hash64.c | 8 --------
>  2 files changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 8339c4c0f8..daebcd9f38 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -58,14 +58,10 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>       * we don't get spurious wakups before an RTAS start-cpu call.
>       * For the same reason, set PSSCR_EC.
>       */
> -    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV | pcc->lpcr_pm);
> +    lpcr &= ~(LPCR_VPM1 | LPCR_ISL | LPCR_KBV | pcc->lpcr_pm);
>      lpcr |= LPCR_LPES0 | LPCR_LPES1;
>      env->spr[SPR_PSSCR] |= PSSCR_EC;
>  
> -    /* Set RMLS to the max (ie, 16G) */
> -    lpcr &= ~LPCR_RMLS;
> -    lpcr |= 1ull << LPCR_RMLS_SHIFT;
> -
>      ppc_store_lpcr(cpu, lpcr);
>  
>      /* Set a full AMOR so guest can use the AMR as it sees fit */
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index d878180df5..d7f9933e6d 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -1124,14 +1124,6 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>                        (LPCR_PECE_L_MASK & (LPCR_PDEE | LPCR_HDEE | LPCR_EEE |
>                        LPCR_DEE | LPCR_OEE)) | LPCR_MER | LPCR_GTSE | LPCR_TC |
>                        LPCR_HEIC | LPCR_LPES0 | LPCR_HVICE | LPCR_HDICE);
> -        /*
> -         * If we have a virtual hypervisor, we need to bring back RMLS. It
> -         * doesn't exist on an actual P9 but that's all we know how to
> -         * configure with softmmu at the moment
> -         */
> -        if (cpu->vhyp) {
> -            lpcr |= (val & LPCR_RMLS);
> -        }
>          break;
>      default:
>          g_assert_not_reached();
> 

-- 
Alexey

Re: [PATCH v2 05/10] spapr, ppc: Remove VPM0/RMLS hacks for POWER9

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

On Thu, Jan 09, 2020 at 06:33:36PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 07/01/2020 15:48, David Gibson wrote:
> > For the "pseries" machine, we use "virtual hypervisor" mode where we only
> > model the CPU in non-hypervisor privileged mode.  This means that we need
> > guest physical addresses within the modelled within the cpu to be treated
> > as absolute physical addresses.
> 
> 
> s/within the modelled within the cpu/within the modelled cpu/ ?
> Thanks,

Thanks.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 06/10] target/ppc: Remove RMOR register from POWER9 & POWER10

[David Gibson]

Currently we create the Real Mode Offset Register (RMOR) on all Book3S cpus
from POWER7 onwards.  However the translation mode which the RMOR controls
is no longer supported in POWER9, and so the register has been removed from
the architecture.

Remove it from our model on POWER9 and POWER10.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/translate_init.inc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 436d0d5a51..893fb12e90 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -8003,12 +8003,16 @@ static void gen_spr_book3s_ids(CPUPPCState *env)
                  SPR_NOACCESS, SPR_NOACCESS,
                  &spr_read_generic, &spr_write_generic,
                  0x00000000);
-    spr_register_hv(env, SPR_RMOR, "RMOR",
+    spr_register_hv(env, SPR_HRMOR, "HRMOR",
                  SPR_NOACCESS, SPR_NOACCESS,
                  SPR_NOACCESS, SPR_NOACCESS,
                  &spr_read_generic, &spr_write_generic,
                  0x00000000);
-    spr_register_hv(env, SPR_HRMOR, "HRMOR",
+}
+
+static void gen_spr_rmor(CPUPPCState *env)
+{
+    spr_register_hv(env, SPR_RMOR, "RMOR",
                  SPR_NOACCESS, SPR_NOACCESS,
                  SPR_NOACCESS, SPR_NOACCESS,
                  &spr_read_generic, &spr_write_generic,
@@ -8522,6 +8526,7 @@ static void init_proc_POWER7(CPUPPCState *env)
 
     /* POWER7 Specific Registers */
     gen_spr_book3s_ids(env);
+    gen_spr_rmor(env);
     gen_spr_amr(env);
     gen_spr_book3s_purr(env);
     gen_spr_power5p_common(env);
@@ -8663,6 +8668,7 @@ static void init_proc_POWER8(CPUPPCState *env)
 
     /* POWER8 Specific Registers */
     gen_spr_book3s_ids(env);
+    gen_spr_rmor(env);
     gen_spr_amr(env);
     gen_spr_iamr(env);
     gen_spr_book3s_purr(env);
-- 
2.24.1

Re: [PATCH v2 06/10] target/ppc: Remove RMOR register from POWER9 & POWER10

[Cédric Le Goater]

On 1/7/20 5:48 AM, David Gibson wrote:
> Currently we create the Real Mode Offset Register (RMOR) on all Book3S cpus
> from POWER7 onwards.  However the translation mode which the RMOR controls
> is no longer supported in POWER9, and so the register has been removed from
> the architecture.
> 
> Remove it from our model on POWER9 and POWER10.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>


> ---
>  target/ppc/translate_init.inc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 436d0d5a51..893fb12e90 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -8003,12 +8003,16 @@ static void gen_spr_book3s_ids(CPUPPCState *env)
>                   SPR_NOACCESS, SPR_NOACCESS,
>                   &spr_read_generic, &spr_write_generic,
>                   0x00000000);
> -    spr_register_hv(env, SPR_RMOR, "RMOR",
> +    spr_register_hv(env, SPR_HRMOR, "HRMOR",
>                   SPR_NOACCESS, SPR_NOACCESS,
>                   SPR_NOACCESS, SPR_NOACCESS,
>                   &spr_read_generic, &spr_write_generic,
>                   0x00000000);
> -    spr_register_hv(env, SPR_HRMOR, "HRMOR",
> +}
> +
> +static void gen_spr_rmor(CPUPPCState *env)
> +{
> +    spr_register_hv(env, SPR_RMOR, "RMOR",
>                   SPR_NOACCESS, SPR_NOACCESS,
>                   SPR_NOACCESS, SPR_NOACCESS,
>                   &spr_read_generic, &spr_write_generic,
> @@ -8522,6 +8526,7 @@ static void init_proc_POWER7(CPUPPCState *env)
>  
>      /* POWER7 Specific Registers */
>      gen_spr_book3s_ids(env);
> +    gen_spr_rmor(env);
>      gen_spr_amr(env);
>      gen_spr_book3s_purr(env);
>      gen_spr_power5p_common(env);
> @@ -8663,6 +8668,7 @@ static void init_proc_POWER8(CPUPPCState *env)
>  
>      /* POWER8 Specific Registers */
>      gen_spr_book3s_ids(env);
> +    gen_spr_rmor(env);
>      gen_spr_amr(env);
>      gen_spr_iamr(env);
>      gen_spr_book3s_purr(env);
> 

[PATCH v2 07/10] target/ppc: Use class fields to simplify LPCR masking

[David Gibson]

When we store the Logical Partitioning Control Register (LPCR) we have a
big switch statement to work out which are valid bits for the cpu model
we're emulating.

As well as being ugly, this isn't really conceptually correct, since it is
based on the mmu_model variable, whereas the LPCR isn't (only) about the
MMU, so mmu_model is basically just acting as a proxy for the cpu model.

Handle this in a simpler way, by adding a suitable lpcr_mask to the QOM
class.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/cpu-qom.h            |  1 +
 target/ppc/mmu-hash64.c         | 37 ++-------------------------------
 target/ppc/translate_init.inc.c | 27 ++++++++++++++++++++----
 3 files changed, 26 insertions(+), 39 deletions(-)

diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
index e499575dc8..15d6b54a7d 100644
--- a/target/ppc/cpu-qom.h
+++ b/target/ppc/cpu-qom.h
@@ -177,6 +177,7 @@ typedef struct PowerPCCPUClass {
     uint64_t insns_flags;
     uint64_t insns_flags2;
     uint64_t msr_mask;
+    uint64_t lpcr_mask;         /* Available bits in the LPCR */
     uint64_t lpcr_pm;           /* Power-saving mode Exit Cause Enable bits */
     powerpc_mmu_t   mmu_model;
     powerpc_excp_t  excp_model;
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index d7f9933e6d..127b7250ae 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -1093,43 +1093,10 @@ static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
 
 void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
 {
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
-    uint64_t lpcr = 0;
 
-    /* Filter out bits */
-    switch (env->mmu_model) {
-    case POWERPC_MMU_2_03: /* P5p */
-        lpcr = val & (LPCR_RMLS | LPCR_ILE |
-                      LPCR_LPES0 | LPCR_LPES1 |
-                      LPCR_RMI | LPCR_HDICE);
-        break;
-    case POWERPC_MMU_2_06: /* P7 */
-        lpcr = val & (LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_DPFD |
-                      LPCR_VRMASD | LPCR_RMLS | LPCR_ILE |
-                      LPCR_P7_PECE0 | LPCR_P7_PECE1 | LPCR_P7_PECE2 |
-                      LPCR_MER | LPCR_TC |
-                      LPCR_LPES0 | LPCR_LPES1 | LPCR_HDICE);
-        break;
-    case POWERPC_MMU_2_07: /* P8 */
-        lpcr = val & (LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV |
-                      LPCR_DPFD | LPCR_VRMASD | LPCR_RMLS | LPCR_ILE |
-                      LPCR_AIL | LPCR_ONL | LPCR_P8_PECE0 | LPCR_P8_PECE1 |
-                      LPCR_P8_PECE2 | LPCR_P8_PECE3 | LPCR_P8_PECE4 |
-                      LPCR_MER | LPCR_TC | LPCR_LPES0 | LPCR_HDICE);
-        break;
-    case POWERPC_MMU_3_00: /* P9 */
-        lpcr = val & (LPCR_VPM1 | LPCR_ISL | LPCR_KBV | LPCR_DPFD |
-                      (LPCR_PECE_U_MASK & LPCR_HVEE) | LPCR_ILE | LPCR_AIL |
-                      LPCR_UPRT | LPCR_EVIRT | LPCR_ONL | LPCR_HR | LPCR_LD |
-                      (LPCR_PECE_L_MASK & (LPCR_PDEE | LPCR_HDEE | LPCR_EEE |
-                      LPCR_DEE | LPCR_OEE)) | LPCR_MER | LPCR_GTSE | LPCR_TC |
-                      LPCR_HEIC | LPCR_LPES0 | LPCR_HVICE | LPCR_HDICE);
-        break;
-    default:
-        g_assert_not_reached();
-        ;
-    }
-    env->spr[SPR_LPCR] = lpcr;
+    env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
     ppc_hash64_update_rmls(cpu);
     ppc_hash64_update_vrma(cpu);
 }
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 893fb12e90..240ac00506 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -8463,6 +8463,8 @@ POWERPC_FAMILY(POWER5P)(ObjectClass *oc, void *data)
                     (1ull << MSR_DR) |
                     (1ull << MSR_PMM) |
                     (1ull << MSR_RI);
+    pcc->lpcr_mask = LPCR_RMLS | LPCR_ILE | LPCR_LPES0 | LPCR_LPES1 |
+        LPCR_RMI | LPCR_HDICE;
     pcc->mmu_model = POWERPC_MMU_2_03;
 #if defined(CONFIG_SOFTMMU)
     pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault;
@@ -8640,6 +8642,12 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
                     (1ull << MSR_PMM) |
                     (1ull << MSR_RI) |
                     (1ull << MSR_LE);
+    pcc->lpcr_mask = LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_DPFD |
+        LPCR_VRMASD | LPCR_RMLS | LPCR_ILE |
+        LPCR_P7_PECE0 | LPCR_P7_PECE1 | LPCR_P7_PECE2 |
+        LPCR_MER | LPCR_TC |
+        LPCR_LPES0 | LPCR_LPES1 | LPCR_HDICE;
+    pcc->lpcr_pm = LPCR_P7_PECE0 | LPCR_P7_PECE1 | LPCR_P7_PECE2;
     pcc->mmu_model = POWERPC_MMU_2_06;
 #if defined(CONFIG_SOFTMMU)
     pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault;
@@ -8656,7 +8664,6 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
     pcc->l1_dcache_size = 0x8000;
     pcc->l1_icache_size = 0x8000;
     pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
-    pcc->lpcr_pm = LPCR_P7_PECE0 | LPCR_P7_PECE1 | LPCR_P7_PECE2;
 }
 
 static void init_proc_POWER8(CPUPPCState *env)
@@ -8812,6 +8819,13 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
                     (1ull << MSR_TS0) |
                     (1ull << MSR_TS1) |
                     (1ull << MSR_LE);
+    pcc->lpcr_mask = LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV |
+        LPCR_DPFD | LPCR_VRMASD | LPCR_RMLS | LPCR_ILE |
+        LPCR_AIL | LPCR_ONL | LPCR_P8_PECE0 | LPCR_P8_PECE1 |
+        LPCR_P8_PECE2 | LPCR_P8_PECE3 | LPCR_P8_PECE4 |
+        LPCR_MER | LPCR_TC | LPCR_LPES0 | LPCR_HDICE;
+    pcc->lpcr_pm = LPCR_P8_PECE0 | LPCR_P8_PECE1 | LPCR_P8_PECE2 |
+                   LPCR_P8_PECE3 | LPCR_P8_PECE4;
     pcc->mmu_model = POWERPC_MMU_2_07;
 #if defined(CONFIG_SOFTMMU)
     pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault;
@@ -8829,8 +8843,6 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
     pcc->l1_dcache_size = 0x8000;
     pcc->l1_icache_size = 0x8000;
     pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
-    pcc->lpcr_pm = LPCR_P8_PECE0 | LPCR_P8_PECE1 | LPCR_P8_PECE2 |
-                   LPCR_P8_PECE3 | LPCR_P8_PECE4;
 }
 
 #ifdef CONFIG_SOFTMMU
@@ -9023,6 +9035,14 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
                     (1ull << MSR_PMM) |
                     (1ull << MSR_RI) |
                     (1ull << MSR_LE);
+    pcc->lpcr_mask = LPCR_VPM1 | LPCR_ISL | LPCR_KBV | LPCR_DPFD |
+        (LPCR_PECE_U_MASK & LPCR_HVEE) | LPCR_ILE | LPCR_AIL |
+        LPCR_UPRT | LPCR_EVIRT | LPCR_ONL | LPCR_HR | LPCR_LD |
+        (LPCR_PECE_L_MASK & (LPCR_PDEE | LPCR_HDEE | LPCR_EEE |
+                             LPCR_DEE | LPCR_OEE))
+        | LPCR_MER | LPCR_GTSE | LPCR_TC |
+        LPCR_HEIC | LPCR_LPES0 | LPCR_HVICE | LPCR_HDICE;
+    pcc->lpcr_pm = LPCR_PDEE | LPCR_HDEE | LPCR_EEE | LPCR_DEE | LPCR_OEE;
     pcc->mmu_model = POWERPC_MMU_3_00;
 #if defined(CONFIG_SOFTMMU)
     pcc->handle_mmu_fault = ppc64_v3_handle_mmu_fault;
@@ -9042,7 +9062,6 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
     pcc->l1_dcache_size = 0x8000;
     pcc->l1_icache_size = 0x8000;
     pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
-    pcc->lpcr_pm = LPCR_PDEE | LPCR_HDEE | LPCR_EEE | LPCR_DEE | LPCR_OEE;
 }
 
 #ifdef CONFIG_SOFTMMU
-- 
2.24.1

Re: [PATCH v2 07/10] target/ppc: Use class fields to simplify LPCR masking

[Cédric Le Goater]

On 1/7/20 5:48 AM, David Gibson wrote:
> When we store the Logical Partitioning Control Register (LPCR) we have a
> big switch statement to work out which are valid bits for the cpu model
> we're emulating.
> 
> As well as being ugly, this isn't really conceptually correct, since it is
> based on the mmu_model variable, whereas the LPCR isn't (only) about the
> MMU, so mmu_model is basically just acting as a proxy for the cpu model.
> 
> Handle this in a simpler way, by adding a suitable lpcr_mask to the QOM
> class.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  target/ppc/cpu-qom.h            |  1 +
>  target/ppc/mmu-hash64.c         | 37 ++-------------------------------
>  target/ppc/translate_init.inc.c | 27 ++++++++++++++++++++----
>  3 files changed, 26 insertions(+), 39 deletions(-)
> 
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index e499575dc8..15d6b54a7d 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -177,6 +177,7 @@ typedef struct PowerPCCPUClass {
>      uint64_t insns_flags;
>      uint64_t insns_flags2;
>      uint64_t msr_mask;
> +    uint64_t lpcr_mask;         /* Available bits in the LPCR */
>      uint64_t lpcr_pm;           /* Power-saving mode Exit Cause Enable bits */
>      powerpc_mmu_t   mmu_model;
>      powerpc_excp_t  excp_model;
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index d7f9933e6d..127b7250ae 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -1093,43 +1093,10 @@ static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
>  
>  void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>  {
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      CPUPPCState *env = &cpu->env;
> -    uint64_t lpcr = 0;
>  
> -    /* Filter out bits */
> -    switch (env->mmu_model) {
> -    case POWERPC_MMU_2_03: /* P5p */
> -        lpcr = val & (LPCR_RMLS | LPCR_ILE |
> -                      LPCR_LPES0 | LPCR_LPES1 |
> -                      LPCR_RMI | LPCR_HDICE);
> -        break;
> -    case POWERPC_MMU_2_06: /* P7 */
> -        lpcr = val & (LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_DPFD |
> -                      LPCR_VRMASD | LPCR_RMLS | LPCR_ILE |
> -                      LPCR_P7_PECE0 | LPCR_P7_PECE1 | LPCR_P7_PECE2 |
> -                      LPCR_MER | LPCR_TC |
> -                      LPCR_LPES0 | LPCR_LPES1 | LPCR_HDICE);
> -        break;
> -    case POWERPC_MMU_2_07: /* P8 */
> -        lpcr = val & (LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV |
> -                      LPCR_DPFD | LPCR_VRMASD | LPCR_RMLS | LPCR_ILE |
> -                      LPCR_AIL | LPCR_ONL | LPCR_P8_PECE0 | LPCR_P8_PECE1 |
> -                      LPCR_P8_PECE2 | LPCR_P8_PECE3 | LPCR_P8_PECE4 |
> -                      LPCR_MER | LPCR_TC | LPCR_LPES0 | LPCR_HDICE);
> -        break;
> -    case POWERPC_MMU_3_00: /* P9 */
> -        lpcr = val & (LPCR_VPM1 | LPCR_ISL | LPCR_KBV | LPCR_DPFD |
> -                      (LPCR_PECE_U_MASK & LPCR_HVEE) | LPCR_ILE | LPCR_AIL |
> -                      LPCR_UPRT | LPCR_EVIRT | LPCR_ONL | LPCR_HR | LPCR_LD |
> -                      (LPCR_PECE_L_MASK & (LPCR_PDEE | LPCR_HDEE | LPCR_EEE |
> -                      LPCR_DEE | LPCR_OEE)) | LPCR_MER | LPCR_GTSE | LPCR_TC |
> -                      LPCR_HEIC | LPCR_LPES0 | LPCR_HVICE | LPCR_HDICE);
> -        break;
> -    default:
> -        g_assert_not_reached();
> -        ;
> -    }
> -    env->spr[SPR_LPCR] = lpcr;
> +    env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
>      ppc_hash64_update_rmls(cpu);
>      ppc_hash64_update_vrma(cpu);
>  }
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 893fb12e90..240ac00506 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -8463,6 +8463,8 @@ POWERPC_FAMILY(POWER5P)(ObjectClass *oc, void *data)
>                      (1ull << MSR_DR) |
>                      (1ull << MSR_PMM) |
>                      (1ull << MSR_RI);
> +    pcc->lpcr_mask = LPCR_RMLS | LPCR_ILE | LPCR_LPES0 | LPCR_LPES1 |
> +        LPCR_RMI | LPCR_HDICE;
>      pcc->mmu_model = POWERPC_MMU_2_03;
>  #if defined(CONFIG_SOFTMMU)
>      pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault;
> @@ -8640,6 +8642,12 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>                      (1ull << MSR_PMM) |
>                      (1ull << MSR_RI) |
>                      (1ull << MSR_LE);
> +    pcc->lpcr_mask = LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_DPFD |
> +        LPCR_VRMASD | LPCR_RMLS | LPCR_ILE |
> +        LPCR_P7_PECE0 | LPCR_P7_PECE1 | LPCR_P7_PECE2 |
> +        LPCR_MER | LPCR_TC |
> +        LPCR_LPES0 | LPCR_LPES1 | LPCR_HDICE;
> +    pcc->lpcr_pm = LPCR_P7_PECE0 | LPCR_P7_PECE1 | LPCR_P7_PECE2;
>      pcc->mmu_model = POWERPC_MMU_2_06;
>  #if defined(CONFIG_SOFTMMU)
>      pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault;
> @@ -8656,7 +8664,6 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>      pcc->l1_dcache_size = 0x8000;
>      pcc->l1_icache_size = 0x8000;
>      pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
> -    pcc->lpcr_pm = LPCR_P7_PECE0 | LPCR_P7_PECE1 | LPCR_P7_PECE2;
>  }
>  
>  static void init_proc_POWER8(CPUPPCState *env)
> @@ -8812,6 +8819,13 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>                      (1ull << MSR_TS0) |
>                      (1ull << MSR_TS1) |
>                      (1ull << MSR_LE);
> +    pcc->lpcr_mask = LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV |
> +        LPCR_DPFD | LPCR_VRMASD | LPCR_RMLS | LPCR_ILE |
> +        LPCR_AIL | LPCR_ONL | LPCR_P8_PECE0 | LPCR_P8_PECE1 |
> +        LPCR_P8_PECE2 | LPCR_P8_PECE3 | LPCR_P8_PECE4 |
> +        LPCR_MER | LPCR_TC | LPCR_LPES0 | LPCR_HDICE;
> +    pcc->lpcr_pm = LPCR_P8_PECE0 | LPCR_P8_PECE1 | LPCR_P8_PECE2 |
> +                   LPCR_P8_PECE3 | LPCR_P8_PECE4;
>      pcc->mmu_model = POWERPC_MMU_2_07;
>  #if defined(CONFIG_SOFTMMU)
>      pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault;
> @@ -8829,8 +8843,6 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>      pcc->l1_dcache_size = 0x8000;
>      pcc->l1_icache_size = 0x8000;
>      pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
> -    pcc->lpcr_pm = LPCR_P8_PECE0 | LPCR_P8_PECE1 | LPCR_P8_PECE2 |
> -                   LPCR_P8_PECE3 | LPCR_P8_PECE4;
>  }
>  
>  #ifdef CONFIG_SOFTMMU
> @@ -9023,6 +9035,14 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
>                      (1ull << MSR_PMM) |
>                      (1ull << MSR_RI) |
>                      (1ull << MSR_LE);
> +    pcc->lpcr_mask = LPCR_VPM1 | LPCR_ISL | LPCR_KBV | LPCR_DPFD |
> +        (LPCR_PECE_U_MASK & LPCR_HVEE) | LPCR_ILE | LPCR_AIL |
> +        LPCR_UPRT | LPCR_EVIRT | LPCR_ONL | LPCR_HR | LPCR_LD |
> +        (LPCR_PECE_L_MASK & (LPCR_PDEE | LPCR_HDEE | LPCR_EEE |
> +                             LPCR_DEE | LPCR_OEE))
> +        | LPCR_MER | LPCR_GTSE | LPCR_TC |
> +        LPCR_HEIC | LPCR_LPES0 | LPCR_HVICE | LPCR_HDICE;
> +    pcc->lpcr_pm = LPCR_PDEE | LPCR_HDEE | LPCR_EEE | LPCR_DEE | LPCR_OEE;
>      pcc->mmu_model = POWERPC_MMU_3_00;
>  #if defined(CONFIG_SOFTMMU)
>      pcc->handle_mmu_fault = ppc64_v3_handle_mmu_fault;
> @@ -9042,7 +9062,6 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
>      pcc->l1_dcache_size = 0x8000;
>      pcc->l1_icache_size = 0x8000;
>      pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
> -    pcc->lpcr_pm = LPCR_PDEE | LPCR_HDEE | LPCR_EEE | LPCR_DEE | LPCR_OEE;
>  }
>  
>  #ifdef CONFIG_SOFTMMU
> 

[PATCH v2 08/10] target/ppc: Streamline calculation of RMA limit from LPCR[RMLS]

[David Gibson]

Currently we use a big switch statement in ppc_hash64_update_rmls() to work
out what the right RMA limit is based on the LPCR[RMLS] field.  There's no
formula for this - it's just an arbitrary mapping defined by the existing
CPU implementations - but we can make it a bit more readable by using a
lookup table rather than a switch.  In addition we can use the MiB/GiB
symbols to make it a bit clearer.

While there we add a bit of clarity and rationale to the comment about
what happens if the LPCR[RMLS] doesn't contain a valid value.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/mmu-hash64.c | 71 ++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 127b7250ae..bb9ebeaf48 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -18,6 +18,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "cpu.h"
 #include "exec/exec-all.h"
 #include "exec/helper-proto.h"
@@ -755,6 +756,39 @@ static void ppc_hash64_set_c(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
     stb_phys(CPU(cpu)->as, base + offset, (pte1 & 0xff) | 0x80);
 }
 
+static target_ulong rmls_limit(PowerPCCPU *cpu)
+{
+    CPUPPCState *env = &cpu->env;
+    /*
+     * This is the full 4 bits encoding of POWER8. Previous
+     * CPUs only support a subset of these but the filtering
+     * is done when writing LPCR
+     */
+    const target_ulong rma_sizes[] = {
+        [0] = 0,
+        [1] = 16 * GiB,
+        [2] = 1 * GiB,
+        [3] = 64 * MiB,
+        [4] = 256 * MiB,
+        [5] = 0,
+        [6] = 0,
+        [7] = 128 * MiB,
+        [8] = 32 * MiB,
+    };
+    target_ulong rmls = (env->spr[SPR_LPCR] & LPCR_RMLS) >> LPCR_RMLS_SHIFT;
+
+    if (rmls < ARRAY_SIZE(rma_sizes)) {
+        return rma_sizes[rmls];
+    } else {
+        /*
+         * Bad value, so the OS has shot itself in the foot.  Return a
+         * 0-sized RMA which we expect to trigger an immediate DSI or
+         * ISI
+         */
+        return 0;
+    }
+}
+
 int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
                                 int rwx, int mmu_idx)
 {
@@ -1004,41 +1038,6 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
     cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
 }
 
-static void ppc_hash64_update_rmls(PowerPCCPU *cpu)
-{
-    CPUPPCState *env = &cpu->env;
-    uint64_t lpcr = env->spr[SPR_LPCR];
-
-    /*
-     * This is the full 4 bits encoding of POWER8. Previous
-     * CPUs only support a subset of these but the filtering
-     * is done when writing LPCR
-     */
-    switch ((lpcr & LPCR_RMLS) >> LPCR_RMLS_SHIFT) {
-    case 0x8: /* 32MB */
-        env->rmls = 0x2000000ull;
-        break;
-    case 0x3: /* 64MB */
-        env->rmls = 0x4000000ull;
-        break;
-    case 0x7: /* 128MB */
-        env->rmls = 0x8000000ull;
-        break;
-    case 0x4: /* 256MB */
-        env->rmls = 0x10000000ull;
-        break;
-    case 0x2: /* 1GB */
-        env->rmls = 0x40000000ull;
-        break;
-    case 0x1: /* 16GB */
-        env->rmls = 0x400000000ull;
-        break;
-    default:
-        /* What to do here ??? */
-        env->rmls = 0;
-    }
-}
-
 static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
@@ -1097,7 +1096,7 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
     CPUPPCState *env = &cpu->env;
 
     env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
-    ppc_hash64_update_rmls(cpu);
+    env->rmls = rmls_limit(cpu);
     ppc_hash64_update_vrma(cpu);
 }
 
-- 
2.24.1

Re: [PATCH v2 08/10] target/ppc: Streamline calculation of RMA limit from LPCR[RMLS]

[Cédric Le Goater]

On 1/7/20 5:48 AM, David Gibson wrote:
> Currently we use a big switch statement in ppc_hash64_update_rmls() to work
> out what the right RMA limit is based on the LPCR[RMLS] field.  There's no
> formula for this - it's just an arbitrary mapping defined by the existing
> CPU implementations - but we can make it a bit more readable by using a
> lookup table rather than a switch.  In addition we can use the MiB/GiB
> symbols to make it a bit clearer.
> 
> While there we add a bit of clarity and rationale to the comment about
> what happens if the LPCR[RMLS] doesn't contain a valid value.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>


> ---
>  target/ppc/mmu-hash64.c | 71 ++++++++++++++++++++---------------------
>  1 file changed, 35 insertions(+), 36 deletions(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 127b7250ae..bb9ebeaf48 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -18,6 +18,7 @@
>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "cpu.h"
>  #include "exec/exec-all.h"
>  #include "exec/helper-proto.h"
> @@ -755,6 +756,39 @@ static void ppc_hash64_set_c(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
>      stb_phys(CPU(cpu)->as, base + offset, (pte1 & 0xff) | 0x80);
>  }
>  
> +static target_ulong rmls_limit(PowerPCCPU *cpu)
> +{
> +    CPUPPCState *env = &cpu->env;
> +    /*
> +     * This is the full 4 bits encoding of POWER8. Previous
> +     * CPUs only support a subset of these but the filtering
> +     * is done when writing LPCR
> +     */
> +    const target_ulong rma_sizes[] = {
> +        [0] = 0,
> +        [1] = 16 * GiB,
> +        [2] = 1 * GiB,
> +        [3] = 64 * MiB,
> +        [4] = 256 * MiB,
> +        [5] = 0,
> +        [6] = 0,
> +        [7] = 128 * MiB,
> +        [8] = 32 * MiB,
> +    };
> +    target_ulong rmls = (env->spr[SPR_LPCR] & LPCR_RMLS) >> LPCR_RMLS_SHIFT;
> +
> +    if (rmls < ARRAY_SIZE(rma_sizes)) {
> +        return rma_sizes[rmls];
> +    } else {
> +        /*
> +         * Bad value, so the OS has shot itself in the foot.  Return a
> +         * 0-sized RMA which we expect to trigger an immediate DSI or
> +         * ISI
> +         */
> +        return 0;
> +    }
> +}
> +
>  int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
>                                  int rwx, int mmu_idx)
>  {
> @@ -1004,41 +1038,6 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
>      cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
>  }
>  
> -static void ppc_hash64_update_rmls(PowerPCCPU *cpu)
> -{
> -    CPUPPCState *env = &cpu->env;
> -    uint64_t lpcr = env->spr[SPR_LPCR];
> -
> -    /*
> -     * This is the full 4 bits encoding of POWER8. Previous
> -     * CPUs only support a subset of these but the filtering
> -     * is done when writing LPCR
> -     */
> -    switch ((lpcr & LPCR_RMLS) >> LPCR_RMLS_SHIFT) {
> -    case 0x8: /* 32MB */
> -        env->rmls = 0x2000000ull;
> -        break;
> -    case 0x3: /* 64MB */
> -        env->rmls = 0x4000000ull;
> -        break;
> -    case 0x7: /* 128MB */
> -        env->rmls = 0x8000000ull;
> -        break;
> -    case 0x4: /* 256MB */
> -        env->rmls = 0x10000000ull;
> -        break;
> -    case 0x2: /* 1GB */
> -        env->rmls = 0x40000000ull;
> -        break;
> -    case 0x1: /* 16GB */
> -        env->rmls = 0x400000000ull;
> -        break;
> -    default:
> -        /* What to do here ??? */
> -        env->rmls = 0;
> -    }
> -}
> -
>  static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
> @@ -1097,7 +1096,7 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>      CPUPPCState *env = &cpu->env;
>  
>      env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
> -    ppc_hash64_update_rmls(cpu);
> +    env->rmls = rmls_limit(cpu);
>      ppc_hash64_update_vrma(cpu);
>  }
>  
> 

[PATCH v2 09/10] target/ppc: Correct RMLS table

[David Gibson]

The table of RMA limits based on the LPCR[RMLS] field is slightly wrong.
We're missing the RMLS == 0 => 256 GiB RMA option, which is available on
POWER8, so add that.

The comment that goes with the table is much more wrong.  We *don't* filter
invalid RMLS values when writing the LPCR, and there's not really a
sensible way to do so.  Furthermore, while in theory the set of RMLS values
is implementation dependent, it seems in practice the same set has been
available since around POWER4+ up until POWER8, the last model which
supports RMLS at all.  So, correct that as well.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/mmu-hash64.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index bb9ebeaf48..e6f24be93e 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -760,12 +760,12 @@ static target_ulong rmls_limit(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
     /*
-     * This is the full 4 bits encoding of POWER8. Previous
-     * CPUs only support a subset of these but the filtering
-     * is done when writing LPCR
+     * In theory the meanings of RMLS values are implementation
+     * dependent.  In practice, this seems to have been the set from
+     * POWER4+..POWER8, and RMLS is no longer supported in POWER9.
      */
     const target_ulong rma_sizes[] = {
-        [0] = 0,
+        [0] = 256 * GiB,
         [1] = 16 * GiB,
         [2] = 1 * GiB,
         [3] = 64 * MiB,
-- 
2.24.1

Re: [PATCH v2 09/10] target/ppc: Correct RMLS table

[Cédric Le Goater]

On 1/7/20 5:48 AM, David Gibson wrote:
> The table of RMA limits based on the LPCR[RMLS] field is slightly wrong.
> We're missing the RMLS == 0 => 256 GiB RMA option, which is available on
> POWER8, so add that.

Where is this defined ? 

C. 

> 
> The comment that goes with the table is much more wrong.  We *don't* filter
> invalid RMLS values when writing the LPCR, and there's not really a
> sensible way to do so.  Furthermore, while in theory the set of RMLS values
> is implementation dependent, it seems in practice the same set has been
> available since around POWER4+ up until POWER8, the last model which
> supports RMLS at all.  So, correct that as well.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/mmu-hash64.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index bb9ebeaf48..e6f24be93e 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -760,12 +760,12 @@ static target_ulong rmls_limit(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
>      /*
> -     * This is the full 4 bits encoding of POWER8. Previous
> -     * CPUs only support a subset of these but the filtering
> -     * is done when writing LPCR
> +     * In theory the meanings of RMLS values are implementation
> +     * dependent.  In practice, this seems to have been the set from
> +     * POWER4+..POWER8, and RMLS is no longer supported in POWER9.
>       */
>      const target_ulong rma_sizes[] = {
> -        [0] = 0,
> +        [0] = 256 * GiB,
>          [1] = 16 * GiB,
>          [2] = 1 * GiB,
>          [3] = 64 * MiB,
> 

Re: [PATCH v2 09/10] target/ppc: Correct RMLS table

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2123 bytes --]

On Tue, Jan 07, 2020 at 03:21:42PM +0100, Cédric Le Goater wrote:
> On 1/7/20 5:48 AM, David Gibson wrote:
> > The table of RMA limits based on the LPCR[RMLS] field is slightly wrong.
> > We're missing the RMLS == 0 => 256 GiB RMA option, which is available on
> > POWER8, so add that.
> 
> Where is this defined ?

It's in the Book4, so not easily available, unfortunately :(.

> > The comment that goes with the table is much more wrong.  We *don't* filter
> > invalid RMLS values when writing the LPCR, and there's not really a
> > sensible way to do so.  Furthermore, while in theory the set of RMLS values
> > is implementation dependent, it seems in practice the same set has been
> > available since around POWER4+ up until POWER8, the last model which
> > supports RMLS at all.  So, correct that as well.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  target/ppc/mmu-hash64.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > index bb9ebeaf48..e6f24be93e 100644
> > --- a/target/ppc/mmu-hash64.c
> > +++ b/target/ppc/mmu-hash64.c
> > @@ -760,12 +760,12 @@ static target_ulong rmls_limit(PowerPCCPU *cpu)
> >  {
> >      CPUPPCState *env = &cpu->env;
> >      /*
> > -     * This is the full 4 bits encoding of POWER8. Previous
> > -     * CPUs only support a subset of these but the filtering
> > -     * is done when writing LPCR
> > +     * In theory the meanings of RMLS values are implementation
> > +     * dependent.  In practice, this seems to have been the set from
> > +     * POWER4+..POWER8, and RMLS is no longer supported in POWER9.
> >       */
> >      const target_ulong rma_sizes[] = {
> > -        [0] = 0,
> > +        [0] = 256 * GiB,
> >          [1] = 16 * GiB,
> >          [2] = 1 * GiB,
> >          [3] = 64 * MiB,
> > 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 09/10] target/ppc: Correct RMLS table

[Cédric Le Goater]

On 1/8/20 2:06 AM, David Gibson wrote:
> On Tue, Jan 07, 2020 at 03:21:42PM +0100, Cédric Le Goater wrote:
>> On 1/7/20 5:48 AM, David Gibson wrote:
>>> The table of RMA limits based on the LPCR[RMLS] field is slightly wrong.
>>> We're missing the RMLS == 0 => 256 GiB RMA option, which is available on
>>> POWER8, so add that.
>>
>> Where is this defined ?
> 
> It's in the Book4, so not easily available, unfortunately :(.

Ah yes. I always forget to check that one.

C.
 

>>> The comment that goes with the table is much more wrong.  We *don't* filter
>>> invalid RMLS values when writing the LPCR, and there's not really a
>>> sensible way to do so.  Furthermore, while in theory the set of RMLS values
>>> is implementation dependent, it seems in practice the same set has been
>>> available since around POWER4+ up until POWER8, the last model which
>>> supports RMLS at all.  So, correct that as well.
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>>  target/ppc/mmu-hash64.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
>>> index bb9ebeaf48..e6f24be93e 100644
>>> --- a/target/ppc/mmu-hash64.c
>>> +++ b/target/ppc/mmu-hash64.c
>>> @@ -760,12 +760,12 @@ static target_ulong rmls_limit(PowerPCCPU *cpu)
>>>  {
>>>      CPUPPCState *env = &cpu->env;
>>>      /*
>>> -     * This is the full 4 bits encoding of POWER8. Previous
>>> -     * CPUs only support a subset of these but the filtering
>>> -     * is done when writing LPCR
>>> +     * In theory the meanings of RMLS values are implementation
>>> +     * dependent.  In practice, this seems to have been the set from
>>> +     * POWER4+..POWER8, and RMLS is no longer supported in POWER9.
>>>       */
>>>      const target_ulong rma_sizes[] = {
>>> -        [0] = 0,
>>> +        [0] = 256 * GiB,
>>>          [1] = 16 * GiB,
>>>          [2] = 1 * GiB,
>>>          [3] = 64 * MiB,
>>>
>>
> 

Re: [PATCH v2 09/10] target/ppc: Correct RMLS table

[Alexey Kardashevskiy]

On 08/01/2020 12:06, David Gibson wrote:
> On Tue, Jan 07, 2020 at 03:21:42PM +0100, Cédric Le Goater wrote:
>> On 1/7/20 5:48 AM, David Gibson wrote:
>>> The table of RMA limits based on the LPCR[RMLS] field is slightly wrong.
>>> We're missing the RMLS == 0 => 256 GiB RMA option, which is available on
>>> POWER8, so add that.
>>
>> Where is this defined ?
> 
> It's in the Book4, so not easily available, unfortunately :(.


It is in "User’s Manual Single-Chip Module POWER8 Processor" which is
public.


> 
>>> The comment that goes with the table is much more wrong.  We *don't* filter
>>> invalid RMLS values when writing the LPCR, and there's not really a
>>> sensible way to do so.  Furthermore, while in theory the set of RMLS values
>>> is implementation dependent, it seems in practice the same set has been
>>> available since around POWER4+ up until POWER8, the last model which
>>> supports RMLS at all.  So, correct that as well.
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>>  target/ppc/mmu-hash64.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
>>> index bb9ebeaf48..e6f24be93e 100644
>>> --- a/target/ppc/mmu-hash64.c
>>> +++ b/target/ppc/mmu-hash64.c
>>> @@ -760,12 +760,12 @@ static target_ulong rmls_limit(PowerPCCPU *cpu)
>>>  {
>>>      CPUPPCState *env = &cpu->env;
>>>      /*
>>> -     * This is the full 4 bits encoding of POWER8. Previous
>>> -     * CPUs only support a subset of these but the filtering
>>> -     * is done when writing LPCR
>>> +     * In theory the meanings of RMLS values are implementation
>>> +     * dependent.  In practice, this seems to have been the set from
>>> +     * POWER4+..POWER8, and RMLS is no longer supported in POWER9.
>>>       */
>>>      const target_ulong rma_sizes[] = {
>>> -        [0] = 0,
>>> +        [0] = 256 * GiB,
>>>          [1] = 16 * GiB,
>>>          [2] = 1 * GiB,
>>>          [3] = 64 * MiB,
>>>
>>
> 

-- 
Alexey

Re: [PATCH v2 09/10] target/ppc: Correct RMLS table

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

On Thu, Jan 09, 2020 at 06:46:19PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 08/01/2020 12:06, David Gibson wrote:
> > On Tue, Jan 07, 2020 at 03:21:42PM +0100, Cédric Le Goater wrote:
> >> On 1/7/20 5:48 AM, David Gibson wrote:
> >>> The table of RMA limits based on the LPCR[RMLS] field is slightly wrong.
> >>> We're missing the RMLS == 0 => 256 GiB RMA option, which is available on
> >>> POWER8, so add that.
> >>
> >> Where is this defined ?
> > 
> > It's in the Book4, so not easily available, unfortunately :(.
> 
> 
> It is in "User’s Manual Single-Chip Module POWER8 Processor" which is
> public.

Oh, excellent!  Looks like it can be downloaded from openpower.org

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 09/10] target/ppc: Correct RMLS table

[Cédric Le Goater]

On 1/7/20 5:48 AM, David Gibson wrote:
> The table of RMA limits based on the LPCR[RMLS] field is slightly wrong.
> We're missing the RMLS == 0 => 256 GiB RMA option, which is available on
> POWER8, so add that.
> 
> The comment that goes with the table is much more wrong.  We *don't* filter
> invalid RMLS values when writing the LPCR, and there's not really a
> sensible way to do so.  Furthermore, while in theory the set of RMLS values
> is implementation dependent, it seems in practice the same set has been
> available since around POWER4+ up until POWER8, the last model which
> supports RMLS at all.  So, correct that as well.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>


> ---
>  target/ppc/mmu-hash64.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index bb9ebeaf48..e6f24be93e 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -760,12 +760,12 @@ static target_ulong rmls_limit(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
>      /*
> -     * This is the full 4 bits encoding of POWER8. Previous
> -     * CPUs only support a subset of these but the filtering
> -     * is done when writing LPCR
> +     * In theory the meanings of RMLS values are implementation
> +     * dependent.  In practice, this seems to have been the set from
> +     * POWER4+..POWER8, and RMLS is no longer supported in POWER9.
>       */
>      const target_ulong rma_sizes[] = {
> -        [0] = 0,
> +        [0] = 256 * GiB,
>          [1] = 16 * GiB,
>          [2] = 1 * GiB,
>          [3] = 64 * MiB,
> 

[PATCH v2 10/10] target/ppc: Only calculate RMLS derived RMA limit on demand

[David Gibson]

When the LPCR is written, we update the env->rmls field with the RMA limit
it implies.  Simplify things by just calculating the value directly from
the LPCR value when we need it.

It's possible this is a little slower, but it's unlikely to be significant,
since this is only for real mode accesses in a translation configuration
that's not used very often, and the whole thing is behind the qemu TLB
anyway.  Therefore, keeping the number of state variables down and not
having to worry about making sure it's always in sync seems the better
option.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/cpu.h        | 1 -
 target/ppc/mmu-hash64.c | 8 +++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 2de9e2fa2b..2a79b97bd8 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1075,7 +1075,6 @@ struct CPUPPCState {
     uint64_t insns_flags2;
 #if defined(TARGET_PPC64)
     ppc_slb_t vrma_slb;
-    target_ulong rmls;
 #endif
 
     int error_code;
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index e6f24be93e..170a78bd2e 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -842,8 +842,10 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
 
             goto skip_slb_search;
         } else {
+            target_ulong limit = rmls_limit(cpu);
+
             /* Emulated old-style RMO mode, bounds check against RMLS */
-            if (raddr >= env->rmls) {
+            if (raddr >= limit) {
                 if (rwx == 2) {
                     ppc_hash64_set_isi(cs, SRR1_PROTFAULT);
                 } else {
@@ -1005,8 +1007,9 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
                 return -1;
             }
         } else {
+            target_ulong limit = rmls_limit(cpu);
             /* Emulated old-style RMO mode, bounds check against RMLS */
-            if (raddr >= env->rmls) {
+            if (raddr >= limit) {
                 return -1;
             }
             return raddr | env->spr[SPR_RMOR];
@@ -1096,7 +1099,6 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
     CPUPPCState *env = &cpu->env;
 
     env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
-    env->rmls = rmls_limit(cpu);
     ppc_hash64_update_vrma(cpu);
 }
 
-- 
2.24.1

Re: [PATCH v2 10/10] target/ppc: Only calculate RMLS derived RMA limit on demand

[Cédric Le Goater]

On 1/7/20 5:48 AM, David Gibson wrote:
> When the LPCR is written, we update the env->rmls field with the RMA limit
> it implies.  Simplify things by just calculating the value directly from
> the LPCR value when we need it.
> 
> It's possible this is a little slower, but it's unlikely to be significant,
> since this is only for real mode accesses in a translation configuration
> that's not used very often, and the whole thing is behind the qemu TLB
> anyway.  Therefore, keeping the number of state variables down and not
> having to worry about making sure it's always in sync seems the better
> option.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>


> ---
>  target/ppc/cpu.h        | 1 -
>  target/ppc/mmu-hash64.c | 8 +++++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 2de9e2fa2b..2a79b97bd8 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1075,7 +1075,6 @@ struct CPUPPCState {
>      uint64_t insns_flags2;
>  #if defined(TARGET_PPC64)
>      ppc_slb_t vrma_slb;
> -    target_ulong rmls;
>  #endif
>  
>      int error_code;
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index e6f24be93e..170a78bd2e 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -842,8 +842,10 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
>  
>              goto skip_slb_search;
>          } else {
> +            target_ulong limit = rmls_limit(cpu);
> +
>              /* Emulated old-style RMO mode, bounds check against RMLS */
> -            if (raddr >= env->rmls) {
> +            if (raddr >= limit) {
>                  if (rwx == 2) {
>                      ppc_hash64_set_isi(cs, SRR1_PROTFAULT);
>                  } else {
> @@ -1005,8 +1007,9 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
>                  return -1;
>              }
>          } else {
> +            target_ulong limit = rmls_limit(cpu);
>              /* Emulated old-style RMO mode, bounds check against RMLS */
> -            if (raddr >= env->rmls) {
> +            if (raddr >= limit) {
>                  return -1;
>              }
>              return raddr | env->spr[SPR_RMOR];
> @@ -1096,7 +1099,6 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>      CPUPPCState *env = &cpu->env;
>  
>      env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
> -    env->rmls = rmls_limit(cpu);
>      ppc_hash64_update_vrma(cpu);
>  }
>  
>