[Qemu-devel] [PATCH 0/6] Cleanups to Hash Page Table handling

[Qemu-devel] [PATCH 0/6] Cleanups to Hash Page Table handling

[David Gibson]

This contains some assorted cleanups and small improvements to the
management of the Hash Page Table for 64-bit ppc systems, and the
"pseries" machine type in particular.

These were devised in the context of getting hash page table resizing
working, but can stand on their own.

David Gibson (6):
  target-ppc: Remove unused kvmppc_update_sdr1() stub
  target-ppc: Include missing MMU models for SDR1 in info registers
  pseries: Simplify handling of the hash page table fd
  pseries: Move hash page table allocation to reset time
  target-ppc: Remove hack for ppc_hash64_load_hpte*() with HV KVM
  target-ppc: Add helpers for updating a CPU's SDR1 and external HPT

 hw/ppc/spapr.c          | 242 +++++++++++++++++++++---------------------------
 include/hw/ppc/spapr.h  |   4 +-
 target-ppc/kvm.c        |  12 +++
 target-ppc/kvm_ppc.h    |  11 ++-
 target-ppc/mmu-hash64.c |  36 +++++++
 target-ppc/mmu-hash64.h |   8 +-
 target-ppc/mmu_helper.c |  13 ++-
 target-ppc/translate.c  |   2 +
 8 files changed, 174 insertions(+), 154 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/6] target-ppc: Remove unused kvmppc_update_sdr1() stub

[David Gibson]

This KVM stub implementation isn't used anywhere.

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

diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 62406ce..aaa828c 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -184,11 +184,6 @@ static inline uint64_t kvmppc_rma_size(uint64_t current_size,
     return ram_size;
 }
 
-static inline int kvmppc_update_sdr1(CPUPPCState *env)
-{
-    return 0;
-}
-
 #endif /* !CONFIG_USER_ONLY */
 
 static inline bool kvmppc_has_cap_epr(void)
-- 
2.5.0

[Qemu-devel] [PATCH 2/6] target-ppc: Include missing MMU models for SDR1 in info registers

[David Gibson]

The HMP command "info registers" produces somewhat different information on
different ppc cpu variants.  For those with a hash MMU it's supposed to
include the SDR1, DAR and DSISR registers related to the MMU.  However,
the switch is missing a couple of MMU model variants, meaning we will
miss out this information on certain CPUs which should have it.

This patch corrects the oversight.  (Really these MMU model IDs need a big
cleanup, but we might as well fix the bug in the interim).

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

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 0057bda..287d679 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -11352,7 +11352,9 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
     case POWERPC_MMU_64B:
     case POWERPC_MMU_2_03:
     case POWERPC_MMU_2_06:
+    case POWERPC_MMU_2_06a:
     case POWERPC_MMU_2_07:
+    case POWERPC_MMU_2_07a:
 #endif
         cpu_fprintf(f, " SDR1 " TARGET_FMT_lx "   DAR " TARGET_FMT_lx
                        "  DSISR " TARGET_FMT_lx "\n", env->spr[SPR_SDR1],
-- 
2.5.0

[Qemu-devel] [PATCH 3/6] pseries: Simplify handling of the hash page table fd

[David Gibson]

When migrating the 'pseries' machine type with KVM, we use a special fd
to access the hash page table stored within KVM.  Usually, this fd is
opened at the beginning of migration, and kept open until the migration
is complete.

However, if there is a guest reset during the migration, the fd can become
stale and we need to re-open it.  At the moment we use an 'htab_fd_stale'
flag in sPAPRMachineState to signal this, which is checked in the migration
iterators.

But that's rather ugly.  It's simpler to just close and invalidate the
fd on reset, and lazily re-open it in migration if necessary.  This patch
implements that change.

This requires a small addition to the machine state's instance_init,
so that htab_fd is initialized to -1 (telling the migration code it
needs to open it) instead of 0, which could be a valid fd.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         | 86 ++++++++++++++++++++++++--------------------------
 include/hw/ppc/spapr.h |  1 -
 2 files changed, 41 insertions(+), 46 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5bd8fd3..c315715 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1024,6 +1024,32 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
 #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
 #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
 
+/*
+ * Get the fd to access the kernel htab, re-opening it if necessary
+ */
+static int get_htab_fd(sPAPRMachineState *spapr)
+{
+    if (spapr->htab_fd >= 0) {
+        return spapr->htab_fd;
+    }
+
+    spapr->htab_fd = kvmppc_get_htab_fd(false);
+    if (spapr->htab_fd < 0) {
+        error_report("Unable to open fd for reading hash table from KVM: %s",
+                     strerror(errno));
+    }
+
+    return spapr->htab_fd;
+}
+
+static void close_htab_fd(sPAPRMachineState *spapr)
+{
+    if (spapr->htab_fd >= 0) {
+        close(spapr->htab_fd);
+    }
+    spapr->htab_fd = -1;
+}
+
 static void spapr_alloc_htab(sPAPRMachineState *spapr)
 {
     long shift;
@@ -1085,10 +1111,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr)
             error_setg(&error_abort, "Requested HTAB allocation failed during reset");
         }
 
-        /* Tell readers to update their file descriptor */
-        if (spapr->htab_fd >= 0) {
-            spapr->htab_fd_stale = true;
-        }
+        close_htab_fd(spapr);
     } else {
         memset(spapr->htab, 0, HTAB_SIZE(spapr));
 
@@ -1121,28 +1144,6 @@ static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
     return 0;
 }
 
-/*
- * A guest reset will cause spapr->htab_fd to become stale if being used.
- * Reopen the file descriptor to make sure the whole HTAB is properly read.
- */
-static int spapr_check_htab_fd(sPAPRMachineState *spapr)
-{
-    int rc = 0;
-
-    if (spapr->htab_fd_stale) {
-        close(spapr->htab_fd);
-        spapr->htab_fd = kvmppc_get_htab_fd(false);
-        if (spapr->htab_fd < 0) {
-            error_report("Unable to open fd for reading hash table from KVM: "
-                         "%s", strerror(errno));
-            rc = -1;
-        }
-        spapr->htab_fd_stale = false;
-    }
-
-    return rc;
-}
-
 static void ppc_spapr_reset(void)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
@@ -1313,14 +1314,6 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
         spapr->htab_first_pass = true;
     } else {
         assert(kvm_enabled());
-
-        spapr->htab_fd = kvmppc_get_htab_fd(false);
-        spapr->htab_fd_stale = false;
-        if (spapr->htab_fd < 0) {
-            fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n",
-                    strerror(errno));
-            return -1;
-        }
     }
 
 
@@ -1460,6 +1453,7 @@ static int htab_save_later_pass(QEMUFile *f, sPAPRMachineState *spapr,
 static int htab_save_iterate(QEMUFile *f, void *opaque)
 {
     sPAPRMachineState *spapr = opaque;
+    int fd;
     int rc = 0;
 
     /* Iteration header */
@@ -1468,13 +1462,12 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
     if (!spapr->htab) {
         assert(kvm_enabled());
 
-        rc = spapr_check_htab_fd(spapr);
-        if (rc < 0) {
-            return rc;
+        fd = get_htab_fd(spapr);
+        if (fd < 0) {
+            return fd;
         }
 
-        rc = kvmppc_save_htab(f, spapr->htab_fd,
-                              MAX_KVM_BUF_SIZE, MAX_ITERATION_NS);
+        rc = kvmppc_save_htab(f, fd, MAX_KVM_BUF_SIZE, MAX_ITERATION_NS);
         if (rc < 0) {
             return rc;
         }
@@ -1495,6 +1488,7 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
 static int htab_save_complete(QEMUFile *f, void *opaque)
 {
     sPAPRMachineState *spapr = opaque;
+    int fd;
 
     /* Iteration header */
     qemu_put_be32(f, 0);
@@ -1504,17 +1498,16 @@ static int htab_save_complete(QEMUFile *f, void *opaque)
 
         assert(kvm_enabled());
 
-        rc = spapr_check_htab_fd(spapr);
-        if (rc < 0) {
-            return rc;
+        fd = get_htab_fd(spapr);
+        if (fd < 0) {
+            return fd;
         }
 
-        rc = kvmppc_save_htab(f, spapr->htab_fd, MAX_KVM_BUF_SIZE, -1);
+        rc = kvmppc_save_htab(f, fd, MAX_KVM_BUF_SIZE, -1);
         if (rc < 0) {
             return rc;
         }
-        close(spapr->htab_fd);
-        spapr->htab_fd = -1;
+        close_htab_fd(spapr);
     } else {
         htab_save_later_pass(f, spapr, -1);
     }
@@ -2125,6 +2118,9 @@ static void spapr_set_kvm_type(Object *obj, const char *value, Error **errp)
 
 static void spapr_machine_initfn(Object *obj)
 {
+    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
+
+    spapr->htab_fd = -1;
     object_property_add_str(obj, "kvm-type",
                             spapr_get_kvm_type, spapr_set_kvm_type, NULL);
     object_property_set_description(obj, "kvm-type",
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 1f9e722..098d85d 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -72,7 +72,6 @@ struct sPAPRMachineState {
     int htab_save_index;
     bool htab_first_pass;
     int htab_fd;
-    bool htab_fd_stale;
 
     /* RTAS state */
     QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
-- 
2.5.0

[Qemu-devel] [PATCH 4/6] pseries: Move hash page table allocation to reset time

[David Gibson]

At the moment the size of the hash page table (HPT) is fixed based on the
maximum memory allowed to the guest.  As such, we allocate the table during
machine construction, and just clear it at reset.

However, we're planning to implement a PAPR extension allowing the hash
page table to be resized at runtime.  This will mean that on reset we want
to revert it to the default size.  It also means that when migrating, we
need to make sure the destination allocates an HPT of size matching the
host, since the guest could have changed it before the migration.

This patch replaces the spapr_alloc_htab() and spapr_reset_htab() functions
with a new spapr_reallocate_hpt() function.  This is called at reset and
inbound migration only, not during machine init any more.

In addition, we add a new helper to compute the recommended hash table size
for a given RAM size.  We export this as well as spapr_reallocate_hpt(),
since we'll be needing them elsewhere in future.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         | 151 ++++++++++++++++++++++---------------------------
 include/hw/ppc/spapr.h |   3 +
 2 files changed, 71 insertions(+), 83 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c315715..d228375 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1050,81 +1050,67 @@ static void close_htab_fd(sPAPRMachineState *spapr)
     spapr->htab_fd = -1;
 }
 
-static void spapr_alloc_htab(sPAPRMachineState *spapr)
-{
-    long shift;
-    int index;
-
-    /* allocate hash page table.  For now we always make this 16mb,
-     * later we should probably make it scale to the size of guest
-     * RAM */
-
-    shift = kvmppc_reset_htab(spapr->htab_shift);
-    if (shift < 0) {
-        /*
-         * For HV KVM, host kernel will return -ENOMEM when requested
-         * HTAB size can't be allocated.
-         */
-        error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
-    } else if (shift > 0) {
-        /*
-         * Kernel handles htab, we don't need to allocate one
-         *
-         * Older kernels can fall back to lower HTAB shift values,
-         * but we don't allow booting of such guests.
-         */
-        if (shift != spapr->htab_shift) {
-            error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
+int spapr_hpt_shift_for_ramsize(uint64_t ramsize)
+{
+    int shift;
+
+    /* We aim for a hash table of size 1/128 the size of RAM (rounded
+     * up).  The PAPR recommendation is actually 1/64 of RAM size, but
+     * that's much more than is needed for Linux guests */
+    shift = ctz64(pow2ceil(ramsize)) - 7;
+    shift = MAX(shift, 18); /* Minimum architected size */
+    shift = MIN(shift, 46); /* Maximum architected size */
+    return shift;
+}
+
+void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift, Error **errp)
+{
+    long rc;
+
+    /* Clean up any HPT info from a previous boot */
+    g_free(spapr->htab);
+    spapr->htab = NULL;
+    spapr->htab_shift = 0;
+    close_htab_fd(spapr);
+
+    rc = kvmppc_reset_htab(shift);
+    if (rc < 0) {
+        /* kernel-side HPT needed, but couldn't allocate one */
+        error_setg_errno(errp, errno,
+                         "Failed to allocate KVM HPT of order %d (try smaller maxmem?)",
+                         shift);
+        /* This is almost certainly fatal, but if the caller really
+         * wants to carry on with shift == 0, it's welcome to try */
+    } else if (rc > 0) {
+        /* kernel-side HPT allocated */
+        if (rc != shift) {
+            error_setg(errp,
+                       "Requested order %d HPT, but kernel allocated order %ld (try smaller maxmem?)",
+                       shift, rc);
         }
 
         spapr->htab_shift = shift;
         kvmppc_kern_htab = true;
     } else {
-        /* Allocate htab */
-        spapr->htab = qemu_memalign(HTAB_SIZE(spapr), HTAB_SIZE(spapr));
-
-        /* And clear it */
-        memset(spapr->htab, 0, HTAB_SIZE(spapr));
-
-        for (index = 0; index < HTAB_SIZE(spapr) / HASH_PTE_SIZE_64; index++) {
-            DIRTY_HPTE(HPTE(spapr->htab, index));
-        }
-    }
-}
-
-/*
- * Clear HTAB entries during reset.
- *
- * If host kernel has allocated HTAB, KVM_PPC_ALLOCATE_HTAB ioctl is
- * used to clear HTAB. Otherwise QEMU-allocated HTAB is cleared manually.
- */
-static void spapr_reset_htab(sPAPRMachineState *spapr)
-{
-    long shift;
-    int index;
+        /* kernel-side HPT not needed, allocate in userspace instead */
+        size_t size = 1ULL << shift;
+        int i;
 
-    shift = kvmppc_reset_htab(spapr->htab_shift);
-    if (shift < 0) {
-        error_setg(&error_abort, "Failed to reset HTAB");
-    } else if (shift > 0) {
-        if (shift != spapr->htab_shift) {
-            error_setg(&error_abort, "Requested HTAB allocation failed during reset");
+        spapr->htab = qemu_memalign(size, size);
+        if (!spapr->htab) {
+            error_setg_errno(errp, errno,
+                             "Could not allocate HPT of order %d", shift);
+            return;
         }
 
-        close_htab_fd(spapr);
-    } else {
-        memset(spapr->htab, 0, HTAB_SIZE(spapr));
+        memset(spapr->htab, 0, size);
+        spapr->htab_shift = shift;
+        kvmppc_kern_htab = false;
 
-        for (index = 0; index < HTAB_SIZE(spapr) / HASH_PTE_SIZE_64; index++) {
-            DIRTY_HPTE(HPTE(spapr->htab, index));
+        for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
+            DIRTY_HPTE(HPTE(spapr->htab, i));
         }
     }
-
-    /* Update the RMA size if necessary */
-    if (spapr->vrma_adjust) {
-        spapr->rma_size = kvmppc_rma_size(spapr_node0_size(),
-                                          spapr->htab_shift);
-    }
 }
 
 static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
@@ -1146,15 +1132,24 @@ static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
 
 static void ppc_spapr_reset(void)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    MachineState *machine = MACHINE(qdev_get_machine());
+    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
     PowerPCCPU *first_ppc_cpu;
     uint32_t rtas_limit;
 
     /* Check for unknown sysbus devices */
     foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
 
-    /* Reset the hash table & recalc the RMA */
-    spapr_reset_htab(spapr);
+    /* Allocate and/or reset the hash page table */
+    spapr_reallocate_hpt(spapr,
+                         spapr_hpt_shift_for_ramsize(machine->maxram_size),
+                         &error_fatal);
+
+    /* Update the RMA size if necessary */
+    if (spapr->vrma_adjust) {
+        spapr->rma_size = kvmppc_rma_size(spapr_node0_size(),
+                                          spapr->htab_shift);
+    }
 
     qemu_devices_reset();
 
@@ -1534,10 +1529,12 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
     section_hdr = qemu_get_be32(f);
 
     if (section_hdr) {
-        /* First section, just the hash shift */
-        if (spapr->htab_shift != section_hdr) {
-            error_report("htab_shift mismatch: source %d target %d",
-                         section_hdr, spapr->htab_shift);
+        Error *local_err;
+
+        /* First section gives the htab size */
+        spapr_reallocate_hpt(spapr, section_hdr, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
             return -EINVAL;
         }
         return 0;
@@ -1790,18 +1787,6 @@ static void ppc_spapr_init(MachineState *machine)
     /* Setup a load limit for the ramdisk leaving room for SLOF and FDT */
     load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD;
 
-    /* We aim for a hash table of size 1/128 the size of RAM.  The
-     * normal rule of thumb is 1/64 the size of RAM, but that's much
-     * more than needed for the Linux guests we support. */
-    spapr->htab_shift = 18; /* Minimum architected size */
-    while (spapr->htab_shift <= 46) {
-        if ((1ULL << (spapr->htab_shift + 7)) >= machine->maxram_size) {
-            break;
-        }
-        spapr->htab_shift++;
-    }
-    spapr_alloc_htab(spapr);
-
     /* Set up Interrupt Controller before we create the VCPUs */
     spapr->icp = xics_system_init(machine,
                                   DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 098d85d..4e3dce5 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -373,6 +373,9 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
 int spapr_allocate_irq(int hint, bool lsi);
 int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 
+int spapr_hpt_shift_for_ramsize(uint64_t ramsize);
+void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift, Error **errp);
+
 /* ibm,set-eeh-option */
 #define RTAS_EEH_DISABLE                 0
 #define RTAS_EEH_ENABLE                  1
-- 
2.5.0

[Qemu-devel] [PATCH 5/6] target-ppc: Remove hack for ppc_hash64_load_hpte*() with HV KVM

[David Gibson]

With HV KVM, the guest's hash page table (HPT) is managed by the kernel and
not directly accessible to QEMU.  This means that spapr->htab is NULL
and normally env->external_htab would also be NULL for each cpu.

However, that would cause ppc_hash64_load_hpte*() to do the wrong thing in
the few cases where QEMU does need to load entries from the in-kernel HPT.
Specifically, seeing external_htab is NULL, they would look for an HPT
within the guest's address space instead.

To stop that we have an ugly hack in the pseries machine type code to
set external htab to (void *)1 instead.

This patch removes that hack by having ppc_hash64_load_hpte*() explicitly
check kvmppc_kern_htab instead, which makes more sense.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d228375..77dd1b6 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1196,13 +1196,6 @@ 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.
diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
index ab0f86b..e7d9925 100644
--- a/target-ppc/mmu-hash64.h
+++ b/target-ppc/mmu-hash64.h
@@ -102,7 +102,7 @@ static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu,
     uint64_t addr;
 
     addr = token + (index * HASH_PTE_SIZE_64);
-    if (env->external_htab) {
+    if (kvmppc_kern_htab || env->external_htab) {
         return  ldq_p((const void *)(uintptr_t)addr);
     } else {
         return ldq_phys(CPU(cpu)->as, addr);
@@ -116,7 +116,7 @@ static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU *cpu,
     uint64_t addr;
 
     addr = token + (index * HASH_PTE_SIZE_64) + HASH_PTE_SIZE_64/2;
-    if (env->external_htab) {
+    if (kvmppc_kern_htab || env->external_htab) {
         return  ldq_p((const void *)(uintptr_t)addr);
     } else {
         return ldq_phys(CPU(cpu)->as, addr);
-- 
2.5.0

[Qemu-devel] [PATCH 6/6] target-ppc: Add helpers for updating a CPU's SDR1 and external HPT

[David Gibson]

When a Power cpu with 64-bit hash MMU has it's hash page table (HPT)
pointer updated by a write to the SDR1 register we need to update some
derived variables.  Likewise, when the cpu is configured for an external
HPT (one not in the guest memory space) some derived variables need to be
updated.

Currently the logic for this is (partially) duplicated in ppc_store_sdr1()
and in spapr_cpu_reset().  In future we're going to need it in some other
places, so make some common helpers for this update.

In addition extend the helpers to update SDR1 in KVM - it's not updated
by the normal runtime KVM<->qemu CPU synchronization.  Currently there
aren't situations where it matters, but there are going to be in future.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c          | 12 +-----------
 target-ppc/kvm.c        | 12 ++++++++++++
 target-ppc/kvm_ppc.h    |  6 ++++++
 target-ppc/mmu-hash64.c | 36 ++++++++++++++++++++++++++++++++++++
 target-ppc/mmu-hash64.h |  4 ++++
 target-ppc/mmu_helper.c | 13 ++++++-------
 6 files changed, 65 insertions(+), 18 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 77dd1b6..af3023b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1195,17 +1195,7 @@ static void spapr_cpu_reset(void *opaque)
 
     env->spr[SPR_HIOR] = 0;
 
-    env->external_htab = (uint8_t *)spapr->htab;
-    env->htab_base = -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);
+    ppc_hash64_set_external_hpt(cpu, spapr->htab, spapr->htab_shift);
 }
 
 static void spapr_create_nvram(sPAPRMachineState *spapr)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 70ca296..8430d43 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2530,3 +2530,15 @@ int kvmppc_enable_hwrng(void)
 
     return kvmppc_enable_hcall(kvm_state, H_RANDOM);
 }
+
+int kvmppc_update_sdr1(PowerPCCPU *cpu)
+{
+    CPUState *cs = CPU(cpu);
+
+    if (!kvm_enabled()) {
+        return 0; /* nothing to do */
+    }
+
+    /* This is overkill, but this shouldn't be a common operation */
+    return kvm_arch_put_registers(cs, KVM_PUT_RESET_STATE);
+}
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index aaa828c..434b3d1 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -55,6 +55,7 @@ void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
                              target_ulong pte0, target_ulong pte1);
 bool kvmppc_has_cap_fixup_hcalls(void);
 int kvmppc_enable_hwrng(void);
+int kvmppc_update_sdr1(PowerPCCPU *cpu);
 
 #else
 
@@ -246,6 +247,11 @@ static inline int kvmppc_enable_hwrng(void)
 {
     return -1;
 }
+
+static inline int kvmppc_update_sdr1(PowerPCCPU *cpu)
+{
+    return 0; /* nothing to do */
+}
 #endif
 
 #ifndef CONFIG_KVM
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 9c58fbf..e15d7b0 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -258,6 +258,42 @@ target_ulong helper_load_slb_vsid(CPUPPCState *env, target_ulong rb)
 /*
  * 64-bit hash table MMU handling
  */
+void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
+                         Error **errp)
+{
+    CPUPPCState *env = &cpu->env;
+    target_ulong htabsize = value & SDR_64_HTABSIZE;
+
+    cpu_synchronize_state(CPU(cpu));
+
+    env->spr[SPR_SDR1] = value;
+    if (htabsize > 28) {
+        error_setg(errp,
+                   "Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1",
+                   htabsize);
+        htabsize = 28;
+    }
+    env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
+    env->htab_base = value & SDR_64_HTABORG;
+
+    if (kvmppc_update_sdr1(cpu) < 0) {
+        error_setg(errp,
+                   "Unable to update SDR1 in KVM");
+    }
+}
+
+void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift)
+{
+    CPUPPCState *env = &cpu->env;
+
+    env->external_htab = hpt;
+    ppc_hash64_set_sdr1(cpu, (target_ulong)(uintptr_t)hpt | (shift - 18),
+                        &error_abort);
+
+    /* Not strictly necessary, but makes it clearer that an external
+     * htab is in use when debugging */
+    env->htab_base = -1;
+}
 
 static int ppc_hash64_pte_prot(PowerPCCPU *cpu,
                                ppc_slb_t *slb, ppc_hash_pte64_t pte)
diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
index e7d9925..09a0f12 100644
--- a/target-ppc/mmu-hash64.h
+++ b/target-ppc/mmu-hash64.h
@@ -91,6 +91,10 @@ unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
 #define HPTE64_V_VRMA_MASK      0x4001ffffff000000ULL
 
 
+void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
+                         Error **errp);
+void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift);
+
 extern bool kvmppc_kern_htab;
 uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong pte_index);
 void ppc_hash64_stop_access(uint64_t token);
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index e5ec8d6..fcb2cc5 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -2005,15 +2005,14 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
     env->spr[SPR_SDR1] = value;
 #if defined(TARGET_PPC64)
     if (env->mmu_model & POWERPC_MMU_64) {
-        target_ulong htabsize = value & SDR_64_HTABSIZE;
+        PowerPCCPU *cpu = ppc_env_get_cpu(env);
+        Error *local_err = NULL;
 
-        if (htabsize > 28) {
-            fprintf(stderr, "Invalid HTABSIZE 0x" TARGET_FMT_lx
-                    " stored in SDR1\n", htabsize);
-            htabsize = 28;
+        ppc_hash64_set_sdr1(cpu, value, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            error_free(local_err);
         }
-        env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
-        env->htab_base = value & SDR_64_HTABORG;
     } else
 #endif /* defined(TARGET_PPC64) */
     {
-- 
2.5.0

Re: [Qemu-devel] [PATCH 4/6] pseries: Move hash page table allocation to reset time

[Alexey Kardashevskiy]

On 02/05/2016 01:13 PM, David Gibson wrote:
> At the moment the size of the hash page table (HPT) is fixed based on the
> maximum memory allowed to the guest.  As such, we allocate the table during
> machine construction, and just clear it at reset.
>
> However, we're planning to implement a PAPR extension allowing the hash
> page table to be resized at runtime.  This will mean that on reset we want
> to revert it to the default size.  It also means that when migrating, we
> need to make sure the destination allocates an HPT of size matching the
> host, since the guest could have changed it before the migration.
>
> This patch replaces the spapr_alloc_htab() and spapr_reset_htab() functions
> with a new spapr_reallocate_hpt() function.  This is called at reset and
> inbound migration only, not during machine init any more.


I'd suggest splitting this patch in two (move spapr_hpt_shift_for_ramsize 
to a new patch, or reorganizing it somehow (move 
spapr_hpt_shift_for_ramsize to a different place) - this one is really hard 
to read and make sure that nothing is lost.


> In addition, we add a new helper to compute the recommended hash table size
> for a given RAM size.  We export this as well as spapr_reallocate_hpt(),
> since we'll be needing them elsewhere in future.

Do you already have a patch in your queue to exploit this?

I was told few times to export stuff when I have to...


-- 
Alexey

Re: [Qemu-devel] [PATCH 6/6] target-ppc: Add helpers for updating a CPU's SDR1 and external HPT

[Alexey Kardashevskiy]

On 02/05/2016 01:13 PM, David Gibson wrote:
> When a Power cpu with 64-bit hash MMU has it's hash page table (HPT)
> pointer updated by a write to the SDR1 register we need to update some
> derived variables.  Likewise, when the cpu is configured for an external
> HPT (one not in the guest memory space) some derived variables need to be
> updated.
>
> Currently the logic for this is (partially) duplicated in ppc_store_sdr1()
> and in spapr_cpu_reset().  In future we're going to need it in some other
> places, so make some common helpers for this update.
>
> In addition extend the helpers to update SDR1 in KVM - it's not updated
> by the normal runtime KVM<->qemu CPU synchronization.  Currently there
> aren't situations where it matters, but there are going to be in future.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>   hw/ppc/spapr.c          | 12 +-----------
>   target-ppc/kvm.c        | 12 ++++++++++++
>   target-ppc/kvm_ppc.h    |  6 ++++++
>   target-ppc/mmu-hash64.c | 36 ++++++++++++++++++++++++++++++++++++
>   target-ppc/mmu-hash64.h |  4 ++++
>   target-ppc/mmu_helper.c | 13 ++++++-------
>   6 files changed, 65 insertions(+), 18 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 77dd1b6..af3023b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1195,17 +1195,7 @@ static void spapr_cpu_reset(void *opaque)
>
>       env->spr[SPR_HIOR] = 0;
>
> -    env->external_htab = (uint8_t *)spapr->htab;
> -    env->htab_base = -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);
> +    ppc_hash64_set_external_hpt(cpu, spapr->htab, spapr->htab_shift);
>   }
>
>   static void spapr_create_nvram(sPAPRMachineState *spapr)
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 70ca296..8430d43 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2530,3 +2530,15 @@ int kvmppc_enable_hwrng(void)
>
>       return kvmppc_enable_hcall(kvm_state, H_RANDOM);
>   }
> +
> +int kvmppc_update_sdr1(PowerPCCPU *cpu)
> +{
> +    CPUState *cs = CPU(cpu);
> +
> +    if (!kvm_enabled()) {
> +        return 0; /* nothing to do */
> +    }
> +
> +    /* This is overkill, but this shouldn't be a common operation */
> +    return kvm_arch_put_registers(cs, KVM_PUT_RESET_STATE);


I had to look at kvm_cpu_exec() (which also calls kvm_arch_put_registers) 
to realize that you need here KVM_PUT_RESET_STATE and not kvm_cpu_exec's 
KVM_PUT_RUNTIME_STATE, that should go to the comment imho.



> +}
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index aaa828c..434b3d1 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -55,6 +55,7 @@ void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
>                                target_ulong pte0, target_ulong pte1);
>   bool kvmppc_has_cap_fixup_hcalls(void);
>   int kvmppc_enable_hwrng(void);
> +int kvmppc_update_sdr1(PowerPCCPU *cpu);
>
>   #else
>
> @@ -246,6 +247,11 @@ static inline int kvmppc_enable_hwrng(void)
>   {
>       return -1;
>   }
> +
> +static inline int kvmppc_update_sdr1(PowerPCCPU *cpu)
> +{
> +    return 0; /* nothing to do */
> +}
>   #endif
>
>   #ifndef CONFIG_KVM
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 9c58fbf..e15d7b0 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -258,6 +258,42 @@ target_ulong helper_load_slb_vsid(CPUPPCState *env, target_ulong rb)
>   /*
>    * 64-bit hash table MMU handling
>    */
> +void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
> +                         Error **errp)
> +{
> +    CPUPPCState *env = &cpu->env;
> +    target_ulong htabsize = value & SDR_64_HTABSIZE;
> +
> +    cpu_synchronize_state(CPU(cpu));
> +
> +    env->spr[SPR_SDR1] = value;
> +    if (htabsize > 28) {
> +        error_setg(errp,
> +                   "Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1",
> +                   htabsize);
> +        htabsize = 28;
> +    }
> +    env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
> +    env->htab_base = value & SDR_64_HTABORG;
> +
> +    if (kvmppc_update_sdr1(cpu) < 0) {
> +        error_setg(errp,
> +                   "Unable to update SDR1 in KVM");
> +    }
> +}
> +
> +void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift)
> +{
> +    CPUPPCState *env = &cpu->env;
> +
> +    env->external_htab = hpt;
> +    ppc_hash64_set_sdr1(cpu, (target_ulong)(uintptr_t)hpt | (shift - 18),
> +                        &error_abort);
> +
> +    /* Not strictly necessary, but makes it clearer that an external
> +     * htab is in use when debugging */
> +    env->htab_base = -1;


imho -1 is not really clearer than 0.


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>





-- 
Alexey

Re: [Qemu-devel] [PATCH 6/6] target-ppc: Add helpers for updating a CPU's SDR1 and external HPT

[Alexey Kardashevskiy]

On 02/05/2016 01:13 PM, David Gibson wrote:
> When a Power cpu with 64-bit hash MMU has it's hash page table (HPT)
> pointer updated by a write to the SDR1 register we need to update some
> derived variables.  Likewise, when the cpu is configured for an external
> HPT (one not in the guest memory space) some derived variables need to be
> updated.
>
> Currently the logic for this is (partially) duplicated in ppc_store_sdr1()
> and in spapr_cpu_reset().  In future we're going to need it in some other
> places, so make some common helpers for this update.
>
> In addition extend the helpers to update SDR1 in KVM - it's not updated
> by the normal runtime KVM<->qemu CPU synchronization.  Currently there
> aren't situations where it matters, but there are going to be in future.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>   hw/ppc/spapr.c          | 12 +-----------
>   target-ppc/kvm.c        | 12 ++++++++++++
>   target-ppc/kvm_ppc.h    |  6 ++++++
>   target-ppc/mmu-hash64.c | 36 ++++++++++++++++++++++++++++++++++++
>   target-ppc/mmu-hash64.h |  4 ++++
>   target-ppc/mmu_helper.c | 13 ++++++-------
>   6 files changed, 65 insertions(+), 18 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 77dd1b6..af3023b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1195,17 +1195,7 @@ static void spapr_cpu_reset(void *opaque)
>
>       env->spr[SPR_HIOR] = 0;
>
> -    env->external_htab = (uint8_t *)spapr->htab;
> -    env->htab_base = -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);
> +    ppc_hash64_set_external_hpt(cpu, spapr->htab, spapr->htab_shift);
>   }
>
>   static void spapr_create_nvram(sPAPRMachineState *spapr)
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 70ca296..8430d43 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2530,3 +2530,15 @@ int kvmppc_enable_hwrng(void)
>
>       return kvmppc_enable_hcall(kvm_state, H_RANDOM);
>   }
> +
> +int kvmppc_update_sdr1(PowerPCCPU *cpu)
> +{
> +    CPUState *cs = CPU(cpu);
> +
> +    if (!kvm_enabled()) {
> +        return 0; /* nothing to do */
> +    }
> +
> +    /* This is overkill, but this shouldn't be a common operation */
> +    return kvm_arch_put_registers(cs, KVM_PUT_RESET_STATE);


I had to look at kvm_cpu_exec() (which also calls kvm_arch_put_registers) 
to realize that you need here KVM_PUT_RESET_STATE and not kvm_cpu_exec's 
KVM_PUT_RUNTIME_STATE, that should go to the comment imho.



> +}
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index aaa828c..434b3d1 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -55,6 +55,7 @@ void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
>                                target_ulong pte0, target_ulong pte1);
>   bool kvmppc_has_cap_fixup_hcalls(void);
>   int kvmppc_enable_hwrng(void);
> +int kvmppc_update_sdr1(PowerPCCPU *cpu);
>
>   #else
>
> @@ -246,6 +247,11 @@ static inline int kvmppc_enable_hwrng(void)
>   {
>       return -1;
>   }
> +
> +static inline int kvmppc_update_sdr1(PowerPCCPU *cpu)
> +{
> +    return 0; /* nothing to do */
> +}
>   #endif
>
>   #ifndef CONFIG_KVM
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 9c58fbf..e15d7b0 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -258,6 +258,42 @@ target_ulong helper_load_slb_vsid(CPUPPCState *env, target_ulong rb)
>   /*
>    * 64-bit hash table MMU handling
>    */
> +void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
> +                         Error **errp)
> +{
> +    CPUPPCState *env = &cpu->env;
> +    target_ulong htabsize = value & SDR_64_HTABSIZE;
> +
> +    cpu_synchronize_state(CPU(cpu));
> +
> +    env->spr[SPR_SDR1] = value;
> +    if (htabsize > 28) {
> +        error_setg(errp,
> +                   "Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1",
> +                   htabsize);
> +        htabsize = 28;
> +    }
> +    env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
> +    env->htab_base = value & SDR_64_HTABORG;
> +
> +    if (kvmppc_update_sdr1(cpu) < 0) {
> +        error_setg(errp,
> +                   "Unable to update SDR1 in KVM");
> +    }
> +}
> +
> +void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift)
> +{
> +    CPUPPCState *env = &cpu->env;
> +
> +    env->external_htab = hpt;
> +    ppc_hash64_set_sdr1(cpu, (target_ulong)(uintptr_t)hpt | (shift - 18),
> +                        &error_abort);
> +
> +    /* Not strictly necessary, but makes it clearer that an external
> +     * htab is in use when debugging */
> +    env->htab_base = -1;


imho -1 is not really clearer than 0.


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>





-- 
Alexey

Re: [Qemu-devel] [PATCH 1/6] target-ppc: Remove unused kvmppc_update_sdr1() stub

[Alexey Kardashevskiy]

On 02/05/2016 01:13 PM, David Gibson wrote:
> This KVM stub implementation isn't used anywhere.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>   target-ppc/kvm_ppc.h | 5 -----
>   1 file changed, 5 deletions(-)
>
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 62406ce..aaa828c 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -184,11 +184,6 @@ static inline uint64_t kvmppc_rma_size(uint64_t current_size,
>       return ram_size;
>   }
>
> -static inline int kvmppc_update_sdr1(CPUPPCState *env)
> -{
> -    return 0;
> -}
> -
>   #endif /* !CONFIG_USER_ONLY */
>
>   static inline bool kvmppc_has_cap_epr(void)
>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>



-- 
Alexey

Re: [Qemu-devel] [PATCH 2/6] target-ppc: Include missing MMU models for SDR1 in info registers

[Alexey Kardashevskiy]

On 02/05/2016 01:13 PM, David Gibson wrote:
> The HMP command "info registers" produces somewhat different information on
> different ppc cpu variants.  For those with a hash MMU it's supposed to
> include the SDR1, DAR and DSISR registers related to the MMU.  However,
> the switch is missing a couple of MMU model variants, meaning we will
> miss out this information on certain CPUs which should have it.
>
> This patch corrects the oversight.  (Really these MMU model IDs need a big
> cleanup, but we might as well fix the bug in the interim).
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>   target-ppc/translate.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 0057bda..287d679 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -11352,7 +11352,9 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
>       case POWERPC_MMU_64B:
>       case POWERPC_MMU_2_03:
>       case POWERPC_MMU_2_06:
> +    case POWERPC_MMU_2_06a:
>       case POWERPC_MMU_2_07:
> +    case POWERPC_MMU_2_07a:
>   #endif
>           cpu_fprintf(f, " SDR1 " TARGET_FMT_lx "   DAR " TARGET_FMT_lx
>                          "  DSISR " TARGET_FMT_lx "\n", env->spr[SPR_SDR1],
>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>



-- 
Alexey

Re: [Qemu-devel] [PATCH 3/6] pseries: Simplify handling of the hash page table fd

[Alexey Kardashevskiy]

On 02/05/2016 01:13 PM, David Gibson wrote:
> When migrating the 'pseries' machine type with KVM, we use a special fd
> to access the hash page table stored within KVM.  Usually, this fd is
> opened at the beginning of migration, and kept open until the migration
> is complete.
>
> However, if there is a guest reset during the migration, the fd can become
> stale and we need to re-open it.  At the moment we use an 'htab_fd_stale'
> flag in sPAPRMachineState to signal this, which is checked in the migration
> iterators.
>
> But that's rather ugly.  It's simpler to just close and invalidate the
> fd on reset, and lazily re-open it in migration if necessary.  This patch
> implements that change.
>
> This requires a small addition to the machine state's instance_init,
> so that htab_fd is initialized to -1 (telling the migration code it
> needs to open it) instead of 0, which could be a valid fd.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>




-- 
Alexey

Re: [Qemu-devel] [PATCH 5/6] target-ppc: Remove hack for ppc_hash64_load_hpte*() with HV KVM

[Alexey Kardashevskiy]

On 02/05/2016 01:13 PM, David Gibson wrote:
> With HV KVM, the guest's hash page table (HPT) is managed by the kernel and
> not directly accessible to QEMU.  This means that spapr->htab is NULL
> and normally env->external_htab would also be NULL for each cpu.
>
> However, that would cause ppc_hash64_load_hpte*() to do the wrong thing in
> the few cases where QEMU does need to load entries from the in-kernel HPT.
> Specifically, seeing external_htab is NULL, they would look for an HPT
> within the guest's address space instead.
>
> To stop that we have an ugly hack in the pseries machine type code to
> set external htab to (void *)1 instead.
>
> This patch removes that hack by having ppc_hash64_load_hpte*() explicitly
> check kvmppc_kern_htab instead, which makes more sense.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

To my limited knowledge of kvmppc_kern_htab&co, this looks good.

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>



> ---
>   hw/ppc/spapr.c          | 7 -------
>   target-ppc/mmu-hash64.h | 4 ++--
>   2 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d228375..77dd1b6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1196,13 +1196,6 @@ 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.
> diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
> index ab0f86b..e7d9925 100644
> --- a/target-ppc/mmu-hash64.h
> +++ b/target-ppc/mmu-hash64.h
> @@ -102,7 +102,7 @@ static inline target_ulong ppc_hash64_load_hpte0(PowerPCCPU *cpu,
>       uint64_t addr;
>
>       addr = token + (index * HASH_PTE_SIZE_64);
> -    if (env->external_htab) {
> +    if (kvmppc_kern_htab || env->external_htab) {
>           return  ldq_p((const void *)(uintptr_t)addr);
>       } else {
>           return ldq_phys(CPU(cpu)->as, addr);


Out of curiosity - how does this work? ldq_p() reads from the userspace address


> @@ -116,7 +116,7 @@ static inline target_ulong ppc_hash64_load_hpte1(PowerPCCPU *cpu,
>       uint64_t addr;
>
>       addr = token + (index * HASH_PTE_SIZE_64) + HASH_PTE_SIZE_64/2;
> -    if (env->external_htab) {
> +    if (kvmppc_kern_htab || env->external_htab) {
>           return  ldq_p((const void *)(uintptr_t)addr);
>       } else {
>           return ldq_phys(CPU(cpu)->as, addr);
>


-- 
Alexey

Re: [Qemu-devel] [PATCH 4/6] pseries: Move hash page table allocation to reset time

[David Gibson]

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

On Mon, Feb 08, 2016 at 03:44:18PM +1100, Alexey Kardashevskiy wrote:
> On 02/05/2016 01:13 PM, David Gibson wrote:
> >At the moment the size of the hash page table (HPT) is fixed based on the
> >maximum memory allowed to the guest.  As such, we allocate the table during
> >machine construction, and just clear it at reset.
> >
> >However, we're planning to implement a PAPR extension allowing the hash
> >page table to be resized at runtime.  This will mean that on reset we want
> >to revert it to the default size.  It also means that when migrating, we
> >need to make sure the destination allocates an HPT of size matching the
> >host, since the guest could have changed it before the migration.
> >
> >This patch replaces the spapr_alloc_htab() and spapr_reset_htab() functions
> >with a new spapr_reallocate_hpt() function.  This is called at reset and
> >inbound migration only, not during machine init any more.
> 
> 
> I'd suggest splitting this patch in two (move spapr_hpt_shift_for_ramsize to
> a new patch, or reorganizing it somehow (move spapr_hpt_shift_for_ramsize to
> a different place) - this one is really hard to read and make sure that
> nothing is lost.

Good idea, I'll do that.

> >In addition, we add a new helper to compute the recommended hash table size
> >for a given RAM size.  We export this as well as spapr_reallocate_hpt(),
> >since we'll be needing them elsewhere in future.
> 
> Do you already have a patch in your queue to exploit this?
> 
> I was told few times to export stuff when I have to...

True, I'll look at that again.

-- 
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: 819 bytes --]

Re: [Qemu-devel] [PATCH 6/6] target-ppc: Add helpers for updating a CPU's SDR1 and external HPT

[David Gibson]

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

On Mon, Feb 08, 2016 at 04:11:13PM +1100, Alexey Kardashevskiy wrote:
> On 02/05/2016 01:13 PM, David Gibson wrote:
> >When a Power cpu with 64-bit hash MMU has it's hash page table (HPT)
> >pointer updated by a write to the SDR1 register we need to update some
> >derived variables.  Likewise, when the cpu is configured for an external
> >HPT (one not in the guest memory space) some derived variables need to be
> >updated.
> >
> >Currently the logic for this is (partially) duplicated in ppc_store_sdr1()
> >and in spapr_cpu_reset().  In future we're going to need it in some other
> >places, so make some common helpers for this update.
> >
> >In addition extend the helpers to update SDR1 in KVM - it's not updated
> >by the normal runtime KVM<->qemu CPU synchronization.  Currently there
> >aren't situations where it matters, but there are going to be in future.
> >
> >Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >---
> >  hw/ppc/spapr.c          | 12 +-----------
> >  target-ppc/kvm.c        | 12 ++++++++++++
> >  target-ppc/kvm_ppc.h    |  6 ++++++
> >  target-ppc/mmu-hash64.c | 36 ++++++++++++++++++++++++++++++++++++
> >  target-ppc/mmu-hash64.h |  4 ++++
> >  target-ppc/mmu_helper.c | 13 ++++++-------
> >  6 files changed, 65 insertions(+), 18 deletions(-)
> >
> >diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >index 77dd1b6..af3023b 100644
> >--- a/hw/ppc/spapr.c
> >+++ b/hw/ppc/spapr.c
> >@@ -1195,17 +1195,7 @@ static void spapr_cpu_reset(void *opaque)
> >
> >      env->spr[SPR_HIOR] = 0;
> >
> >-    env->external_htab = (uint8_t *)spapr->htab;
> >-    env->htab_base = -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);
> >+    ppc_hash64_set_external_hpt(cpu, spapr->htab, spapr->htab_shift);
> >  }
> >
> >  static void spapr_create_nvram(sPAPRMachineState *spapr)
> >diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> >index 70ca296..8430d43 100644
> >--- a/target-ppc/kvm.c
> >+++ b/target-ppc/kvm.c
> >@@ -2530,3 +2530,15 @@ int kvmppc_enable_hwrng(void)
> >
> >      return kvmppc_enable_hcall(kvm_state, H_RANDOM);
> >  }
> >+
> >+int kvmppc_update_sdr1(PowerPCCPU *cpu)
> >+{
> >+    CPUState *cs = CPU(cpu);
> >+
> >+    if (!kvm_enabled()) {
> >+        return 0; /* nothing to do */
> >+    }
> >+
> >+    /* This is overkill, but this shouldn't be a common operation */
> >+    return kvm_arch_put_registers(cs, KVM_PUT_RESET_STATE);
> 
> 
> I had to look at kvm_cpu_exec() (which also calls kvm_arch_put_registers) to
> realize that you need here KVM_PUT_RESET_STATE and not kvm_cpu_exec's
> KVM_PUT_RUNTIME_STATE, that should go to the comment imho.

Ok, I'll update that.

> >+}
> >diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> >index aaa828c..434b3d1 100644
> >--- a/target-ppc/kvm_ppc.h
> >+++ b/target-ppc/kvm_ppc.h
> >@@ -55,6 +55,7 @@ void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
> >                               target_ulong pte0, target_ulong pte1);
> >  bool kvmppc_has_cap_fixup_hcalls(void);
> >  int kvmppc_enable_hwrng(void);
> >+int kvmppc_update_sdr1(PowerPCCPU *cpu);
> >
> >  #else
> >
> >@@ -246,6 +247,11 @@ static inline int kvmppc_enable_hwrng(void)
> >  {
> >      return -1;
> >  }
> >+
> >+static inline int kvmppc_update_sdr1(PowerPCCPU *cpu)
> >+{
> >+    return 0; /* nothing to do */
> >+}
> >  #endif
> >
> >  #ifndef CONFIG_KVM
> >diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> >index 9c58fbf..e15d7b0 100644
> >--- a/target-ppc/mmu-hash64.c
> >+++ b/target-ppc/mmu-hash64.c
> >@@ -258,6 +258,42 @@ target_ulong helper_load_slb_vsid(CPUPPCState *env, target_ulong rb)
> >  /*
> >   * 64-bit hash table MMU handling
> >   */
> >+void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
> >+                         Error **errp)
> >+{
> >+    CPUPPCState *env = &cpu->env;
> >+    target_ulong htabsize = value & SDR_64_HTABSIZE;
> >+
> >+    cpu_synchronize_state(CPU(cpu));
> >+
> >+    env->spr[SPR_SDR1] = value;
> >+    if (htabsize > 28) {
> >+        error_setg(errp,
> >+                   "Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1",
> >+                   htabsize);
> >+        htabsize = 28;
> >+    }
> >+    env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
> >+    env->htab_base = value & SDR_64_HTABORG;
> >+
> >+    if (kvmppc_update_sdr1(cpu) < 0) {
> >+        error_setg(errp,
> >+                   "Unable to update SDR1 in KVM");
> >+    }
> >+}
> >+
> >+void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift)
> >+{
> >+    CPUPPCState *env = &cpu->env;
> >+
> >+    env->external_htab = hpt;
> >+    ppc_hash64_set_sdr1(cpu, (target_ulong)(uintptr_t)hpt | (shift - 18),
> >+                        &error_abort);
> >+
> >+    /* Not strictly necessary, but makes it clearer that an external
> >+     * htab is in use when debugging */
> >+    env->htab_base = -1;
> 
> 
> imho -1 is not really clearer than 0.

It won't be 0 though, because ppc_hash64_set_sdr1() will update it
based on the SDR1 value, which in turn is based on the user address of
the external htab (and PR KVM needs that).
the user address of the external htab.

So, it will look kind of like an address, but won't actually be an
address within the guest AS, which it's supposed to be.  Hence, the
override here.

-- 
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: 819 bytes --]