[PATCH v3 00/12] target/ppc: Correct some errors with real mode handling

[PATCH v3 00/12] 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 v2:
 * Removed 32-bit hypervisor stubs more completely
 * Minor polish based on review comments
Changes since RFCv1:
 * Add a number of extra patches taking advantage of the initial
   cleanups

David Gibson (12):
  ppc: Remove stub support for 32-bit hypervisor mode
  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
  target/ppc: Streamline construction of VRMA SLB entry
  target/ppc: Don't store VRMA SLBE persistently

 hw/ppc/spapr_cpu_core.c         |   6 +-
 target/ppc/cpu-qom.h            |   1 +
 target/ppc/cpu.h                |  25 +--
 target/ppc/mmu-hash64.c         | 329 ++++++++++++--------------------
 target/ppc/translate_init.inc.c |  60 ++++--
 5 files changed, 175 insertions(+), 246 deletions(-)

-- 
2.24.1

[PATCH v3 01/12] ppc: Remove stub support for 32-bit hypervisor mode

[David Gibson]

a4f30719a8cd, way back in 2007 noted that "PowerPC hypervisor mode is not
fundamentally available only for PowerPC 64" and added a 32-bit version
of the MSR[HV] bit.

But nothing was ever really done with that; there is no meaningful support
for 32-bit hypervisor mode 13 years later.  Let's stop pretending and just
remove the stubs.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/cpu.h                | 21 +++++++--------------
 target/ppc/translate_init.inc.c |  6 +++---
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index b283042515..8077fdb068 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -24,8 +24,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
@@ -300,13 +298,12 @@ typedef struct ppc_v3_pate_t {
 #define MSR_SF   63 /* Sixty-four-bit mode                            hflags */
 #define MSR_TAG  62 /* Tag-active mode (POWERx ?)                            */
 #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on 630                  */
-#define MSR_SHV  60 /* hypervisor state                               hflags */
+#define MSR_HV   60 /* hypervisor state                               hflags */
 #define MSR_TS0  34 /* Transactional state, 2 bits (Book3s)                  */
 #define MSR_TS1  33
 #define MSR_TM   32 /* Transactional Memory Available (Book3s)               */
 #define MSR_CM   31 /* Computation mode for BookE                     hflags */
 #define MSR_ICM  30 /* Interrupt computation mode for BookE                  */
-#define MSR_THV  29 /* hypervisor state for 32 bits PowerPC           hflags */
 #define MSR_GS   28 /* guest state for BookE                                 */
 #define MSR_UCLE 26 /* User-mode cache lock enable for BookE                 */
 #define MSR_VR   25 /* altivec available                            x hflags */
@@ -401,10 +398,13 @@ typedef struct ppc_v3_pate_t {
 
 #define msr_sf   ((env->msr >> MSR_SF)   & 1)
 #define msr_isf  ((env->msr >> MSR_ISF)  & 1)
-#define msr_shv  ((env->msr >> MSR_SHV)  & 1)
+#if defined(TARGET_PPC64)
+#define msr_hv   ((env->msr >> MSR_HV)   & 1)
+#else
+#define msr_hv   (0)
+#endif
 #define msr_cm   ((env->msr >> MSR_CM)   & 1)
 #define msr_icm  ((env->msr >> MSR_ICM)  & 1)
-#define msr_thv  ((env->msr >> MSR_THV)  & 1)
 #define msr_gs   ((env->msr >> MSR_GS)   & 1)
 #define msr_ucle ((env->msr >> MSR_UCLE) & 1)
 #define msr_vr   ((env->msr >> MSR_VR)   & 1)
@@ -449,16 +449,9 @@ typedef struct ppc_v3_pate_t {
 
 /* Hypervisor bit is more specific */
 #if defined(TARGET_PPC64)
-#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
+#define MSR_HVB (1ULL << MSR_HV)
 #else
 #define MSR_HVB (0ULL)
-#define msr_hv  (0)
-#endif
 #endif
 
 /* DSISR */
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 53995f62ea..a0d0eaabf2 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -8804,7 +8804,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
                         PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 |
                         PPC2_TM | PPC2_PM_ISA206;
     pcc->msr_mask = (1ull << MSR_SF) |
-                    (1ull << MSR_SHV) |
+                    (1ull << MSR_HV) |
                     (1ull << MSR_TM) |
                     (1ull << MSR_VR) |
                     (1ull << MSR_VSX) |
@@ -9017,7 +9017,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
                         PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 |
                         PPC2_TM | PPC2_ISA300 | PPC2_PRCNTL;
     pcc->msr_mask = (1ull << MSR_SF) |
-                    (1ull << MSR_SHV) |
+                    (1ull << MSR_HV) |
                     (1ull << MSR_TM) |
                     (1ull << MSR_VR) |
                     (1ull << MSR_VSX) |
@@ -9228,7 +9228,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data)
                         PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 |
                         PPC2_TM | PPC2_ISA300 | PPC2_PRCNTL;
     pcc->msr_mask = (1ull << MSR_SF) |
-                    (1ull << MSR_SHV) |
+                    (1ull << MSR_HV) |
                     (1ull << MSR_TM) |
                     (1ull << MSR_VR) |
                     (1ull << MSR_VSX) |
-- 
2.24.1

Re: [PATCH v3 01/12] ppc: Remove stub support for 32-bit hypervisor mode

[Fabiano Rosas]

David Gibson <david@gibson.dropbear.id.au> writes:

> a4f30719a8cd, way back in 2007 noted that "PowerPC hypervisor mode is not
> fundamentally available only for PowerPC 64" and added a 32-bit version
> of the MSR[HV] bit.
>
> But nothing was ever really done with that; there is no meaningful support
> for 32-bit hypervisor mode 13 years later.  Let's stop pretending and just
> remove the stubs.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>

> ---
>  target/ppc/cpu.h                | 21 +++++++--------------
>  target/ppc/translate_init.inc.c |  6 +++---
>  2 files changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index b283042515..8077fdb068 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -24,8 +24,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
> @@ -300,13 +298,12 @@ typedef struct ppc_v3_pate_t {
>  #define MSR_SF   63 /* Sixty-four-bit mode                            hflags */
>  #define MSR_TAG  62 /* Tag-active mode (POWERx ?)                            */
>  #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on 630                  */
> -#define MSR_SHV  60 /* hypervisor state                               hflags */
> +#define MSR_HV   60 /* hypervisor state                               hflags */
>  #define MSR_TS0  34 /* Transactional state, 2 bits (Book3s)                  */
>  #define MSR_TS1  33
>  #define MSR_TM   32 /* Transactional Memory Available (Book3s)               */
>  #define MSR_CM   31 /* Computation mode for BookE                     hflags */
>  #define MSR_ICM  30 /* Interrupt computation mode for BookE                  */
> -#define MSR_THV  29 /* hypervisor state for 32 bits PowerPC           hflags */
>  #define MSR_GS   28 /* guest state for BookE                                 */
>  #define MSR_UCLE 26 /* User-mode cache lock enable for BookE                 */
>  #define MSR_VR   25 /* altivec available                            x hflags */
> @@ -401,10 +398,13 @@ typedef struct ppc_v3_pate_t {
>  
>  #define msr_sf   ((env->msr >> MSR_SF)   & 1)
>  #define msr_isf  ((env->msr >> MSR_ISF)  & 1)
> -#define msr_shv  ((env->msr >> MSR_SHV)  & 1)
> +#if defined(TARGET_PPC64)
> +#define msr_hv   ((env->msr >> MSR_HV)   & 1)
> +#else
> +#define msr_hv   (0)
> +#endif
>  #define msr_cm   ((env->msr >> MSR_CM)   & 1)
>  #define msr_icm  ((env->msr >> MSR_ICM)  & 1)
> -#define msr_thv  ((env->msr >> MSR_THV)  & 1)
>  #define msr_gs   ((env->msr >> MSR_GS)   & 1)
>  #define msr_ucle ((env->msr >> MSR_UCLE) & 1)
>  #define msr_vr   ((env->msr >> MSR_VR)   & 1)
> @@ -449,16 +449,9 @@ typedef struct ppc_v3_pate_t {
>  
>  /* Hypervisor bit is more specific */
>  #if defined(TARGET_PPC64)
> -#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
> +#define MSR_HVB (1ULL << MSR_HV)
>  #else
>  #define MSR_HVB (0ULL)
> -#define msr_hv  (0)
> -#endif
>  #endif
>  
>  /* DSISR */
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 53995f62ea..a0d0eaabf2 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -8804,7 +8804,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>                          PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 |
>                          PPC2_TM | PPC2_PM_ISA206;
>      pcc->msr_mask = (1ull << MSR_SF) |
> -                    (1ull << MSR_SHV) |
> +                    (1ull << MSR_HV) |
>                      (1ull << MSR_TM) |
>                      (1ull << MSR_VR) |
>                      (1ull << MSR_VSX) |
> @@ -9017,7 +9017,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
>                          PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 |
>                          PPC2_TM | PPC2_ISA300 | PPC2_PRCNTL;
>      pcc->msr_mask = (1ull << MSR_SF) |
> -                    (1ull << MSR_SHV) |
> +                    (1ull << MSR_HV) |
>                      (1ull << MSR_TM) |
>                      (1ull << MSR_VR) |
>                      (1ull << MSR_VSX) |
> @@ -9228,7 +9228,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data)
>                          PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 |
>                          PPC2_TM | PPC2_ISA300 | PPC2_PRCNTL;
>      pcc->msr_mask = (1ull << MSR_SF) |
> -                    (1ull << MSR_SHV) |
> +                    (1ull << MSR_HV) |
>                      (1ull << MSR_TM) |
>                      (1ull << MSR_VR) |
>                      (1ull << MSR_VSX) |

[PATCH v3 02/12] 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>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 target/ppc/mmu-hash64.c         | 28 +---------------------------
 target/ppc/translate_init.inc.c | 17 ++++++-----------
 2 files changed, 7 insertions(+), 38 deletions(-)

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

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

[BALATON Zoltan]

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

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

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

Regards,
BALATON Zoltan

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

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

[David Gibson]

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

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

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

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


-- 
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 v3 03/12] 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>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
 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

[PATCH v3 04/12] 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>
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);
-- 
2.24.1

Re: [PATCH v3 04/12] target/ppc: Introduce ppc_hash64_use_vrma() helper

[Fabiano Rosas]

David Gibson <david@gibson.dropbear.id.au> writes:

> 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

... when translation is off, right? Because I see in the 3.0 ISA that
LPCR[VPM1] is still there.

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

Shouldn't this be !ppc_hash64_use_vrma(env)?

And a comment about the original code: all other places that check
LPCR_VPM0 do it after verifying that translation is off, except here
(ppc_hash64_update_vrma). Could that be an issue?

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

Re: [PATCH v3 04/12] target/ppc: Introduce ppc_hash64_use_vrma() helper

[Paul Mackerras]

On Wed, Feb 19, 2020 at 11:06:20AM -0300, Fabiano Rosas wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > 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
> 
> ... when translation is off, right? Because I see in the 3.0 ISA that
> LPCR[VPM1] is still there.

VRMA stands for virtual real mode area, and the "real mode" part
implies that translation is off.  VRMA is not used when translation is
on because then the CPU is not in real mode.

LPCR[VPM1] is indeed still there, but it is a bit different to VPM0
(or what VPM0 used to do); VPM1 doesn't change how translation is
done, just what happens on a fault.

Paul.

Re: [PATCH v3 04/12] target/ppc: Introduce ppc_hash64_use_vrma() helper

[David Gibson]

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

On Wed, Feb 19, 2020 at 11:06:20AM -0300, Fabiano Rosas wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > 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
> 
> ... when translation is off, right? Because I see in the 3.0 ISA that
> LPCR[VPM1] is still there.

Right.  This whole patch (and the whole series) is about when the
guest is in translation off mode.

> 
> > 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)) {
> 
> Shouldn't this be !ppc_hash64_use_vrma(env)?
> 
> And a comment about the original code: all other places that check
> LPCR_VPM0 do it after verifying that translation is off, except here
> (ppc_hash64_update_vrma). Could that be an issue?
> 
> >          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);
> 

-- 
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 v3 05/12] 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 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 d09125d9af..ea5e11f1d9 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

[PATCH v3 06/12] 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>
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 d7d4f012b8..c5629d8ba9 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -8014,12 +8014,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,
@@ -8534,6 +8538,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);
@@ -8675,6 +8680,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

[PATCH v3 07/12] 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>
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 c5629d8ba9..823c3c7b54 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -8475,6 +8475,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;
@@ -8652,6 +8654,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;
@@ -8668,7 +8676,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)
@@ -8824,6 +8831,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;
@@ -8841,8 +8855,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
@@ -9035,6 +9047,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;
@@ -9054,7 +9074,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

[PATCH v3 08/12] 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>
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);
 }
 
-- 
2.24.1

[PATCH v3 09/12] 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>
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,
-- 
2.24.1

[PATCH v3 10/12] 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>
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 8077fdb068..f9871b1233 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1046,7 +1046,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

[PATCH v3 11/12] target/ppc: Streamline construction of VRMA SLB entry

[David Gibson]

When in VRMA mode (i.e. a guest thinks it has the MMU off, but the
hypervisor is still applying translation) we use a special SLB entry,
rather than looking up an SLBE by address as we do when guest translation
is on.

We build that special entry in ppc_hash64_update_vrma() along with some
logic for handling some non-VRMA cases.  Split the actual build of the
VRMA SLBE into a separate helper and streamline it a bit.

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

diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 170a78bd2e..06cfff9860 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -789,6 +789,39 @@ static target_ulong rmls_limit(PowerPCCPU *cpu)
     }
 }
 
+static int build_vrma_slbe(PowerPCCPU *cpu, ppc_slb_t *slb)
+{
+    CPUPPCState *env = &cpu->env;
+    target_ulong lpcr = env->spr[SPR_LPCR];
+    uint32_t vrmasd = (lpcr & LPCR_VRMASD) >> LPCR_VRMASD_SHIFT;
+    target_ulong vsid = SLB_VSID_VRMA | ((vrmasd << 4) & SLB_VSID_LLP_MASK);
+    int i;
+
+    /*
+     * Make one up. Mostly ignore the ESID which will not be needed
+     * for translation
+     */
+    for (i = 0; i < PPC_PAGE_SIZES_MAX_SZ; i++) {
+        const PPCHash64SegmentPageSizes *sps = &cpu->hash64_opts->sps[i];
+
+        if (!sps->page_shift) {
+            break;
+        }
+
+        if ((vsid & SLB_VSID_LLP_MASK) == sps->slb_enc) {
+            slb->esid = SLB_ESID_V;
+            slb->vsid = vsid;
+            slb->sps = sps;
+            return 0;
+        }
+    }
+
+    error_report("Bad page size encoding in LPCR[VRMASD]; LPCR=0x"
+                 TARGET_FMT_lx"\n", lpcr);
+
+    return -1;
+}
+
 int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
                                 int rwx, int mmu_idx)
 {
@@ -1044,53 +1077,17 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
 static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
-    const PPCHash64SegmentPageSizes *sps = NULL;
-    target_ulong esid, vsid, lpcr;
     ppc_slb_t *slb = &env->vrma_slb;
-    uint32_t vrmasd;
-    int i;
-
-    /* First clear it */
-    slb->esid = slb->vsid = 0;
-    slb->sps = NULL;
 
     /* Is VRMA enabled ? */
     if (ppc_hash64_use_vrma(env)) {
-        return;
-    }
-
-    /*
-     * 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);
-    esid = SLB_ESID_V;
-
-    for (i = 0; i < PPC_PAGE_SIZES_MAX_SZ; i++) {
-        const PPCHash64SegmentPageSizes *sps1 = &cpu->hash64_opts->sps[i];
-
-        if (!sps1->page_shift) {
-            break;
-        }
-
-        if ((vsid & SLB_VSID_LLP_MASK) == sps1->slb_enc) {
-            sps = sps1;
-            break;
-        }
-    }
-
-    if (!sps) {
-        error_report("Bad page size encoding esid 0x"TARGET_FMT_lx
-                     " vsid 0x"TARGET_FMT_lx, esid, vsid);
-        return;
+        if (build_vrma_slbe(cpu, slb) == 0)
+            return;
     }
 
-    slb->vsid = vsid;
-    slb->esid = esid;
-    slb->sps = sps;
+    /* Otherwise, clear it to indicate error */
+    slb->esid = slb->vsid = 0;
+    slb->sps = NULL;
 }
 
 void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
-- 
2.24.1

Re: [PATCH v3 11/12] target/ppc: Streamline construction of VRMA SLB entry

[Fabiano Rosas]

David Gibson <david@gibson.dropbear.id.au> writes:


Hi, just a nitpick, feel free to ignore.

> When in VRMA mode (i.e. a guest thinks it has the MMU off, but the
> hypervisor is still applying translation) we use a special SLB entry,
> rather than looking up an SLBE by address as we do when guest translation
> is on.
>
> We build that special entry in ppc_hash64_update_vrma() along with some
> logic for handling some non-VRMA cases.  Split the actual build of the
> VRMA SLBE into a separate helper and streamline it a bit.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/mmu-hash64.c | 79 ++++++++++++++++++++---------------------
>  1 file changed, 38 insertions(+), 41 deletions(-)
>
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 170a78bd2e..06cfff9860 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -789,6 +789,39 @@ static target_ulong rmls_limit(PowerPCCPU *cpu)
>      }
>  }
>  
> +static int build_vrma_slbe(PowerPCCPU *cpu, ppc_slb_t *slb)
> +{
> +    CPUPPCState *env = &cpu->env;
> +    target_ulong lpcr = env->spr[SPR_LPCR];
> +    uint32_t vrmasd = (lpcr & LPCR_VRMASD) >> LPCR_VRMASD_SHIFT;
> +    target_ulong vsid = SLB_VSID_VRMA | ((vrmasd << 4) & SLB_VSID_LLP_MASK);
> +    int i;
> +
> +    /*
> +     * Make one up. Mostly ignore the ESID which will not be needed
> +     * for translation
> +     */

I find this comment a bit vague. I suggest we either leave it behind or
make it more precise. The ISA says:

"translation of effective addresses to virtual addresses use the SLBE
values in Figure 18 instead of the entry in the SLB corresponding to the
ESID"

Re: [PATCH v3 11/12] target/ppc: Streamline construction of VRMA SLB entry

[David Gibson]

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

On Wed, Feb 19, 2020 at 11:34:22AM -0300, Fabiano Rosas wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> 
> Hi, just a nitpick, feel free to ignore.
> 
> > When in VRMA mode (i.e. a guest thinks it has the MMU off, but the
> > hypervisor is still applying translation) we use a special SLB entry,
> > rather than looking up an SLBE by address as we do when guest translation
> > is on.
> >
> > We build that special entry in ppc_hash64_update_vrma() along with some
> > logic for handling some non-VRMA cases.  Split the actual build of the
> > VRMA SLBE into a separate helper and streamline it a bit.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  target/ppc/mmu-hash64.c | 79 ++++++++++++++++++++---------------------
> >  1 file changed, 38 insertions(+), 41 deletions(-)
> >
> > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > index 170a78bd2e..06cfff9860 100644
> > --- a/target/ppc/mmu-hash64.c
> > +++ b/target/ppc/mmu-hash64.c
> > @@ -789,6 +789,39 @@ static target_ulong rmls_limit(PowerPCCPU *cpu)
> >      }
> >  }
> >  
> > +static int build_vrma_slbe(PowerPCCPU *cpu, ppc_slb_t *slb)
> > +{
> > +    CPUPPCState *env = &cpu->env;
> > +    target_ulong lpcr = env->spr[SPR_LPCR];
> > +    uint32_t vrmasd = (lpcr & LPCR_VRMASD) >> LPCR_VRMASD_SHIFT;
> > +    target_ulong vsid = SLB_VSID_VRMA | ((vrmasd << 4) & SLB_VSID_LLP_MASK);
> > +    int i;
> > +
> > +    /*
> > +     * Make one up. Mostly ignore the ESID which will not be needed
> > +     * for translation
> > +     */
> 
> I find this comment a bit vague. I suggest we either leave it behind or
> make it more precise. The ISA says:
> 
> "translation of effective addresses to virtual addresses use the SLBE
> values in Figure 18 instead of the entry in the SLB corresponding to the
> ESID"

Yeah, it wasn't very helpful in its initial location, and it's even
less helpful here.  I've dropped it.

-- 
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 v3 12/12] target/ppc: Don't store VRMA SLBE persistently

[David Gibson]

Currently, we construct the SLBE used for VRMA translations when the LPCR
is written (which controls some bits in the SLBE), then use it later for
translations.

This is a bit complex and confusing - simplify it by simply constructing
the SLBE directly from the LPCR when we need it.

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

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index f9871b1233..5a55fb02bd 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1044,9 +1044,6 @@ struct CPUPPCState {
     uint32_t flags;
     uint64_t insns_flags;
     uint64_t insns_flags2;
-#if defined(TARGET_PPC64)
-    ppc_slb_t vrma_slb;
-#endif
 
     int error_code;
     uint32_t pending_interrupts;
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 06cfff9860..d93dcf3a08 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -827,6 +827,7 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
+    ppc_slb_t vrma_slbe;
     ppc_slb_t *slb;
     unsigned apshift;
     hwaddr ptex;
@@ -865,8 +866,8 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
             }
         } else if (ppc_hash64_use_vrma(env)) {
             /* Emulated VRMA mode */
-            slb = &env->vrma_slb;
-            if (!slb->sps) {
+            slb = &vrma_slbe;
+            if (build_vrma_slbe(cpu, slb) != 0) {
                 /* Invalid VRMA setup, machine check */
                 cs->exception_index = POWERPC_EXCP_MCHECK;
                 env->error_code = 0;
@@ -1014,6 +1015,7 @@ skip_slb_search:
 hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
 {
     CPUPPCState *env = &cpu->env;
+    ppc_slb_t vrma_slbe;
     ppc_slb_t *slb;
     hwaddr ptex, raddr;
     ppc_hash_pte64_t pte;
@@ -1035,8 +1037,8 @@ hwaddr ppc_hash64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong addr)
             return raddr | env->spr[SPR_HRMOR];
         } else if (ppc_hash64_use_vrma(env)) {
             /* Emulated VRMA mode */
-            slb = &env->vrma_slb;
-            if (!slb->sps) {
+            slb = &vrma_slbe;
+            if (build_vrma_slbe(cpu, slb) != 0) {
                 return -1;
             }
         } else {
@@ -1074,29 +1076,12 @@ 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_vrma(PowerPCCPU *cpu)
-{
-    CPUPPCState *env = &cpu->env;
-    ppc_slb_t *slb = &env->vrma_slb;
-
-    /* Is VRMA enabled ? */
-    if (ppc_hash64_use_vrma(env)) {
-        if (build_vrma_slbe(cpu, slb) == 0)
-            return;
-    }
-
-    /* Otherwise, clear it to indicate error */
-    slb->esid = slb->vsid = 0;
-    slb->sps = NULL;
-}
-
 void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
 
     env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
-    ppc_hash64_update_vrma(cpu);
 }
 
 void helper_store_lpcr(CPUPPCState *env, target_ulong val)
-- 
2.24.1

Re: [PATCH v3 00/12] target/ppc: Correct some errors with real mode handling

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200219005414.15635-1-david@gibson.dropbear.id.au/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH v3 00/12] target/ppc: Correct some errors with real mode handling
Message-id: 20200219005414.15635-1-david@gibson.dropbear.id.au
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200219005414.15635-1-david@gibson.dropbear.id.au -> patchew/20200219005414.15635-1-david@gibson.dropbear.id.au
Switched to a new branch 'test'
275db2f target/ppc: Don't store VRMA SLBE persistently
8f4ef78 target/ppc: Streamline construction of VRMA SLB entry
5329f3b target/ppc: Only calculate RMLS derived RMA limit on demand
656a372 target/ppc: Correct RMLS table
6432e7f target/ppc: Streamline calculation of RMA limit from LPCR[RMLS]
00f78cd target/ppc: Use class fields to simplify LPCR masking
c6f6cea target/ppc: Remove RMOR register from POWER9 & POWER10
c6daae6 spapr, ppc: Remove VPM0/RMLS hacks for POWER9
3374197 target/ppc: Introduce ppc_hash64_use_vrma() helper
7e14e97 target/ppc: Correct handling of real mode accesses with vhyp on hash MMU
7c298cb ppc: Remove stub of PPC970 HID4 implementation
4525879 ppc: Remove stub support for 32-bit hypervisor mode

=== OUTPUT BEGIN ===
1/12 Checking commit 4525879e6fae (ppc: Remove stub support for 32-bit hypervisor mode)
2/12 Checking commit 7c298cb58821 (ppc: Remove stub of PPC970 HID4 implementation)
WARNING: Block comments use a leading /* on a separate line
#98: FILE: target/ppc/translate_init.inc.c:7904:
+    /* PPC970: HID4 covers things later controlled by the LPCR and

WARNING: Block comments use a trailing */ on a separate line
#102: FILE: target/ppc/translate_init.inc.c:7908:
+     * ignore it */

total: 0 errors, 2 warnings, 71 lines checked

Patch 2/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/12 Checking commit 7e14e97fa725 (target/ppc: Correct handling of real mode accesses with vhyp on hash MMU)
4/12 Checking commit 337419739ee8 (target/ppc: Introduce ppc_hash64_use_vrma() helper)
WARNING: Block comments use a leading /* on a separate line
#41: FILE: target/ppc/mmu-hash64.c:675:
+        /* ISAv3.0 (POWER9) always uses VRMA, the VPM0 field and RMOR

WARNING: Block comments use a trailing */ on a separate line
#42: FILE: target/ppc/mmu-hash64.c:676:
+         * register no longer exist */

total: 0 errors, 2 warnings, 83 lines checked

Patch 4/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/12 Checking commit c6daae6e9b06 (spapr, ppc: Remove VPM0/RMLS hacks for POWER9)
6/12 Checking commit c6f6ceaa43fc (target/ppc: Remove RMOR register from POWER9 & POWER10)
7/12 Checking commit 00f78cdfebbd (target/ppc: Use class fields to simplify LPCR masking)
8/12 Checking commit 6432e7fe864f (target/ppc: Streamline calculation of RMA limit from LPCR[RMLS])
9/12 Checking commit 656a372f677c (target/ppc: Correct RMLS table)
10/12 Checking commit 5329f3b07fba (target/ppc: Only calculate RMLS derived RMA limit on demand)
11/12 Checking commit 8f4ef78a4af3 (target/ppc: Streamline construction of VRMA SLB entry)
ERROR: braces {} are necessary for all arms of this statement
#80: FILE: target/ppc/mmu-hash64.c:1084:
+        if (build_vrma_slbe(cpu, slb) == 0)
[...]

total: 1 errors, 0 warnings, 97 lines checked

Patch 11/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

12/12 Checking commit 275db2fe2a84 (target/ppc: Don't store VRMA SLBE persistently)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200219005414.15635-1-david@gibson.dropbear.id.au/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v3 00/12] target/ppc: Correct some errors with real mode handling

[David Gibson]

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

On Wed, Feb 19, 2020 at 11:54:02AM +1100, David Gibson wrote:
> 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.

Duh.  Forgot to run checkpatch, new version coming shortly.

> 
> Changes since v2:
>  * Removed 32-bit hypervisor stubs more completely
>  * Minor polish based on review comments
> Changes since RFCv1:
>  * Add a number of extra patches taking advantage of the initial
>    cleanups
> 
> David Gibson (12):
>   ppc: Remove stub support for 32-bit hypervisor mode
>   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
>   target/ppc: Streamline construction of VRMA SLB entry
>   target/ppc: Don't store VRMA SLBE persistently
> 
>  hw/ppc/spapr_cpu_core.c         |   6 +-
>  target/ppc/cpu-qom.h            |   1 +
>  target/ppc/cpu.h                |  25 +--
>  target/ppc/mmu-hash64.c         | 329 ++++++++++++--------------------
>  target/ppc/translate_init.inc.c |  60 ++++--
>  5 files changed, 175 insertions(+), 246 deletions(-)
> 

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