[Qemu-devel] [PATCH -V8 1/3] target-ppc: Fix htab_mask calculation

[Qemu-devel] [PATCH -V8 1/3] target-ppc: Fix htab_mask calculation

[Aneesh Kumar K.V]

From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>

Correctly update the htab_mask using the return value of
KVM_PPC_ALLOCATE_HTAB ioctl. Also we don't update sdr1
on GET_SREGS for HV. So don't update htab_mask if sdr1
is found to be zero. Fix the pte index calculation to be
same as that found in the kernel

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c          | 8 +++++++-
 target-ppc/cpu.h        | 1 +
 target-ppc/mmu_helper.c | 4 +++-
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 267a47d6cc4d..e99a66170661 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -731,7 +731,13 @@ static void spapr_cpu_reset(void *opaque)
 
     env->external_htab = (uint8_t *)spapr->htab;
     env->htab_base = -1;
-    env->htab_mask = HTAB_SIZE(spapr) - 1;
+    /*
+     * htab_mask is the mask used to normalize hash value to PTEG index.
+     * htab_shift is log2 of hash table size.
+     * We have 8 hpte per group, and each hpte is 16 bytes.
+     * ie have 128 bytes per hpte entry.
+     */
+    env->htab_mask = (1ULL << ((spapr)->htab_shift - 7)) - 1;
     env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab |
         (spapr->htab_shift - 18);
 }
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 26acdba847fa..82db9a442be5 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -955,6 +955,7 @@ struct CPUPPCState {
 #endif
     /* segment registers */
     hwaddr htab_base;
+    /* mask used to normalize hash value to PTEG index */
     hwaddr htab_mask;
     target_ulong sr[32];
     /* externally stored hash table */
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index 04a840b01697..c39cb7b89ee0 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -2025,7 +2025,9 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
                         " stored in SDR1\n", htabsize);
                 htabsize = 28;
             }
-            env->htab_mask = (1ULL << (htabsize + 18)) - 1;
+            if (htabsize) {
+                env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
+            }
             env->htab_base = value & SDR_64_HTABORG;
         } else
 #endif /* defined(TARGET_PPC64) */
-- 
1.8.3.2

[Qemu-devel] [PATCH -V8 2/3] target-ppc: Update external_htab even when HTAB is managed by kernel

[Aneesh Kumar K.V]

From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>

We will use this in later patches to make sure we use the right load
functions when copying hpte entries.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e99a66170661..3c0f29c99820 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -730,6 +730,13 @@ static void spapr_cpu_reset(void *opaque)
     env->spr[SPR_HIOR] = 0;
 
     env->external_htab = (uint8_t *)spapr->htab;
+    if (kvm_enabled() && !env->external_htab) {
+        /*
+         * HV KVM, set external_htab to 1 so our ppc_hash64_load_hpte*
+         * functions do the right thing.
+         */
+        env->external_htab = (void *)1;
+    }
     env->htab_base = -1;
     /*
      * htab_mask is the mask used to normalize hash value to PTEG index.
-- 
1.8.3.2

[Qemu-devel] [PATCH -V8 3/3] target-ppc: Fix page table lookup with kvm enabled

[Aneesh Kumar K.V]

From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>

With kvm enabled, we store the hash page table information in the hypervisor.
Use ioctl to read the htab contents. Without this we get the below error when
trying to read the guest address

 (gdb) x/10 do_fork
 0xc000000000098660 <do_fork>:   Cannot access memory at address 0xc000000000098660
 (gdb)

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
Changes from V7:
* used uint64_t token

 hw/ppc/spapr.c          |  1 +
 hw/ppc/spapr_hcall.c    | 50 +++++++++++++++++++------------
 target-ppc/kvm.c        | 53 +++++++++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h    | 19 ++++++++++++
 target-ppc/mmu-hash64.c | 78 ++++++++++++++++++++++++++++++++++++++++---------
 target-ppc/mmu-hash64.h | 19 ++++++++----
 6 files changed, 181 insertions(+), 39 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3c0f29c99820..05244244301a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -676,6 +676,7 @@ static void spapr_reset_htab(sPAPREnvironment *spapr)
     if (shift > 0) {
         /* Kernel handles htab, we don't need to allocate one */
         spapr->htab_shift = shift;
+        kvmppc_kern_htab = true;
     } else {
         if (!spapr->htab) {
             /* Allocate an htab if we don't yet have one */
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index f755a5392317..01cf6b05fee7 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -50,8 +50,9 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     target_ulong ptel = args[3];
     target_ulong page_shift = 12;
     target_ulong raddr;
-    target_ulong i;
+    target_ulong index;
     hwaddr hpte;
+    uint64_t token;
 
     /* only handle 4k and 16M pages for now */
     if (pteh & HPTE64_V_LARGE) {
@@ -94,30 +95,37 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) {
         return H_PARAMETER;
     }
+
+    index = 0;
+    hpte = pte_index * HASH_PTE_SIZE_64;
     if (likely((flags & H_EXACT) == 0)) {
         pte_index &= ~7ULL;
-        hpte = pte_index * HASH_PTE_SIZE_64;
-        for (i = 0; ; ++i) {
-            if (i == 8) {
+        token = ppc_hash64_start_access(cpu, pte_index);
+        do {
+            if (index == 8) {
+                ppc_hash64_stop_access(token);
                 return H_PTEG_FULL;
             }
-            if ((ppc_hash64_load_hpte0(env, hpte) & HPTE64_V_VALID) == 0) {
+            if ((ppc_hash64_load_hpte0(env, token, index) & HPTE64_V_VALID) == 0) {
                 break;
             }
-            hpte += HASH_PTE_SIZE_64;
-        }
+        } while (index++);
+        ppc_hash64_stop_access(token);
     } else {
-        i = 0;
-        hpte = pte_index * HASH_PTE_SIZE_64;
-        if (ppc_hash64_load_hpte0(env, hpte) & HPTE64_V_VALID) {
+        token = ppc_hash64_start_access(cpu, pte_index);
+        if (ppc_hash64_load_hpte0(env, token, 0) & HPTE64_V_VALID) {
+            ppc_hash64_stop_access(token);
             return H_PTEG_FULL;
         }
+        ppc_hash64_stop_access(token);
     }
+    hpte += index * HASH_PTE_SIZE_64;
+
     ppc_hash64_store_hpte1(env, hpte, ptel);
     /* eieio();  FIXME: need some sort of barrier for smp? */
     ppc_hash64_store_hpte0(env, hpte, pteh | HPTE64_V_HPTE_DIRTY);
 
-    args[0] = pte_index + i;
+    args[0] = pte_index + index;
     return H_SUCCESS;
 }
 
@@ -134,16 +142,17 @@ static RemoveResult remove_hpte(CPUPPCState *env, target_ulong ptex,
                                 target_ulong *vp, target_ulong *rp)
 {
     hwaddr hpte;
+    uint64_t token;
     target_ulong v, r, rb;
 
     if ((ptex * HASH_PTE_SIZE_64) & ~env->htab_mask) {
         return REMOVE_PARM;
     }
 
-    hpte = ptex * HASH_PTE_SIZE_64;
-
-    v = ppc_hash64_load_hpte0(env, hpte);
-    r = ppc_hash64_load_hpte1(env, hpte);
+    token = ppc_hash64_start_access(ppc_env_get_cpu(env), ptex);
+    v = ppc_hash64_load_hpte0(env, token, 0);
+    r = ppc_hash64_load_hpte1(env, token, 0);
+    ppc_hash64_stop_access(token);
 
     if ((v & HPTE64_V_VALID) == 0 ||
         ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
@@ -152,6 +161,7 @@ static RemoveResult remove_hpte(CPUPPCState *env, target_ulong ptex,
     }
     *vp = v;
     *rp = r;
+    hpte = ptex * HASH_PTE_SIZE_64;
     ppc_hash64_store_hpte0(env, hpte, HPTE64_V_HPTE_DIRTY);
     rb = compute_tlbie_rb(v, r, ptex);
     ppc_tlb_invalidate_one(env, rb);
@@ -260,16 +270,17 @@ static target_ulong h_protect(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     target_ulong pte_index = args[1];
     target_ulong avpn = args[2];
     hwaddr hpte;
+    uint64_t token;
     target_ulong v, r, rb;
 
     if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) {
         return H_PARAMETER;
     }
 
-    hpte = pte_index * HASH_PTE_SIZE_64;
-
-    v = ppc_hash64_load_hpte0(env, hpte);
-    r = ppc_hash64_load_hpte1(env, hpte);
+    token = ppc_hash64_start_access(cpu, pte_index);
+    v = ppc_hash64_load_hpte0(env, token, 0);
+    r = ppc_hash64_load_hpte1(env, token, 0);
+    ppc_hash64_stop_access(token);
 
     if ((v & HPTE64_V_VALID) == 0 ||
         ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
@@ -282,6 +293,7 @@ static target_ulong h_protect(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     r |= (flags << 48) & HPTE64_R_KEY_HI;
     r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
     rb = compute_tlbie_rb(v, r, pte_index);
+    hpte = pte_index * HASH_PTE_SIZE_64;
     ppc_hash64_store_hpte0(env, hpte, (v & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY);
     ppc_tlb_invalidate_one(env, rb);
     ppc_hash64_store_hpte1(env, hpte, r);
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index b77ce5e94cbc..8f5c6b39eb38 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1783,6 +1783,11 @@ bool kvmppc_has_cap_epr(void)
     return cap_epr;
 }
 
+bool kvmppc_has_cap_htab_fd(void)
+{
+    return cap_htab_fd;
+}
+
 static int kvm_ppc_register_host_cpu_type(void)
 {
     TypeInfo type_info = {
@@ -1902,3 +1907,51 @@ int kvm_arch_on_sigbus(int code, void *addr)
 void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
+
+struct kvm_get_htab_buf {
+    struct kvm_get_htab_header header;
+    /*
+     * We required one extra byte for read
+     */
+    unsigned long hpte[(HPTES_PER_GROUP * 2) + 1];
+};
+
+uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, unsigned long pte_index)
+{
+    int htab_fd;
+    struct kvm_get_htab_fd ghf;
+    struct kvm_get_htab_buf  *hpte_buf;
+
+    ghf.flags = 0;
+    ghf.start_index = pte_index;
+    htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
+    if (htab_fd < 0) {
+        goto error_out;
+    }
+
+    hpte_buf = g_malloc0(sizeof(*hpte_buf));
+    /*
+     * Read the hpte group
+     */
+    if (read(htab_fd, hpte_buf, sizeof(*hpte_buf)) < 0) {
+        goto out_close;
+    }
+
+    close(htab_fd);
+    return (uint64_t) hpte_buf->hpte;
+
+out_close:
+    g_free(hpte_buf);
+    close(htab_fd);
+error_out:
+    return 0;
+}
+
+void kvmppc_hash64_free_pteg(uint64_t token)
+{
+    struct kvm_get_htab_buf *htab_buf;
+
+    htab_buf = container_of((void *)token, struct kvm_get_htab_buf, hpte);
+    g_free(htab_buf);
+    return;
+}
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 5f78e4be14af..4767dc380162 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -39,10 +39,13 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
 int kvmppc_fixup_cpu(PowerPCCPU *cpu);
 bool kvmppc_has_cap_epr(void);
 int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function);
+bool kvmppc_has_cap_htab_fd(void);
 int kvmppc_get_htab_fd(bool write);
 int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
 int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
                            uint16_t n_valid, uint16_t n_invalid);
+uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, unsigned long pte_index);
+void kvmppc_hash64_free_pteg(uint64_t token);
 
 #else
 
@@ -171,6 +174,11 @@ static inline int kvmppc_define_rtas_kernel_token(uint32_t token,
     return -1;
 }
 
+static inline bool kvmppc_has_cap_htab_fd(void)
+{
+    return false;
+}
+
 static inline int kvmppc_get_htab_fd(bool write)
 {
     return -1;
@@ -188,6 +196,17 @@ static inline int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
     abort();
 }
 
+static inline uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu,
+                                               unsigned long pte_index)
+{
+    abort();
+}
+
+static inline void kvmppc_hash64_free_pteg(uint64_t token)
+{
+    abort();
+}
+
 #endif
 
 #ifndef CONFIG_KVM
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 67fc1b5dec5e..bf2dabe3a6e8 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -41,6 +41,11 @@
 #endif
 
 /*
+ * Used to indicate whether we have allocated htab in the
+ * host kernel
+ */
+bool kvmppc_kern_htab;
+/*
  * SLB handling
  */
 
@@ -302,29 +307,76 @@ static int ppc_hash64_amr_prot(CPUPPCState *env, ppc_hash_pte64_t pte)
     return prot;
 }
 
-static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, hwaddr pteg_off,
+uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, unsigned long pte_index)
+{
+    uint64_t token = 0;
+    hwaddr pte_offset;
+
+    pte_offset = pte_index * HASH_PTE_SIZE_64;
+    if (kvmppc_kern_htab) {
+        /*
+         * HTAB is controlled by KVM. Fetch the PTEG into a new buffer.
+         */
+        token = kvmppc_hash64_read_pteg(cpu, pte_index);
+        if (token) {
+            return token;
+        }
+        /*
+         * pteg read failed, even though we have allocated htab via
+         * kvmppc_reset_htab.
+         */
+        return 0;
+    }
+    /*
+     * HTAB is controlled by QEMU. Just point to the internally
+     * accessible PTEG.
+     */
+    if (cpu->env.external_htab) {
+        token = (uint64_t) cpu->env.external_htab + pte_offset;
+    } else if (cpu->env.htab_base) {
+        token = cpu->env.htab_base + pte_offset;
+    }
+    return token;
+}
+
+void ppc_hash64_stop_access(uint64_t token)
+{
+    if (kvmppc_kern_htab) {
+        return kvmppc_hash64_free_pteg(token);
+    }
+}
+
+static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, hwaddr hash,
                                      bool secondary, target_ulong ptem,
                                      ppc_hash_pte64_t *pte)
 {
-    hwaddr pte_offset = pteg_off;
-    target_ulong pte0, pte1;
     int i;
+    uint64_t token;
+    target_ulong pte0, pte1;
+    unsigned long pte_index;
 
+    pte_index = (hash & env->htab_mask) * HPTES_PER_GROUP;
+    token = ppc_hash64_start_access(ppc_env_get_cpu(env), pte_index);
+    if (!token) {
+        return -1;
+    }
     for (i = 0; i < HPTES_PER_GROUP; i++) {
-        pte0 = ppc_hash64_load_hpte0(env, pte_offset);
-        pte1 = ppc_hash64_load_hpte1(env, pte_offset);
+        pte0 = ppc_hash64_load_hpte0(env, token, i);
+        pte1 = ppc_hash64_load_hpte1(env, token, i);
 
         if ((pte0 & HPTE64_V_VALID)
             && (secondary == !!(pte0 & HPTE64_V_SECONDARY))
             && HPTE64_V_COMPARE(pte0, ptem)) {
             pte->pte0 = pte0;
             pte->pte1 = pte1;
-            return pte_offset;
+            ppc_hash64_stop_access(token);
+            return (pte_index + i) * HASH_PTE_SIZE_64;
         }
-
-        pte_offset += HASH_PTE_SIZE_64;
     }
-
+    ppc_hash64_stop_access(token);
+    /*
+     * We didn't find a valid entry.
+     */
     return -1;
 }
 
@@ -332,7 +384,7 @@ static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
                                      ppc_slb_t *slb, target_ulong eaddr,
                                      ppc_hash_pte64_t *pte)
 {
-    hwaddr pteg_off, pte_offset;
+    hwaddr pte_offset;
     hwaddr hash;
     uint64_t vsid, epnshift, epnmask, epn, ptem;
 
@@ -367,8 +419,7 @@ static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
             " vsid=" TARGET_FMT_lx " ptem=" TARGET_FMT_lx
             " hash=" TARGET_FMT_plx "\n",
             env->htab_base, env->htab_mask, vsid, ptem,  hash);
-    pteg_off = (hash * HASH_PTEG_SIZE_64) & env->htab_mask;
-    pte_offset = ppc_hash64_pteg_search(env, pteg_off, 0, ptem, pte);
+    pte_offset = ppc_hash64_pteg_search(env, hash, 0, ptem, pte);
 
     if (pte_offset == -1) {
         /* Secondary PTEG lookup */
@@ -377,8 +428,7 @@ static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
                 " hash=" TARGET_FMT_plx "\n", env->htab_base,
                 env->htab_mask, vsid, ptem, ~hash);
 
-        pteg_off = (~hash * HASH_PTEG_SIZE_64) & env->htab_mask;
-        pte_offset = ppc_hash64_pteg_search(env, pteg_off, 1, ptem, pte);
+        pte_offset = ppc_hash64_pteg_search(env, ~hash, 1, ptem, pte);
     }
 
     return pte_offset;
diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
index 55f5a230fd20..5543f488fe13 100644
--- a/target-ppc/mmu-hash64.h
+++ b/target-ppc/mmu-hash64.h
@@ -75,23 +75,30 @@ int ppc_hash64_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw,
 #define HPTE64_V_1TB_SEG        0x4000000000000000ULL
 #define HPTE64_V_VRMA_MASK      0x4001ffffff000000ULL
 
+
+extern bool kvmppc_kern_htab;
+uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, unsigned long pte_index);
+void ppc_hash64_stop_access(uint64_t token);
+
 static inline target_ulong ppc_hash64_load_hpte0(CPUPPCState *env,
-                                                 hwaddr pte_offset)
+                                                 uint64_t token, int index)
 {
+    index *= HASH_PTE_SIZE_64;
     if (env->external_htab) {
-        return  ldq_p(env->external_htab + pte_offset);
+        return  ldq_p((const void *)(token + index));
     } else {
-        return ldq_phys(env->htab_base + pte_offset);
+        return ldq_phys(token + index);
     }
 }
 
 static inline target_ulong ppc_hash64_load_hpte1(CPUPPCState *env,
-                                                 hwaddr pte_offset)
+                                                 uint64_t token, int index)
 {
+    index *= HASH_PTE_SIZE_64;
     if (env->external_htab) {
-        return ldq_p(env->external_htab + pte_offset + HASH_PTE_SIZE_64/2);
+        return  ldq_p((const void *)(token + index + HASH_PTE_SIZE_64/2));
     } else {
-        return ldq_phys(env->htab_base + pte_offset + HASH_PTE_SIZE_64/2);
+        return ldq_phys(token + index + HASH_PTE_SIZE_64/2);
     }
 }
 
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH -V8 3/3] target-ppc: Fix page table lookup with kvm enabled

[Alexander Graf]

On 23.12.2013, at 18:23, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:

> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> 
> With kvm enabled, we store the hash page table information in the hypervisor.
> Use ioctl to read the htab contents. Without this we get the below error when
> trying to read the guest address
> 
> (gdb) x/10 do_fork
> 0xc000000000098660 <do_fork>:   Cannot access memory at address 0xc000000000098660
> (gdb)
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> Changes from V7:
> * used uint64_t token
> 
> hw/ppc/spapr.c          |  1 +
> hw/ppc/spapr_hcall.c    | 50 +++++++++++++++++++------------
> target-ppc/kvm.c        | 53 +++++++++++++++++++++++++++++++++
> target-ppc/kvm_ppc.h    | 19 ++++++++++++
> target-ppc/mmu-hash64.c | 78 ++++++++++++++++++++++++++++++++++++++++---------
> target-ppc/mmu-hash64.h | 19 ++++++++----
> 6 files changed, 181 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3c0f29c99820..05244244301a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -676,6 +676,7 @@ static void spapr_reset_htab(sPAPREnvironment *spapr)
>     if (shift > 0) {
>         /* Kernel handles htab, we don't need to allocate one */
>         spapr->htab_shift = shift;
> +        kvmppc_kern_htab = true;
>     } else {
>         if (!spapr->htab) {
>             /* Allocate an htab if we don't yet have one */
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index f755a5392317..01cf6b05fee7 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -50,8 +50,9 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>     target_ulong ptel = args[3];
>     target_ulong page_shift = 12;
>     target_ulong raddr;
> -    target_ulong i;
> +    target_ulong index;
>     hwaddr hpte;
> +    uint64_t token;
> 
>     /* only handle 4k and 16M pages for now */
>     if (pteh & HPTE64_V_LARGE) {
> @@ -94,30 +95,37 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>     if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) {
>         return H_PARAMETER;
>     }
> +
> +    index = 0;
> +    hpte = pte_index * HASH_PTE_SIZE_64;
>     if (likely((flags & H_EXACT) == 0)) {
>         pte_index &= ~7ULL;
> -        hpte = pte_index * HASH_PTE_SIZE_64;
> -        for (i = 0; ; ++i) {
> -            if (i == 8) {
> +        token = ppc_hash64_start_access(cpu, pte_index);
> +        do {
> +            if (index == 8) {
> +                ppc_hash64_stop_access(token);
>                 return H_PTEG_FULL;
>             }
> -            if ((ppc_hash64_load_hpte0(env, hpte) & HPTE64_V_VALID) == 0) {
> +            if ((ppc_hash64_load_hpte0(env, token, index) & HPTE64_V_VALID) == 0) {
>                 break;
>             }
> -            hpte += HASH_PTE_SIZE_64;
> -        }
> +        } while (index++);
> +        ppc_hash64_stop_access(token);
>     } else {
> -        i = 0;
> -        hpte = pte_index * HASH_PTE_SIZE_64;
> -        if (ppc_hash64_load_hpte0(env, hpte) & HPTE64_V_VALID) {
> +        token = ppc_hash64_start_access(cpu, pte_index);
> +        if (ppc_hash64_load_hpte0(env, token, 0) & HPTE64_V_VALID) {
> +            ppc_hash64_stop_access(token);
>             return H_PTEG_FULL;
>         }
> +        ppc_hash64_stop_access(token);
>     }
> +    hpte += index * HASH_PTE_SIZE_64;
> +
>     ppc_hash64_store_hpte1(env, hpte, ptel);

Didn't I mention that stores should be converted as well?

>     /* eieio();  FIXME: need some sort of barrier for smp? */
>     ppc_hash64_store_hpte0(env, hpte, pteh | HPTE64_V_HPTE_DIRTY);
> 
> -    args[0] = pte_index + i;
> +    args[0] = pte_index + index;
>     return H_SUCCESS;
> }
> 
> @@ -134,16 +142,17 @@ static RemoveResult remove_hpte(CPUPPCState *env, target_ulong ptex,
>                                 target_ulong *vp, target_ulong *rp)
> {
>     hwaddr hpte;
> +    uint64_t token;
>     target_ulong v, r, rb;
> 
>     if ((ptex * HASH_PTE_SIZE_64) & ~env->htab_mask) {
>         return REMOVE_PARM;
>     }
> 
> -    hpte = ptex * HASH_PTE_SIZE_64;
> -
> -    v = ppc_hash64_load_hpte0(env, hpte);
> -    r = ppc_hash64_load_hpte1(env, hpte);
> +    token = ppc_hash64_start_access(ppc_env_get_cpu(env), ptex);
> +    v = ppc_hash64_load_hpte0(env, token, 0);
> +    r = ppc_hash64_load_hpte1(env, token, 0);
> +    ppc_hash64_stop_access(token);
> 
>     if ((v & HPTE64_V_VALID) == 0 ||
>         ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> @@ -152,6 +161,7 @@ static RemoveResult remove_hpte(CPUPPCState *env, target_ulong ptex,
>     }
>     *vp = v;
>     *rp = r;
> +    hpte = ptex * HASH_PTE_SIZE_64;
>     ppc_hash64_store_hpte0(env, hpte, HPTE64_V_HPTE_DIRTY);
>     rb = compute_tlbie_rb(v, r, ptex);
>     ppc_tlb_invalidate_one(env, rb);
> @@ -260,16 +270,17 @@ static target_ulong h_protect(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>     target_ulong pte_index = args[1];
>     target_ulong avpn = args[2];
>     hwaddr hpte;
> +    uint64_t token;
>     target_ulong v, r, rb;
> 
>     if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) {
>         return H_PARAMETER;
>     }
> 
> -    hpte = pte_index * HASH_PTE_SIZE_64;
> -
> -    v = ppc_hash64_load_hpte0(env, hpte);
> -    r = ppc_hash64_load_hpte1(env, hpte);
> +    token = ppc_hash64_start_access(cpu, pte_index);
> +    v = ppc_hash64_load_hpte0(env, token, 0);
> +    r = ppc_hash64_load_hpte1(env, token, 0);
> +    ppc_hash64_stop_access(token);
> 
>     if ((v & HPTE64_V_VALID) == 0 ||
>         ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> @@ -282,6 +293,7 @@ static target_ulong h_protect(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>     r |= (flags << 48) & HPTE64_R_KEY_HI;
>     r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
>     rb = compute_tlbie_rb(v, r, pte_index);
> +    hpte = pte_index * HASH_PTE_SIZE_64;
>     ppc_hash64_store_hpte0(env, hpte, (v & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY);
>     ppc_tlb_invalidate_one(env, rb);
>     ppc_hash64_store_hpte1(env, hpte, r);
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index b77ce5e94cbc..8f5c6b39eb38 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1783,6 +1783,11 @@ bool kvmppc_has_cap_epr(void)
>     return cap_epr;
> }
> 
> +bool kvmppc_has_cap_htab_fd(void)
> +{
> +    return cap_htab_fd;
> +}
> +
> static int kvm_ppc_register_host_cpu_type(void)
> {
>     TypeInfo type_info = {
> @@ -1902,3 +1907,51 @@ int kvm_arch_on_sigbus(int code, void *addr)
> void kvm_arch_init_irq_routing(KVMState *s)
> {
> }
> +
> +struct kvm_get_htab_buf {
> +    struct kvm_get_htab_header header;
> +    /*
> +     * We required one extra byte for read

Why past tense?


Alex

Re: [Qemu-devel] [PATCH -V8 3/3] target-ppc: Fix page table lookup with kvm enabled

[Aneesh Kumar K.V]

Alexander Graf <agraf@suse.de> writes:

> On 23.12.2013, at 18:23, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
>
>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> 
>> With kvm enabled, we store the hash page table information in the hypervisor.
>> Use ioctl to read the htab contents. Without this we get the below error when
>> trying to read the guest address
>> 
>> (gdb) x/10 do_fork
>> 0xc000000000098660 <do_fork>:   Cannot access memory at address 0xc000000000098660
>> (gdb)
>> 
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>> Changes from V7:
>> * used uint64_t token
>> 
>> hw/ppc/spapr.c          |  1 +
>> hw/ppc/spapr_hcall.c    | 50 +++++++++++++++++++------------
>> target-ppc/kvm.c        | 53 +++++++++++++++++++++++++++++++++
>> target-ppc/kvm_ppc.h    | 19 ++++++++++++
>> target-ppc/mmu-hash64.c | 78 ++++++++++++++++++++++++++++++++++++++++---------
>> target-ppc/mmu-hash64.h | 19 ++++++++----
>> 6 files changed, 181 insertions(+), 39 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 3c0f29c99820..05244244301a 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -676,6 +676,7 @@ static void spapr_reset_htab(sPAPREnvironment *spapr)
>>     if (shift > 0) {
>>         /* Kernel handles htab, we don't need to allocate one */
>>         spapr->htab_shift = shift;
>> +        kvmppc_kern_htab = true;
>>     } else {
>>         if (!spapr->htab) {
>>             /* Allocate an htab if we don't yet have one */
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index f755a5392317..01cf6b05fee7 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -50,8 +50,9 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>     target_ulong ptel = args[3];
>>     target_ulong page_shift = 12;
>>     target_ulong raddr;
>> -    target_ulong i;
>> +    target_ulong index;
>>     hwaddr hpte;
>> +    uint64_t token;
>> 
>>     /* only handle 4k and 16M pages for now */
>>     if (pteh & HPTE64_V_LARGE) {
>> @@ -94,30 +95,37 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>     if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) {
>>         return H_PARAMETER;
>>     }
>> +
>> +    index = 0;
>> +    hpte = pte_index * HASH_PTE_SIZE_64;
>>     if (likely((flags & H_EXACT) == 0)) {
>>         pte_index &= ~7ULL;
>> -        hpte = pte_index * HASH_PTE_SIZE_64;
>> -        for (i = 0; ; ++i) {
>> -            if (i == 8) {
>> +        token = ppc_hash64_start_access(cpu, pte_index);
>> +        do {
>> +            if (index == 8) {
>> +                ppc_hash64_stop_access(token);
>>                 return H_PTEG_FULL;
>>             }
>> -            if ((ppc_hash64_load_hpte0(env, hpte) & HPTE64_V_VALID) == 0) {
>> +            if ((ppc_hash64_load_hpte0(env, token, index) & HPTE64_V_VALID) == 0) {
>>                 break;
>>             }
>> -            hpte += HASH_PTE_SIZE_64;
>> -        }
>> +        } while (index++);
>> +        ppc_hash64_stop_access(token);
>>     } else {
>> -        i = 0;
>> -        hpte = pte_index * HASH_PTE_SIZE_64;
>> -        if (ppc_hash64_load_hpte0(env, hpte) & HPTE64_V_VALID) {
>> +        token = ppc_hash64_start_access(cpu, pte_index);
>> +        if (ppc_hash64_load_hpte0(env, token, 0) & HPTE64_V_VALID) {
>> +            ppc_hash64_stop_access(token);
>>             return H_PTEG_FULL;
>>         }
>> +        ppc_hash64_stop_access(token);
>>     }
>> +    hpte += index * HASH_PTE_SIZE_64;
>> +
>>     ppc_hash64_store_hpte1(env, hpte, ptel);
>
> Didn't I mention that stores should be converted as well?

For the specific use case of x/10i we didn't needed store. And I though
you agreed that just getting read alone now is useful if we can be
sure store follow later. I was hoping to get to that later once this
gets upstream
>
>>     /* eieio();  FIXME: need some sort of barrier for smp? */
>>     ppc_hash64_store_hpte0(env, hpte, pteh | HPTE64_V_HPTE_DIRTY);
>> 
>> -    args[0] = pte_index + i;
>> +    args[0] = pte_index + index;
>>     return H_SUCCESS;
>> }
>> 
>> @@ -134,16 +142,17 @@ static RemoveResult remove_hpte(CPUPPCState *env, target_ulong ptex,
>>                                 target_ulong *vp, target_ulong *rp)
>> {
>>     hwaddr hpte;
>> +    uint64_t token;
>>     target_ulong v, r, rb;
>> 
>>     if ((ptex * HASH_PTE_SIZE_64) & ~env->htab_mask) {
>>         return REMOVE_PARM;
>>     }
>> 
>> -    hpte = ptex * HASH_PTE_SIZE_64;
>> -
>> -    v = ppc_hash64_load_hpte0(env, hpte);
>> -    r = ppc_hash64_load_hpte1(env, hpte);
>> +    token = ppc_hash64_start_access(ppc_env_get_cpu(env), ptex);
>> +    v = ppc_hash64_load_hpte0(env, token, 0);
>> +    r = ppc_hash64_load_hpte1(env, token, 0);
>> +    ppc_hash64_stop_access(token);
>> 
>>     if ((v & HPTE64_V_VALID) == 0 ||
>>         ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
>> @@ -152,6 +161,7 @@ static RemoveResult remove_hpte(CPUPPCState *env, target_ulong ptex,
>>     }
>>     *vp = v;
>>     *rp = r;
>> +    hpte = ptex * HASH_PTE_SIZE_64;
>>     ppc_hash64_store_hpte0(env, hpte, HPTE64_V_HPTE_DIRTY);
>>     rb = compute_tlbie_rb(v, r, ptex);
>>     ppc_tlb_invalidate_one(env, rb);
>> @@ -260,16 +270,17 @@ static target_ulong h_protect(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>     target_ulong pte_index = args[1];
>>     target_ulong avpn = args[2];
>>     hwaddr hpte;
>> +    uint64_t token;
>>     target_ulong v, r, rb;
>> 
>>     if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) {
>>         return H_PARAMETER;
>>     }
>> 
>> -    hpte = pte_index * HASH_PTE_SIZE_64;
>> -
>> -    v = ppc_hash64_load_hpte0(env, hpte);
>> -    r = ppc_hash64_load_hpte1(env, hpte);
>> +    token = ppc_hash64_start_access(cpu, pte_index);
>> +    v = ppc_hash64_load_hpte0(env, token, 0);
>> +    r = ppc_hash64_load_hpte1(env, token, 0);
>> +    ppc_hash64_stop_access(token);
>> 
>>     if ((v & HPTE64_V_VALID) == 0 ||
>>         ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
>> @@ -282,6 +293,7 @@ static target_ulong h_protect(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>     r |= (flags << 48) & HPTE64_R_KEY_HI;
>>     r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
>>     rb = compute_tlbie_rb(v, r, pte_index);
>> +    hpte = pte_index * HASH_PTE_SIZE_64;
>>     ppc_hash64_store_hpte0(env, hpte, (v & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY);
>>     ppc_tlb_invalidate_one(env, rb);
>>     ppc_hash64_store_hpte1(env, hpte, r);
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index b77ce5e94cbc..8f5c6b39eb38 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -1783,6 +1783,11 @@ bool kvmppc_has_cap_epr(void)
>>     return cap_epr;
>> }
>> 
>> +bool kvmppc_has_cap_htab_fd(void)
>> +{
>> +    return cap_htab_fd;
>> +}
>> +
>> static int kvm_ppc_register_host_cpu_type(void)
>> {
>>     TypeInfo type_info = {
>> @@ -1902,3 +1907,51 @@ int kvm_arch_on_sigbus(int code, void *addr)
>> void kvm_arch_init_irq_routing(KVMState *s)
>> {
>> }
>> +
>> +struct kvm_get_htab_buf {
>> +    struct kvm_get_htab_header header;
>> +    /*
>> +     * We required one extra byte for read
>
> Why past tense?

Wrong english, nothing else :). Will fix

-aneesh

Re: [Qemu-devel] [PATCH -V8 3/3] target-ppc: Fix page table lookup with kvm enabled

[Alexander Graf]

On 09.01.2014, at 16:46, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 23.12.2013, at 18:23, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
>> 
>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>> 
>>> With kvm enabled, we store the hash page table information in the hypervisor.
>>> Use ioctl to read the htab contents. Without this we get the below error when
>>> trying to read the guest address
>>> 
>>> (gdb) x/10 do_fork
>>> 0xc000000000098660 <do_fork>:   Cannot access memory at address 0xc000000000098660
>>> (gdb)
>>> 
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>> ---
>>> Changes from V7:
>>> * used uint64_t token
>>> 
>>> hw/ppc/spapr.c          |  1 +
>>> hw/ppc/spapr_hcall.c    | 50 +++++++++++++++++++------------
>>> target-ppc/kvm.c        | 53 +++++++++++++++++++++++++++++++++
>>> target-ppc/kvm_ppc.h    | 19 ++++++++++++
>>> target-ppc/mmu-hash64.c | 78 ++++++++++++++++++++++++++++++++++++++++---------
>>> target-ppc/mmu-hash64.h | 19 ++++++++----
>>> 6 files changed, 181 insertions(+), 39 deletions(-)
>>> 
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 3c0f29c99820..05244244301a 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -676,6 +676,7 @@ static void spapr_reset_htab(sPAPREnvironment *spapr)
>>>    if (shift > 0) {
>>>        /* Kernel handles htab, we don't need to allocate one */
>>>        spapr->htab_shift = shift;
>>> +        kvmppc_kern_htab = true;
>>>    } else {
>>>        if (!spapr->htab) {
>>>            /* Allocate an htab if we don't yet have one */
>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>>> index f755a5392317..01cf6b05fee7 100644
>>> --- a/hw/ppc/spapr_hcall.c
>>> +++ b/hw/ppc/spapr_hcall.c
>>> @@ -50,8 +50,9 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>>    target_ulong ptel = args[3];
>>>    target_ulong page_shift = 12;
>>>    target_ulong raddr;
>>> -    target_ulong i;
>>> +    target_ulong index;
>>>    hwaddr hpte;
>>> +    uint64_t token;
>>> 
>>>    /* only handle 4k and 16M pages for now */
>>>    if (pteh & HPTE64_V_LARGE) {
>>> @@ -94,30 +95,37 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>>    if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) {
>>>        return H_PARAMETER;
>>>    }
>>> +
>>> +    index = 0;
>>> +    hpte = pte_index * HASH_PTE_SIZE_64;
>>>    if (likely((flags & H_EXACT) == 0)) {
>>>        pte_index &= ~7ULL;
>>> -        hpte = pte_index * HASH_PTE_SIZE_64;
>>> -        for (i = 0; ; ++i) {
>>> -            if (i == 8) {
>>> +        token = ppc_hash64_start_access(cpu, pte_index);
>>> +        do {
>>> +            if (index == 8) {
>>> +                ppc_hash64_stop_access(token);
>>>                return H_PTEG_FULL;
>>>            }
>>> -            if ((ppc_hash64_load_hpte0(env, hpte) & HPTE64_V_VALID) == 0) {
>>> +            if ((ppc_hash64_load_hpte0(env, token, index) & HPTE64_V_VALID) == 0) {
>>>                break;
>>>            }
>>> -            hpte += HASH_PTE_SIZE_64;
>>> -        }
>>> +        } while (index++);
>>> +        ppc_hash64_stop_access(token);
>>>    } else {
>>> -        i = 0;
>>> -        hpte = pte_index * HASH_PTE_SIZE_64;
>>> -        if (ppc_hash64_load_hpte0(env, hpte) & HPTE64_V_VALID) {
>>> +        token = ppc_hash64_start_access(cpu, pte_index);
>>> +        if (ppc_hash64_load_hpte0(env, token, 0) & HPTE64_V_VALID) {
>>> +            ppc_hash64_stop_access(token);
>>>            return H_PTEG_FULL;
>>>        }
>>> +        ppc_hash64_stop_access(token);
>>>    }
>>> +    hpte += index * HASH_PTE_SIZE_64;
>>> +
>>>    ppc_hash64_store_hpte1(env, hpte, ptel);
>> 
>> Didn't I mention that stores should be converted as well?
> 
> For the specific use case of x/10i we didn't needed store. And I though
> you agreed that just getting read alone now is useful if we can be
> sure store follow later. I was hoping to get to that later once this
> gets upstream

Ok, please mention these intentions in the cover letter :)


Alex

Re: [Qemu-devel] [PATCH -V8 1/3] target-ppc: Fix htab_mask calculation

[Alexander Graf]

On 23.12.2013, at 18:22, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:

> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> 
> Correctly update the htab_mask using the return value of
> KVM_PPC_ALLOCATE_HTAB ioctl. Also we don't update sdr1
> on GET_SREGS for HV. So don't update htab_mask if sdr1
> is found to be zero. Fix the pte index calculation to be
> same as that found in the kernel
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr.c          | 8 +++++++-
> target-ppc/cpu.h        | 1 +
> target-ppc/mmu_helper.c | 4 +++-
> 3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 267a47d6cc4d..e99a66170661 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -731,7 +731,13 @@ static void spapr_cpu_reset(void *opaque)
> 
>     env->external_htab = (uint8_t *)spapr->htab;
>     env->htab_base = -1;
> -    env->htab_mask = HTAB_SIZE(spapr) - 1;
> +    /*
> +     * htab_mask is the mask used to normalize hash value to PTEG index.
> +     * htab_shift is log2 of hash table size.
> +     * We have 8 hpte per group, and each hpte is 16 bytes.
> +     * ie have 128 bytes per hpte entry.
> +     */
> +    env->htab_mask = (1ULL << ((spapr)->htab_shift - 7)) - 1;
>     env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab |
>         (spapr->htab_shift - 18);
> }
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 26acdba847fa..82db9a442be5 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -955,6 +955,7 @@ struct CPUPPCState {
> #endif
>     /* segment registers */
>     hwaddr htab_base;
> +    /* mask used to normalize hash value to PTEG index */
>     hwaddr htab_mask;
>     target_ulong sr[32];
>     /* externally stored hash table */
> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> index 04a840b01697..c39cb7b89ee0 100644
> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -2025,7 +2025,9 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
>                         " stored in SDR1\n", htabsize);
>                 htabsize = 28;
>             }
> -            env->htab_mask = (1ULL << (htabsize + 18)) - 1;
> +            if (htabsize) {

Better check for external_htab in the caller and never store SDR1 if we use an external htab. If you like, add an assert(!env->external_htab) at the beginning of ppc_store_sdr1() to make sure we really never get here.


Alex

Re: [Qemu-devel] [PATCH -V8 1/3] target-ppc: Fix htab_mask calculation

[Aneesh Kumar K.V]

Alexander Graf <agraf@suse.de> writes:

> On 23.12.2013, at 18:22, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
>
>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> 
>> Correctly update the htab_mask using the return value of
>> KVM_PPC_ALLOCATE_HTAB ioctl. Also we don't update sdr1
>> on GET_SREGS for HV. So don't update htab_mask if sdr1
>> is found to be zero. Fix the pte index calculation to be
>> same as that found in the kernel
>> 
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>> hw/ppc/spapr.c          | 8 +++++++-
>> target-ppc/cpu.h        | 1 +
>> target-ppc/mmu_helper.c | 4 +++-
>> 3 files changed, 11 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 267a47d6cc4d..e99a66170661 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -731,7 +731,13 @@ static void spapr_cpu_reset(void *opaque)
>> 
>>     env->external_htab = (uint8_t *)spapr->htab;
>>     env->htab_base = -1;
>> -    env->htab_mask = HTAB_SIZE(spapr) - 1;
>> +    /*
>> +     * htab_mask is the mask used to normalize hash value to PTEG index.
>> +     * htab_shift is log2 of hash table size.
>> +     * We have 8 hpte per group, and each hpte is 16 bytes.
>> +     * ie have 128 bytes per hpte entry.
>> +     */
>> +    env->htab_mask = (1ULL << ((spapr)->htab_shift - 7)) - 1;
>>     env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab |
>>         (spapr->htab_shift - 18);
>> }
>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>> index 26acdba847fa..82db9a442be5 100644
>> --- a/target-ppc/cpu.h
>> +++ b/target-ppc/cpu.h
>> @@ -955,6 +955,7 @@ struct CPUPPCState {
>> #endif
>>     /* segment registers */
>>     hwaddr htab_base;
>> +    /* mask used to normalize hash value to PTEG index */
>>     hwaddr htab_mask;
>>     target_ulong sr[32];
>>     /* externally stored hash table */
>> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
>> index 04a840b01697..c39cb7b89ee0 100644
>> --- a/target-ppc/mmu_helper.c
>> +++ b/target-ppc/mmu_helper.c
>> @@ -2025,7 +2025,9 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
>>                         " stored in SDR1\n", htabsize);
>>                 htabsize = 28;
>>             }
>> -            env->htab_mask = (1ULL << (htabsize + 18)) - 1;
>> +            if (htabsize) {
>
> Better check for external_htab in the caller and never store SDR1 if
> we use an external htab. If you like, add an
> assert(!env->external_htab) at the beginning of ppc_store_sdr1() to
> make sure we really never get here.


will do

-aneesh