diff mbox

[09/15] Parse SDR1 on mtspr instead of at translate time

Message ID 1297522467-5975-10-git-send-email-david@gibson.dropbear.id.au
State New
Headers show

Commit Message

David Gibson Feb. 12, 2011, 2:54 p.m. UTC
On ppc machines with hash table MMUs, the special purpose register SDR1
contains both the base address of the encoded size (hashed) page tables.

At present, we interpret the SDR1 value within the address translation
path.  But because the encodings of the size for 32-bit and 64-bit are

Comments

Alexander Graf Feb. 12, 2011, 3:37 p.m. UTC | #1
On 12.02.2011, at 15:54, David Gibson wrote:

> On ppc machines with hash table MMUs, the special purpose register SDR1
> contains both the base address of the encoded size (hashed) page tables.
> 
> At present, we interpret the SDR1 value within the address translation
> path.  But because the encodings of the size for 32-bit and 64-bit are
> different this makes for a confusing branch on the MMU type with a bunch
> of curly shifts and masks in the middle of the translate path.
> 
> This patch cleans things up by moving the interpretation on SDR1 into the
> helper function handling the write to the register.  This leaves a simple
> pre-sanitized base address and mask for the hash table in the CPUState
> structure which is easier to work with in the translation path.
> 
> This makes the translation path more readable.  It addresses the FIXME
> comment currently in the mtsdr1 helper, by validating the SDR1 value during
> interpretation.  Finally it opens the way for emulating a pSeries-style
> partition where the hash table used for translation is not mapped into
> the guests's RAM.
> 
> Signed-off-by: David Gibson <dwg@au1.ibm.com>
> ---
> monitor.c                   |    2 +-
> target-ppc/cpu.h            |   11 +++++-
> target-ppc/helper.c         |   79 ++++++++++++++++++++++++-------------------
> target-ppc/machine.c        |    6 ++-
> target-ppc/translate.c      |    2 +-
> target-ppc/translate_init.c |    7 +---
> 6 files changed, 61 insertions(+), 46 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 7fc311d..3f77ffc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3457,7 +3457,7 @@ static const MonitorDef monitor_defs[] = {
>     { "asr", offsetof(CPUState, asr) },
> #endif
>     /* Segment registers */
> -    { "sdr1", offsetof(CPUState, sdr1) },
> +    { "sdr1", offsetof(CPUState, spr[SPR_SDR1]) },
>     { "sr0", offsetof(CPUState, sr[0]) },
>     { "sr1", offsetof(CPUState, sr[1]) },
>     { "sr2", offsetof(CPUState, sr[2]) },
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index f9ad3b8..4d30352 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -359,6 +359,14 @@ union ppc_tlb_t {
> };
> #endif
> 
> +#define SDR_HTABORG_32         0xFFFF0000UL
> +#define SDR_HTABMASK           0x000001FFUL

Please mark this constant as ppc32

> +
> +#if defined(TARGET_PPC64)
> +#define SDR_HTABORG_64         0xFFFFFFFFFFFC0000ULL
> +#define SDR_HTABSIZE           0x000000000000001FULL

Please mark this constant as ppc64


The rest looks good :)


Alex
David Gibson Feb. 13, 2011, 9:02 a.m. UTC | #2
On Sat, Feb 12, 2011 at 04:37:46PM +0100, Alexander Graf wrote:
> On 12.02.2011, at 15:54, David Gibson wrote:
[snip]
> > +#define SDR_HTABORG_32         0xFFFF0000UL
> > +#define SDR_HTABMASK           0x000001FFUL
> 
> Please mark this constant as ppc32
> 
> > +
> > +#if defined(TARGET_PPC64)
> > +#define SDR_HTABORG_64         0xFFFFFFFFFFFC0000ULL
> > +#define SDR_HTABSIZE           0x000000000000001FULL
> 
> Please mark this constant as ppc64

Um.. I'm not sure what you mean by this.
Alexander Graf Feb. 13, 2011, 12:33 p.m. UTC | #3
On 13.02.2011, at 10:02, David Gibson wrote:

> On Sat, Feb 12, 2011 at 04:37:46PM +0100, Alexander Graf wrote:
>> On 12.02.2011, at 15:54, David Gibson wrote:
> [snip]
>>> +#define SDR_HTABORG_32         0xFFFF0000UL
>>> +#define SDR_HTABMASK           0x000001FFUL
>> 
>> Please mark this constant as ppc32
>> 
>>> +
>>> +#if defined(TARGET_PPC64)
>>> +#define SDR_HTABORG_64         0xFFFFFFFFFFFC0000ULL
>>> +#define SDR_HTABSIZE           0x000000000000001FULL
>> 
>> Please mark this constant as ppc64
> 
> Um.. I'm not sure what you mean by this.

Well, while to you SDR_HTABMASK and SDR_HTABSIZE are "obviously" meant for ppc32/ppc64 respectively, the average code reader won't know the difference. What I'm proposing is:

#define SDR_32_HTABORG
#define SDR_32_HTABMASK

#define SDR_64_HTABORG
#define SDR_64_HTABSIZE

This way it's a lot more obvious that the two constants really belong to two completely different semantics :).


Alex
David Gibson Feb. 13, 2011, 12:52 p.m. UTC | #4
On Sun, Feb 13, 2011 at 01:33:44PM +0100, Alexander Graf wrote:
> 
> On 13.02.2011, at 10:02, David Gibson wrote:
> 
> > On Sat, Feb 12, 2011 at 04:37:46PM +0100, Alexander Graf wrote:
> >> On 12.02.2011, at 15:54, David Gibson wrote:
> > [snip]
> >>> +#define SDR_HTABORG_32         0xFFFF0000UL
> >>> +#define SDR_HTABMASK           0x000001FFUL
> >> 
> >> Please mark this constant as ppc32
> >> 
> >>> +
> >>> +#if defined(TARGET_PPC64)
> >>> +#define SDR_HTABORG_64         0xFFFFFFFFFFFC0000ULL
> >>> +#define SDR_HTABSIZE           0x000000000000001FULL
> >> 
> >> Please mark this constant as ppc64
> > 
> > Um.. I'm not sure what you mean by this.
> 
> Well, while to you SDR_HTABMASK and SDR_HTABSIZE are "obviously"
> meant for ppc32/ppc64 respectively, the average code reader won't
> know the difference. What I'm proposing is:
> 
> #define SDR_32_HTABORG
> #define SDR_32_HTABMASK
> 
> #define SDR_64_HTABORG
> #define SDR_64_HTABSIZE
> 
> This way it's a lot more obvious that the two constants really
> belong to two completely different semantics :).

Ah! I see.  Done, I'll have this in the next cut.
diff mbox

Patch

different this makes for a confusing branch on the MMU type with a bunch
of curly shifts and masks in the middle of the translate path.

This patch cleans things up by moving the interpretation on SDR1 into the
helper function handling the write to the register.  This leaves a simple
pre-sanitized base address and mask for the hash table in the CPUState
structure which is easier to work with in the translation path.

This makes the translation path more readable.  It addresses the FIXME
comment currently in the mtsdr1 helper, by validating the SDR1 value during
interpretation.  Finally it opens the way for emulating a pSeries-style
partition where the hash table used for translation is not mapped into
the guests's RAM.

Signed-off-by: David Gibson <dwg@au1.ibm.com>
---
 monitor.c                   |    2 +-
 target-ppc/cpu.h            |   11 +++++-
 target-ppc/helper.c         |   79 ++++++++++++++++++++++++-------------------
 target-ppc/machine.c        |    6 ++-
 target-ppc/translate.c      |    2 +-
 target-ppc/translate_init.c |    7 +---
 6 files changed, 61 insertions(+), 46 deletions(-)

diff --git a/monitor.c b/monitor.c
index 7fc311d..3f77ffc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3457,7 +3457,7 @@  static const MonitorDef monitor_defs[] = {
     { "asr", offsetof(CPUState, asr) },
 #endif
     /* Segment registers */
-    { "sdr1", offsetof(CPUState, sdr1) },
+    { "sdr1", offsetof(CPUState, spr[SPR_SDR1]) },
     { "sr0", offsetof(CPUState, sr[0]) },
     { "sr1", offsetof(CPUState, sr[1]) },
     { "sr2", offsetof(CPUState, sr[2]) },
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index f9ad3b8..4d30352 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -359,6 +359,14 @@  union ppc_tlb_t {
 };
 #endif
 
+#define SDR_HTABORG_32         0xFFFF0000UL
+#define SDR_HTABMASK           0x000001FFUL
+
+#if defined(TARGET_PPC64)
+#define SDR_HTABORG_64         0xFFFFFFFFFFFC0000ULL
+#define SDR_HTABSIZE           0x000000000000001FULL
+#endif /* defined(TARGET_PPC64 */
+
 typedef struct ppc_slb_t ppc_slb_t;
 struct ppc_slb_t {
     uint64_t esid;
@@ -642,7 +650,8 @@  struct CPUPPCState {
     int slb_nr;
 #endif
     /* segment registers */
-    target_ulong sdr1;
+    target_phys_addr_t htab_base;
+    target_phys_addr_t htab_mask;
     target_ulong sr[32];
     /* BATs */
     int nb_BATs;
diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index 73d93ca..dcb336b 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -784,20 +784,19 @@  int ppc_load_slb_vsid (CPUPPCState *env, target_ulong rb, target_ulong *rt)
 #endif /* defined(TARGET_PPC64) */
 
 /* Perform segment based translation */
-static inline target_phys_addr_t get_pgaddr(target_phys_addr_t sdr1,
-                                            int sdr_sh,
-                                            target_phys_addr_t hash,
-                                            target_phys_addr_t mask)
+static inline target_phys_addr_t get_pgaddr(target_phys_addr_t htab_base,
+                                            target_phys_addr_t htab_mask,
+                                            target_phys_addr_t hash)
 {
-    return (sdr1 & ((target_phys_addr_t)(-1ULL) << sdr_sh)) | (hash & mask);
+    return htab_base | (hash & htab_mask);
 }
 
 static inline int get_segment(CPUState *env, mmu_ctx_t *ctx,
                               target_ulong eaddr, int rw, int type)
 {
-    target_phys_addr_t sdr, hash, mask, sdr_mask, htab_mask;
+    target_phys_addr_t hash;
     target_ulong sr, vsid, vsid_mask, pgidx, page_mask;
-    int ds, vsid_sh, sdr_sh, pr, target_page_bits;
+    int ds, vsid_sh, pr, target_page_bits;
     int ret, ret2;
 
     pr = msr_pr;
@@ -822,8 +821,6 @@  static inline int get_segment(CPUState *env, mmu_ctx_t *ctx,
         ctx->eaddr = eaddr;
         vsid_mask = 0x00003FFFFFFFFF80ULL;
         vsid_sh = 7;
-        sdr_sh = 18;
-        sdr_mask = 0x3FF80;
     } else
 #endif /* defined(TARGET_PPC64) */
     {
@@ -836,8 +833,6 @@  static inline int get_segment(CPUState *env, mmu_ctx_t *ctx,
         vsid = sr & 0x00FFFFFF;
         vsid_mask = 0x01FFFFC0;
         vsid_sh = 6;
-        sdr_sh = 16;
-        sdr_mask = 0xFFC0;
         target_page_bits = TARGET_PAGE_BITS;
         LOG_MMU("Check segment v=" TARGET_FMT_lx " %d " TARGET_FMT_lx " nip="
                 TARGET_FMT_lx " lr=" TARGET_FMT_lx
@@ -853,29 +848,26 @@  static inline int get_segment(CPUState *env, mmu_ctx_t *ctx,
         if (type != ACCESS_CODE || ctx->nx == 0) {
             /* Page address translation */
             /* Primary table address */
-            sdr = env->sdr1;
             pgidx = (eaddr & page_mask) >> target_page_bits;
 #if defined(TARGET_PPC64)
             if (env->mmu_model & POWERPC_MMU_64) {
-                htab_mask = 0x0FFFFFFF >> (28 - (sdr & 0x1F));
                 /* XXX: this is false for 1 TB segments */
                 hash = ((vsid ^ pgidx) << vsid_sh) & vsid_mask;
             } else
 #endif
             {
-                htab_mask = sdr & 0x000001FF;
                 hash = ((vsid ^ pgidx) << vsid_sh) & vsid_mask;
             }
-            mask = (htab_mask << sdr_sh) | sdr_mask;
-            LOG_MMU("sdr " TARGET_FMT_plx " sh %d hash " TARGET_FMT_plx
-                    " mask " TARGET_FMT_plx " " TARGET_FMT_lx "\n",
-                    sdr, sdr_sh, hash, mask, page_mask);
-            ctx->pg_addr[0] = get_pgaddr(sdr, sdr_sh, hash, mask);
+            LOG_MMU("htab_base " TARGET_FMT_plx " htab_mask " TARGET_FMT_plx
+                    " hash " TARGET_FMT_plx "\n",
+                    env->htab_base, env->htab_mask, hash);
+            ctx->pg_addr[0] = get_pgaddr(env->htab_base, env->htab_mask, hash);
             /* Secondary table address */
             hash = (~hash) & vsid_mask;
-            LOG_MMU("sdr " TARGET_FMT_plx " sh %d hash " TARGET_FMT_plx
-                    " mask " TARGET_FMT_plx "\n", sdr, sdr_sh, hash, mask);
-            ctx->pg_addr[1] = get_pgaddr(sdr, sdr_sh, hash, mask);
+            LOG_MMU("htab_base " TARGET_FMT_plx " htab_mask " TARGET_FMT_plx
+                    " hash " TARGET_FMT_plx "\n",
+                    env->htab_base, env->htab_mask, hash);
+            ctx->pg_addr[1] = get_pgaddr(env->htab_base, env->htab_mask, hash);
 #if defined(TARGET_PPC64)
             if (env->mmu_model & POWERPC_MMU_64) {
                 /* Only 5 bits of the page index are used in the AVPN */
@@ -897,19 +889,21 @@  static inline int get_segment(CPUState *env, mmu_ctx_t *ctx,
                 /* Software TLB search */
                 ret = ppc6xx_tlb_check(env, ctx, eaddr, rw, type);
             } else {
-                LOG_MMU("0 sdr1=" TARGET_FMT_plx " vsid=" TARGET_FMT_lx " "
-                        "api=" TARGET_FMT_lx " hash=" TARGET_FMT_plx
-                        " pg_addr=" TARGET_FMT_plx "\n",
-                        sdr, vsid, pgidx, hash, ctx->pg_addr[0]);
+                LOG_MMU("0 htab=" TARGET_FMT_plx "/" TARGET_FMT_plx
+                        " vsid=" TARGET_FMT_lx " api=" TARGET_FMT_lx
+                        " hash=" TARGET_FMT_plx " pg_addr=" TARGET_FMT_plx "\n",
+                        env->htab_base, env->htab_mask, vsid, pgidx, hash,
+                        ctx->pg_addr[0]);
                 /* Primary table lookup */
                 ret = find_pte(env, ctx, 0, rw, type, target_page_bits);
                 if (ret < 0) {
                     /* Secondary table lookup */
                     if (eaddr != 0xEFFFFFFF)
-                        LOG_MMU("1 sdr1=" TARGET_FMT_plx " vsid=" TARGET_FMT_lx " "
-                                "api=" TARGET_FMT_lx " hash=" TARGET_FMT_plx
-                                " pg_addr=" TARGET_FMT_plx "\n", sdr, vsid,
-                                pgidx, hash, ctx->pg_addr[1]);
+                        LOG_MMU("1 htab=" TARGET_FMT_plx "/" TARGET_FMT_plx
+                                " vsid=" TARGET_FMT_lx " api=" TARGET_FMT_lx
+                                " hash=" TARGET_FMT_plx " pg_addr=" TARGET_FMT_plx "\n",
+                                env->htab_base, env->htab_mask, vsid, pgidx, hash,
+                                ctx->pg_addr[1]);
                     ret2 = find_pte(env, ctx, 1, rw, type,
                                     target_page_bits);
                     if (ret2 != -1)
@@ -1915,11 +1909,26 @@  void ppc_store_asr (CPUPPCState *env, target_ulong value)
 void ppc_store_sdr1 (CPUPPCState *env, target_ulong value)
 {
     LOG_MMU("%s: " TARGET_FMT_lx "\n", __func__, value);
-    if (env->sdr1 != value) {
-        /* XXX: for PowerPC 64, should check that the HTABSIZE value
-         *      is <= 28
-         */
-        env->sdr1 = value;
+    if (env->spr[SPR_SDR1] != value) {
+        env->spr[SPR_SDR1] = value;
+#if defined(TARGET_PPC64)
+        if (env->mmu_model & POWERPC_MMU_64) {
+            target_ulong htabsize = value & SDR_HTABSIZE;
+
+            if (htabsize > 28) {
+                fprintf(stderr, "Invalid HTABSIZE 0x" TARGET_FMT_lx
+                        " stored in SDR1\n", htabsize);
+                htabsize = 28;
+            }
+            env->htab_mask = (1ULL << (htabsize + 18)) - 1;
+            env->htab_base = value & SDR_HTABORG_64;
+        } else
+#endif /* defined(TARGET_PPC64) */
+        {
+            /* FIXME: Should check for valid HTABMASK values */
+            env->htab_mask = ((value & SDR_HTABMASK) << 16) | 0xFFFF;
+            env->htab_base = value & SDR_HTABORG_32;
+        }
         tlb_flush(env, 1);
     }
 }
diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index 67de951..0c1986e 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -37,7 +37,7 @@  void cpu_save(QEMUFile *f, void *opaque)
     qemu_put_betls(f, &env->asr);
     qemu_put_sbe32s(f, &env->slb_nr);
 #endif
-    qemu_put_betls(f, &env->sdr1);
+    qemu_put_betls(f, &env->spr[SPR_SDR1]);
     for (i = 0; i < 32; i++)
         qemu_put_betls(f, &env->sr[i]);
     for (i = 0; i < 2; i++)
@@ -93,6 +93,7 @@  int cpu_load(QEMUFile *f, void *opaque, int version_id)
 {
     CPUState *env = (CPUState *)opaque;
     unsigned int i, j;
+    target_ulong sdr1;
 
     for (i = 0; i < 32; i++)
         qemu_get_betls(f, &env->gpr[i]);
@@ -124,7 +125,7 @@  int cpu_load(QEMUFile *f, void *opaque, int version_id)
     qemu_get_betls(f, &env->asr);
     qemu_get_sbe32s(f, &env->slb_nr);
 #endif
-    qemu_get_betls(f, &env->sdr1);
+    qemu_get_betls(f, &sdr1);
     for (i = 0; i < 32; i++)
         qemu_get_betls(f, &env->sr[i]);
     for (i = 0; i < 2; i++)
@@ -152,6 +153,7 @@  int cpu_load(QEMUFile *f, void *opaque, int version_id)
 #endif
     for (i = 0; i < 1024; i++)
         qemu_get_betls(f, &env->spr[i]);
+    ppc_store_sdr1(env, sdr1);
     qemu_get_be32s(f, &env->vscr);
     qemu_get_be64s(f, &env->spe_acc);
     qemu_get_be32s(f, &env->spe_fscr);
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 5c28ac3..561b756 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -8930,7 +8930,7 @@  void cpu_dump_state (CPUState *env, FILE *f, fprintf_function cpu_fprintf,
 #if !defined(CONFIG_USER_ONLY)
     cpu_fprintf(f, "SRR0 " TARGET_FMT_lx " SRR1 " TARGET_FMT_lx " SDR1 "
                 TARGET_FMT_lx "\n", env->spr[SPR_SRR0], env->spr[SPR_SRR1],
-                env->sdr1);
+                env->spr[SPR_SDR1]);
 #endif
 
 #undef RGPL
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index c842330..c84581e 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -299,11 +299,6 @@  static void spr_write_dbatl_h (void *opaque, int sprn, int gprn)
 }
 
 /* SDR1 */
-static void spr_read_sdr1 (void *opaque, int gprn, int sprn)
-{
-    tcg_gen_ld_tl(cpu_gpr[gprn], cpu_env, offsetof(CPUState, sdr1));
-}
-
 static void spr_write_sdr1 (void *opaque, int sprn, int gprn)
 {
     gen_helper_store_sdr1(cpu_gpr[gprn]);
@@ -627,7 +622,7 @@  static void gen_spr_ne_601 (CPUPPCState *env)
     /* Memory management */
     spr_register(env, SPR_SDR1, "SDR1",
                  SPR_NOACCESS, SPR_NOACCESS,
-                 &spr_read_sdr1, &spr_write_sdr1,
+                 &spr_read_generic, &spr_write_sdr1,
                  0x00000000);
 }