[Qemu-devel] [RFC PATCH v2 0/4] ppc/spapr: Fix migration of radix guests

[Qemu-devel] [RFC PATCH v2 0/4] ppc/spapr: Fix migration of radix guests

[Bharata B Rao]

This patchset fixes the migration of sPAPR radix guests.

Changes in v1:
--------------
- Dropped the unrequired patch that fixed unregister_savevm().
- Reimplemented unregister_savevm_live() such that the existing
  unregister_savevm() signature isn't modified.
- Folded HTAB savevm unregistration into spapr_free_hpt().
- Obtaining GTSE bit from LPCR now and hence patb_flags needn't be part
  of the migration stream.

v1: https://lists.nongnu.org/archive/html/qemu-devel/2017-05/msg03923.html

Bharata B Rao (4):
  migration: Introduce unregister_savevm_live()
  spapr: Unregister HPT savevm handlers for radix guests
  spapr: Make h_register_process_table hcall flags global
  spapr: Fix migration of Radix guests

 hw/ppc/spapr.c              | 111 +++++++++++++++++++++++++-------------------
 hw/ppc/spapr_hcall.c        |  31 ++++++-------
 include/hw/ppc/spapr.h      |  12 +++++
 include/migration/vmstate.h |   1 +
 migration/savevm.c          |  17 ++++++-
 5 files changed, 104 insertions(+), 68 deletions(-)

-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 1/4] migration: Introduce unregister_savevm_live()

[Bharata B Rao]

Introduce a new function unregister_savevm_live() to unregister the vmstate
handlers registered via register_savevm_live().

register_savevm() allocates SaveVMHandlers while register_savevm_live()
gets passed with SaveVMHandlers. During unregistration, we  want to
free SaveVMHandlers in the former case but not free in the latter case.
Hence this new API is needed to differentiate this.

This new API will be needed by PowerPC to unregister the HTAB savevm
handlers.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 include/migration/vmstate.h |  1 +
 migration/savevm.c          | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 8489659..02a1bac 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -79,6 +79,7 @@ int register_savevm_live(DeviceState *dev,
                          void *opaque);
 
 void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque);
+void unregister_savevm_live(DeviceState *dev, const char *idstr, void *opaque);
 
 typedef struct VMStateInfo VMStateInfo;
 typedef struct VMStateDescription VMStateDescription;
diff --git a/migration/savevm.c b/migration/savevm.c
index f5e8194..4ef6fdc 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -630,7 +630,8 @@ int register_savevm(DeviceState *dev,
                                 ops, opaque);
 }
 
-void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
+static void unregister_savevm_common(DeviceState *dev, const char *idstr,
+                                     void *opaque, bool free_savevmhandlers)
 {
     SaveStateEntry *se, *new_se;
     char id[256] = "";
@@ -649,12 +650,24 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
         if (strcmp(se->idstr, id) == 0 && se->opaque == opaque) {
             QTAILQ_REMOVE(&savevm_state.handlers, se, entry);
             g_free(se->compat);
-            g_free(se->ops);
+            if (free_savevmhandlers) {
+                g_free(se->ops);
+            }
             g_free(se);
         }
     }
 }
 
+void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
+{
+    unregister_savevm_common(dev, idstr, opaque, true);
+}
+
+void unregister_savevm_live(DeviceState *dev, const char *idstr, void *opaque)
+{
+    unregister_savevm_common(dev, idstr, opaque, false);
+}
+
 int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque, int alias_id,
-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 2/4] spapr: Unregister HPT savevm handlers for radix guests

[Bharata B Rao]

HPT gets created by default for TCG guests and later when the guest turns
out to be a radix guest, the HPT is destroyed when guest does
H_REGISTER_PROC_TBL hcall. Let HTAB savevm handlers registration and
unregistration follow the same model so that we don't end up having
unrequired HTAB savevm handlers for radix guests.

This also ensures that HTAB savevm handlers seemlessly get destroyed and
recreated like HTAB itself when hash guest reboots.

HTAB savevm handlers registration/unregistration is now done from
spapr_reallocate_hpt() which itself is called from one of the
savevm_htab_handlers.htab_load(). To cater to this circular dependency
spapr_reallocate_hpt() is made global.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c         | 99 +++++++++++++++++++++++++-------------------------
 include/hw/ppc/spapr.h |  2 +
 2 files changed, 52 insertions(+), 49 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 91f7434..daf335c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1233,53 +1233,7 @@ void spapr_free_hpt(sPAPRMachineState *spapr)
     spapr->htab = NULL;
     spapr->htab_shift = 0;
     close_htab_fd(spapr);
-}
-
-static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
-                                 Error **errp)
-{
-    long rc;
-
-    /* Clean up any HPT info from a previous boot */
-    spapr_free_hpt(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;
-        spapr->htab = NULL;
-    } else {
-        /* kernel-side HPT not needed, allocate in userspace instead */
-        size_t size = 1ULL << shift;
-        int i;
-
-        spapr->htab = qemu_memalign(size, size);
-        if (!spapr->htab) {
-            error_setg_errno(errp, errno,
-                             "Could not allocate HPT of order %d", shift);
-            return;
-        }
-
-        memset(spapr->htab, 0, size);
-        spapr->htab_shift = shift;
-
-        for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
-            DIRTY_HPTE(HPTE(spapr->htab, i));
-        }
-    }
+    unregister_savevm_live(NULL, "spapr/htab", spapr);
 }
 
 void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
@@ -1879,6 +1833,55 @@ static SaveVMHandlers savevm_htab_handlers = {
     .load_state = htab_load,
 };
 
+void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
+                                 Error **errp)
+{
+    long rc;
+
+    /* Clean up any HPT info from a previous boot */
+    spapr_free_hpt(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;
+        spapr->htab = NULL;
+    } else {
+        /* kernel-side HPT not needed, allocate in userspace instead */
+        size_t size = 1ULL << shift;
+        int i;
+
+        spapr->htab = qemu_memalign(size, size);
+        if (!spapr->htab) {
+            error_setg_errno(errp, errno,
+                             "Could not allocate HPT of order %d", shift);
+            return;
+        }
+
+        memset(spapr->htab, 0, size);
+        spapr->htab_shift = shift;
+
+        for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
+            DIRTY_HPTE(HPTE(spapr->htab, i));
+        }
+    }
+    register_savevm_live(NULL, "spapr/htab", -1, 1,
+                         &savevm_htab_handlers, spapr);
+}
+
 static void spapr_boot_set(void *opaque, const char *boot_device,
                            Error **errp)
 {
@@ -2341,8 +2344,6 @@ static void ppc_spapr_init(MachineState *machine)
      * interface, this is a legacy from the sPAPREnvironment structure
      * which predated MachineState but had a similar function */
     vmstate_register(NULL, 0, &vmstate_spapr, spapr);
-    register_savevm_live(NULL, "spapr/htab", -1, 1,
-                         &savevm_htab_handlers, spapr);
 
     /* used by RTAS */
     QTAILQ_INIT(&spapr->ccs_list);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index f875dc4..e581c4a 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -637,6 +637,8 @@ void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType drc_type,
                                                uint32_t count, uint32_t index);
 void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
                                     sPAPRMachineState *spapr);
+void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
+                                 Error **errp);
 
 /* rtas-configure-connector state */
 struct sPAPRConfigureConnectorState {
-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 3/4] spapr: Make h_register_process_table hcall flags global

[Bharata B Rao]

The flags used in h_register_process_table hcall are needed in spapr.c
and hence move them to a header file. While doing so, give them
slightly specific names.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_hcall.c   | 31 ++++++++++++++-----------------
 include/hw/ppc/spapr.h | 10 ++++++++++
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index cea5d99..3915e6f 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -921,13 +921,6 @@ static void spapr_check_setup_free_hpt(sPAPRMachineState *spapr,
     return;
 }
 
-#define FLAGS_MASK              0x01FULL
-#define FLAG_MODIFY             0x10
-#define FLAG_REGISTER           0x08
-#define FLAG_RADIX              0x04
-#define FLAG_HASH_PROC_TBL      0x02
-#define FLAG_GTSE               0x01
-
 static target_ulong h_register_process_table(PowerPCCPU *cpu,
                                              sPAPRMachineState *spapr,
                                              target_ulong opcode,
@@ -940,12 +933,13 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
     target_ulong table_size = args[3];
     uint64_t cproc;
 
-    if (flags & ~FLAGS_MASK) { /* Check no reserved bits are set */
+    if (flags & ~SPAPR_PROC_TABLE_MASK) { /* Check no reserved bits are set */
         return H_PARAMETER;
     }
-    if (flags & FLAG_MODIFY) {
-        if (flags & FLAG_REGISTER) {
-            if (flags & FLAG_RADIX) { /* Register new RADIX process table */
+    if (flags & SPAPR_PROC_TABLE_MODIFY) {
+        if (flags & SPAPR_PROC_TABLE_REGISTER) {
+            if (flags & SPAPR_PROC_TABLE_RADIX) {
+                /* Register new RADIX process table */
                 if (proc_tbl & 0xfff || proc_tbl >> 60) {
                     return H_P2;
                 } else if (page_size) {
@@ -955,7 +949,8 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
                 }
                 cproc = PATBE1_GR | proc_tbl | table_size;
             } else { /* Register new HPT process table */
-                if (flags & FLAG_HASH_PROC_TBL) { /* Hash with Segment Tables */
+                if (flags & SPAPR_PROC_TABLE_HPT_PT) {
+                    /* Hash with Segment Tables */
                     /* TODO - Not Supported */
                     /* Technically caused by flag bits => H_PARAMETER */
                     return H_PARAMETER;
@@ -978,7 +973,8 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
             cproc = spapr->patb_entry & PATBE1_GR;
         }
     } else { /* Maintain current registration */
-        if (!(flags & FLAG_RADIX) != !(spapr->patb_entry & PATBE1_GR)) {
+        if (!(flags & SPAPR_PROC_TABLE_RADIX) !=
+            !(spapr->patb_entry & PATBE1_GR)) {
             /* Technically caused by flag bits => H_PARAMETER */
             return H_PARAMETER; /* Existing Process Table Mismatch */
         }
@@ -993,13 +989,14 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
     /* Update the UPRT and GTSE bits in the LPCR for all cpus */
     CPU_FOREACH(cs) {
         set_spr(cs, SPR_LPCR, LPCR_UPRT | LPCR_GTSE,
-                ((flags & (FLAG_RADIX | FLAG_HASH_PROC_TBL)) ? LPCR_UPRT : 0) |
-                ((flags & FLAG_GTSE) ? LPCR_GTSE : 0));
+                ((flags & (SPAPR_PROC_TABLE_RADIX | SPAPR_PROC_TABLE_HPT_PT)) ?
+                LPCR_UPRT : 0) | ((flags & SPAPR_PROC_TABLE_GTSE) ?
+                SPAPR_PROC_TABLE_GTSE : 0));
     }
 
     if (kvm_enabled()) {
-        return kvmppc_configure_v3_mmu(cpu, flags & FLAG_RADIX,
-                                       flags & FLAG_GTSE, cproc);
+        return kvmppc_configure_v3_mmu(cpu, flags & SPAPR_PROC_TABLE_RADIX,
+                                       flags & SPAPR_PROC_TABLE_GTSE, cproc);
     }
     return H_SUCCESS;
 }
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index e581c4a..588872a 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -685,4 +685,14 @@ int spapr_rng_populate_dt(void *fdt);
 
 void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
 
+/*
+ * Defines for flag value used in H_REGISTER_PROC_TBL hcall.
+ */
+#define SPAPR_PROC_TABLE_MASK        0x01FULL
+#define SPAPR_PROC_TABLE_MODIFY      0x10
+#define SPAPR_PROC_TABLE_REGISTER    0x08
+#define SPAPR_PROC_TABLE_RADIX       0x04
+#define SPAPR_PROC_TABLE_HPT_PT      0x02
+#define SPAPR_PROC_TABLE_GTSE        0x01
+
 #endif /* HW_SPAPR_H */
-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 4/4] spapr: Fix migration of Radix guests

[Bharata B Rao]

Fix migration of radix guests by ensuring that we issue
KVM_PPC_CONFIGURE_V3_MMU for radix case post migration.

Reported-by: Nageswara R Sastry <rnsastry@linux.vnet.ibm.com>
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index daf335c..8f20f14 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1400,6 +1400,18 @@ static int spapr_post_load(void *opaque, int version_id)
         err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
     }
 
+    if (spapr->patb_entry) {
+        PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
+        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
+            err = kvmppc_configure_v3_mmu(cpu, SPAPR_PROC_TABLE_RADIX,
+                ((cpu->env.spr[SPR_LPCR] & LPCR_GTSE) ? SPAPR_PROC_TABLE_GTSE :
+                0), spapr->patb_entry);
+        } else {
+            error_report("Radix guest is unsupported by the host");
+            return -EINVAL;
+        }
+    }
+
     return err;
 }
 
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH v2 4/4] spapr: Fix migration of Radix guests

[Bharata B Rao]

On Fri, May 19, 2017 at 11:10:39AM +0530, Bharata B Rao wrote:
> Fix migration of radix guests by ensuring that we issue
> KVM_PPC_CONFIGURE_V3_MMU for radix case post migration.
> 
> Reported-by: Nageswara R Sastry <rnsastry@linux.vnet.ibm.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index daf335c..8f20f14 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1400,6 +1400,18 @@ static int spapr_post_load(void *opaque, int version_id)
>          err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
>      }
> 
> +    if (spapr->patb_entry) {
> +        PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> +            err = kvmppc_configure_v3_mmu(cpu, SPAPR_PROC_TABLE_RADIX,
> +                ((cpu->env.spr[SPR_LPCR] & LPCR_GTSE) ? SPAPR_PROC_TABLE_GTSE :
> +                0), spapr->patb_entry);

Better to use explicit 'true' and 'false' in the above call. Here is
the updated patch:

>From 937c51cac73b4211ef153c1f5940215960383494 Mon Sep 17 00:00:00 2001
From: Bharata B Rao <bharata@linux.vnet.ibm.com>
Date: Tue, 16 May 2017 12:19:54 +0530
Subject: [RFC PATCH v2.1 4/4] spapr: Fix migration of Radix guests

Fix migration of radix guests by ensuring that we issue
KVM_PPC_CONFIGURE_V3_MMU for radix case post migration.

Reported-by: Nageswara R Sastry <rnsastry@linux.vnet.ibm.com>
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index daf335c..69e184b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1400,6 +1400,18 @@ static int spapr_post_load(void *opaque, int version_id)
         err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
     }
 
+    if (spapr->patb_entry) {
+        PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
+        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
+            err = kvmppc_configure_v3_mmu(cpu, true,
+                ((cpu->env.spr[SPR_LPCR] & LPCR_GTSE) ? true : false),
+                spapr->patb_entry);
+        } else {
+            error_report("Radix guest is unsupported by the host");
+            return -EINVAL;
+        }
+    }
+
     return err;
 }
 
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH v2 1/4] migration: Introduce unregister_savevm_live()

[David Gibson]

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

On Fri, May 19, 2017 at 11:10:36AM +0530, Bharata B Rao wrote:
> Introduce a new function unregister_savevm_live() to unregister the vmstate
> handlers registered via register_savevm_live().
> 
> register_savevm() allocates SaveVMHandlers while register_savevm_live()
> gets passed with SaveVMHandlers. During unregistration, we  want to
> free SaveVMHandlers in the former case but not free in the latter case.
> Hence this new API is needed to differentiate this.
> 
> This new API will be needed by PowerPC to unregister the HTAB savevm
> handlers.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

I could take this through my tree, but it would need an ACK from Dave
Gilbert or Juan Quintela.

> ---
>  include/migration/vmstate.h |  1 +
>  migration/savevm.c          | 17 +++++++++++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 8489659..02a1bac 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -79,6 +79,7 @@ int register_savevm_live(DeviceState *dev,
>                           void *opaque);
>  
>  void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque);
> +void unregister_savevm_live(DeviceState *dev, const char *idstr, void *opaque);
>  
>  typedef struct VMStateInfo VMStateInfo;
>  typedef struct VMStateDescription VMStateDescription;
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f5e8194..4ef6fdc 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -630,7 +630,8 @@ int register_savevm(DeviceState *dev,
>                                  ops, opaque);
>  }
>  
> -void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
> +static void unregister_savevm_common(DeviceState *dev, const char *idstr,
> +                                     void *opaque, bool free_savevmhandlers)
>  {
>      SaveStateEntry *se, *new_se;
>      char id[256] = "";
> @@ -649,12 +650,24 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
>          if (strcmp(se->idstr, id) == 0 && se->opaque == opaque) {
>              QTAILQ_REMOVE(&savevm_state.handlers, se, entry);
>              g_free(se->compat);
> -            g_free(se->ops);
> +            if (free_savevmhandlers) {
> +                g_free(se->ops);
> +            }
>              g_free(se);
>          }
>      }
>  }
>  
> +void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
> +{
> +    unregister_savevm_common(dev, idstr, opaque, true);
> +}
> +
> +void unregister_savevm_live(DeviceState *dev, const char *idstr, void *opaque)
> +{
> +    unregister_savevm_common(dev, idstr, opaque, false);
> +}
> +
>  int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
>                                     const VMStateDescription *vmsd,
>                                     void *opaque, int alias_id,

-- 
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] [RFC PATCH v2 1/4] migration: Introduce unregister_savevm_live()

[Laurent Vivier]

On 19/05/2017 09:33, David Gibson wrote:
> On Fri, May 19, 2017 at 11:10:36AM +0530, Bharata B Rao wrote:
>> Introduce a new function unregister_savevm_live() to unregister the vmstate
>> handlers registered via register_savevm_live().
>>
>> register_savevm() allocates SaveVMHandlers while register_savevm_live()
>> gets passed with SaveVMHandlers. During unregistration, we  want to
>> free SaveVMHandlers in the former case but not free in the latter case.
>> Hence this new API is needed to differentiate this.
>>
>> This new API will be needed by PowerPC to unregister the HTAB savevm
>> handlers.
>>
>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> I could take this through my tree, but it would need an ACK from Dave
> Gilbert or Juan Quintela.

I cc: them for that.

Just a comment on the patch.

Instead of introducing a new function, perhaps we can homogenize the use
of register_savevm() by always providing a SaveVMHandlers pointer and
never a couple of (SaveStateHandler, LoadStateHandler) so the
unregister_save() has never to free se->ops?

Laurent

Re: [Qemu-devel] [RFC PATCH v2 3/4] spapr: Make h_register_process_table hcall flags global

[Laurent Vivier]

On 19/05/2017 07:40, Bharata B Rao wrote:
> The flags used in h_register_process_table hcall are needed in spapr.c
> and hence move them to a header file. While doing so, give them
> slightly specific names.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_hcall.c   | 31 ++++++++++++++-----------------
>  include/hw/ppc/spapr.h | 10 ++++++++++
>  2 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index cea5d99..3915e6f 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -921,13 +921,6 @@ static void spapr_check_setup_free_hpt(sPAPRMachineState *spapr,
>      return;
>  }
>  
> -#define FLAGS_MASK              0x01FULL
> -#define FLAG_MODIFY             0x10
> -#define FLAG_REGISTER           0x08
> -#define FLAG_RADIX              0x04
> -#define FLAG_HASH_PROC_TBL      0x02
> -#define FLAG_GTSE               0x01
> -
...
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index e581c4a..588872a 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -685,4 +685,14 @@ int spapr_rng_populate_dt(void *fdt);
>  
>  void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
>  
> +/*
> + * Defines for flag value used in H_REGISTER_PROC_TBL hcall.
> + */
> +#define SPAPR_PROC_TABLE_MASK        0x01FULL
> +#define SPAPR_PROC_TABLE_MODIFY      0x10
> +#define SPAPR_PROC_TABLE_REGISTER    0x08
> +#define SPAPR_PROC_TABLE_RADIX       0x04
> +#define SPAPR_PROC_TABLE_HPT_PT      0x02
> +#define SPAPR_PROC_TABLE_GTSE        0x01

I think it should be cleaner if you use 0x1fULL

Laurent

Re: [Qemu-devel] [RFC PATCH v2 2/4] spapr: Unregister HPT savevm handlers for radix guests

[David Gibson]

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

On Fri, May 19, 2017 at 11:10:37AM +0530, Bharata B Rao wrote:
> HPT gets created by default for TCG guests and later when the guest turns
> out to be a radix guest, the HPT is destroyed when guest does
> H_REGISTER_PROC_TBL hcall. Let HTAB savevm handlers registration and
> unregistration follow the same model so that we don't end up having
> unrequired HTAB savevm handlers for radix guests.
> 
> This also ensures that HTAB savevm handlers seemlessly get destroyed and
> recreated like HTAB itself when hash guest reboots.
> 
> HTAB savevm handlers registration/unregistration is now done from
> spapr_reallocate_hpt() which itself is called from one of the
> savevm_htab_handlers.htab_load(). To cater to this circular dependency
> spapr_reallocate_hpt() is made global.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>


Weirdly, diff seems to have done a terrible job at minimizing the
patch below.  AFAICT this is really just a one line addition to
spapr_free_hpt() and spapr_reallocate_hpt().

> ---
>  hw/ppc/spapr.c         | 99 +++++++++++++++++++++++++-------------------------
>  include/hw/ppc/spapr.h |  2 +
>  2 files changed, 52 insertions(+), 49 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 91f7434..daf335c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1233,53 +1233,7 @@ void spapr_free_hpt(sPAPRMachineState *spapr)
>      spapr->htab = NULL;
>      spapr->htab_shift = 0;
>      close_htab_fd(spapr);
> -}
> -
> -static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> -                                 Error **errp)
> -{
> -    long rc;
> -
> -    /* Clean up any HPT info from a previous boot */
> -    spapr_free_hpt(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;
> -        spapr->htab = NULL;
> -    } else {
> -        /* kernel-side HPT not needed, allocate in userspace instead */
> -        size_t size = 1ULL << shift;
> -        int i;
> -
> -        spapr->htab = qemu_memalign(size, size);
> -        if (!spapr->htab) {
> -            error_setg_errno(errp, errno,
> -                             "Could not allocate HPT of order %d", shift);
> -            return;
> -        }
> -
> -        memset(spapr->htab, 0, size);
> -        spapr->htab_shift = shift;
> -
> -        for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
> -            DIRTY_HPTE(HPTE(spapr->htab, i));
> -        }
> -    }
> +    unregister_savevm_live(NULL, "spapr/htab", spapr);
>  }
>  
>  void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
> @@ -1879,6 +1833,55 @@ static SaveVMHandlers savevm_htab_handlers = {
>      .load_state = htab_load,
>  };
>  
> +void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> +                                 Error **errp)
> +{
> +    long rc;
> +
> +    /* Clean up any HPT info from a previous boot */
> +    spapr_free_hpt(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;
> +        spapr->htab = NULL;
> +    } else {
> +        /* kernel-side HPT not needed, allocate in userspace instead */
> +        size_t size = 1ULL << shift;
> +        int i;
> +
> +        spapr->htab = qemu_memalign(size, size);
> +        if (!spapr->htab) {
> +            error_setg_errno(errp, errno,
> +                             "Could not allocate HPT of order %d", shift);
> +            return;
> +        }
> +
> +        memset(spapr->htab, 0, size);
> +        spapr->htab_shift = shift;
> +
> +        for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
> +            DIRTY_HPTE(HPTE(spapr->htab, i));
> +        }
> +    }
> +    register_savevm_live(NULL, "spapr/htab", -1, 1,
> +                         &savevm_htab_handlers, spapr);
> +}
> +
>  static void spapr_boot_set(void *opaque, const char *boot_device,
>                             Error **errp)
>  {
> @@ -2341,8 +2344,6 @@ static void ppc_spapr_init(MachineState *machine)
>       * interface, this is a legacy from the sPAPREnvironment structure
>       * which predated MachineState but had a similar function */
>      vmstate_register(NULL, 0, &vmstate_spapr, spapr);
> -    register_savevm_live(NULL, "spapr/htab", -1, 1,
> -                         &savevm_htab_handlers, spapr);
>  
>      /* used by RTAS */
>      QTAILQ_INIT(&spapr->ccs_list);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index f875dc4..e581c4a 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -637,6 +637,8 @@ void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType drc_type,
>                                                 uint32_t count, uint32_t index);
>  void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
>                                      sPAPRMachineState *spapr);
> +void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> +                                 Error **errp);
>  
>  /* rtas-configure-connector state */
>  struct sPAPRConfigureConnectorState {

-- 
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] [RFC PATCH v2 1/4] migration: Introduce unregister_savevm_live()

[David Gibson]

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

On Fri, May 19, 2017 at 03:14:35PM +0200, Laurent Vivier wrote:
> On 19/05/2017 09:33, David Gibson wrote:
> > On Fri, May 19, 2017 at 11:10:36AM +0530, Bharata B Rao wrote:
> >> Introduce a new function unregister_savevm_live() to unregister the vmstate
> >> handlers registered via register_savevm_live().
> >>
> >> register_savevm() allocates SaveVMHandlers while register_savevm_live()
> >> gets passed with SaveVMHandlers. During unregistration, we  want to
> >> free SaveVMHandlers in the former case but not free in the latter case.
> >> Hence this new API is needed to differentiate this.
> >>
> >> This new API will be needed by PowerPC to unregister the HTAB savevm
> >> handlers.
> >>
> >> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > I could take this through my tree, but it would need an ACK from Dave
> > Gilbert or Juan Quintela.
> 
> I cc: them for that.

Dave, Juan: his is fairly urgent, since it's a prereq for (sanely)
fixing a migration bug.  Please let me know what I can do to expedite
review.

> 
> Just a comment on the patch.
> 
> Instead of introducing a new function, perhaps we can homogenize the use
> of register_savevm() by always providing a SaveVMHandlers pointer and
> never a couple of (SaveStateHandler, LoadStateHandler) so the
> unregister_save() has never to free se->ops?

Sounds reasonable to me.  Again, Dave?  Juan?

-- 
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] [RFC PATCH v2 3/4] spapr: Make h_register_process_table hcall flags global

[David Gibson]

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

On Sun, May 21, 2017 at 08:48:42PM +0200, Laurent Vivier wrote:
> On 19/05/2017 07:40, Bharata B Rao wrote:
> > The flags used in h_register_process_table hcall are needed in spapr.c
> > and hence move them to a header file. While doing so, give them
> > slightly specific names.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_hcall.c   | 31 ++++++++++++++-----------------
> >  include/hw/ppc/spapr.h | 10 ++++++++++
> >  2 files changed, 24 insertions(+), 17 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index cea5d99..3915e6f 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -921,13 +921,6 @@ static void spapr_check_setup_free_hpt(sPAPRMachineState *spapr,
> >      return;
> >  }
> >  
> > -#define FLAGS_MASK              0x01FULL
> > -#define FLAG_MODIFY             0x10
> > -#define FLAG_REGISTER           0x08
> > -#define FLAG_RADIX              0x04
> > -#define FLAG_HASH_PROC_TBL      0x02
> > -#define FLAG_GTSE               0x01
> > -
> ...
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index e581c4a..588872a 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -685,4 +685,14 @@ int spapr_rng_populate_dt(void *fdt);
> >  
> >  void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
> >  
> > +/*
> > + * Defines for flag value used in H_REGISTER_PROC_TBL hcall.
> > + */
> > +#define SPAPR_PROC_TABLE_MASK        0x01FULL
> > +#define SPAPR_PROC_TABLE_MODIFY      0x10
> > +#define SPAPR_PROC_TABLE_REGISTER    0x08
> > +#define SPAPR_PROC_TABLE_RADIX       0x04
> > +#define SPAPR_PROC_TABLE_HPT_PT      0x02
> > +#define SPAPR_PROC_TABLE_GTSE        0x01
> 
> I think it should be cleaner if you use 0x1fULL

I agree, but looking at the new version of the final patch, I don't
think this patch will actually be necessary any more.

-- 
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] [RFC PATCH v2 4/4] spapr: Fix migration of Radix guests

[David Gibson]

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

On Fri, May 19, 2017 at 12:06:14PM +0530, Bharata B Rao wrote:
> On Fri, May 19, 2017 at 11:10:39AM +0530, Bharata B Rao wrote:
> > Fix migration of radix guests by ensuring that we issue
> > KVM_PPC_CONFIGURE_V3_MMU for radix case post migration.
> > 
> > Reported-by: Nageswara R Sastry <rnsastry@linux.vnet.ibm.com>
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index daf335c..8f20f14 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1400,6 +1400,18 @@ static int spapr_post_load(void *opaque, int version_id)
> >          err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
> >      }
> > 
> > +    if (spapr->patb_entry) {
> > +        PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> > +            err = kvmppc_configure_v3_mmu(cpu, SPAPR_PROC_TABLE_RADIX,
> > +                ((cpu->env.spr[SPR_LPCR] & LPCR_GTSE) ? SPAPR_PROC_TABLE_GTSE :
> > +                0), spapr->patb_entry);
> 
> Better to use explicit 'true' and 'false' in the above call. Here is
> the updated patch:

Or just !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE) and avoid the ?:
entirely.

With this version you no longer need patch 3/4 AFAICT.

> 
> >From 937c51cac73b4211ef153c1f5940215960383494 Mon Sep 17 00:00:00 2001
> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Date: Tue, 16 May 2017 12:19:54 +0530
> Subject: [RFC PATCH v2.1 4/4] spapr: Fix migration of Radix guests
> 
> Fix migration of radix guests by ensuring that we issue
> KVM_PPC_CONFIGURE_V3_MMU for radix case post migration.
> 
> Reported-by: Nageswara R Sastry <rnsastry@linux.vnet.ibm.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index daf335c..69e184b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1400,6 +1400,18 @@ static int spapr_post_load(void *opaque, int version_id)
>          err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
>      }
>  
> +    if (spapr->patb_entry) {
> +        PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> +            err = kvmppc_configure_v3_mmu(cpu, true,
> +                ((cpu->env.spr[SPR_LPCR] & LPCR_GTSE) ? true : false),
> +                spapr->patb_entry);
> +        } else {
> +            error_report("Radix guest is unsupported by the host");
> +            return -EINVAL;
> +        }
> +    }
> +
>      return err;
>  }
>  

-- 
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] [RFC PATCH v2 4/4] spapr: Fix migration of Radix guests

[Bharata B Rao]

On Mon, May 22, 2017 at 12:44:48PM +1000, David Gibson wrote:
> On Fri, May 19, 2017 at 12:06:14PM +0530, Bharata B Rao wrote:
> > On Fri, May 19, 2017 at 11:10:39AM +0530, Bharata B Rao wrote:
> > > Fix migration of radix guests by ensuring that we issue
> > > KVM_PPC_CONFIGURE_V3_MMU for radix case post migration.
> > > 
> > > Reported-by: Nageswara R Sastry <rnsastry@linux.vnet.ibm.com>
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index daf335c..8f20f14 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1400,6 +1400,18 @@ static int spapr_post_load(void *opaque, int version_id)
> > >          err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
> > >      }
> > > 
> > > +    if (spapr->patb_entry) {
> > > +        PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > > +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> > > +            err = kvmppc_configure_v3_mmu(cpu, SPAPR_PROC_TABLE_RADIX,
> > > +                ((cpu->env.spr[SPR_LPCR] & LPCR_GTSE) ? SPAPR_PROC_TABLE_GTSE :
> > > +                0), spapr->patb_entry);
> > 
> > Better to use explicit 'true' and 'false' in the above call. Here is
> > the updated patch:
> 
> Or just !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE) and avoid the ?:
> entirely.
> 
> With this version you no longer need patch 3/4 AFAICT.

Ah yes, will send the updated version next.

Regards,
Bharata.

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH v2 4/4] spapr: Fix migration of Radix guests

[Suraj Jitindar Singh]

On Fri, 2017-05-19 at 11:10 +0530, Bharata B Rao wrote:
> Fix migration of radix guests by ensuring that we issue
> KVM_PPC_CONFIGURE_V3_MMU for radix case post migration.
> 
> Reported-by: Nageswara R Sastry <rnsastry@linux.vnet.ibm.com>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index daf335c..8f20f14 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1400,6 +1400,18 @@ static int spapr_post_load(void *opaque, int
> version_id)
>          err = spapr_rtc_import_offset(&spapr->rtc, spapr-
> >rtc_offset);
>      }

This will break migration for tcg radix guests.

Given that there is essentially nothing special we need to do on
incoming tcg migration, I suggest we make it:

if (spapr->patb_entry && kvm_enabled()) {
    [snip]
}

>  
> +    if (spapr->patb_entry) {
> +        PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {

Why not make this a bit more generic? Something like:

err = kvmppc_configure_v3_mmu(cpu, !!(spapr->patb_entry & PATBE1_GR),
!!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE), spapr->patb_entry);
if (err) {
    error_report("Process table config unsupported by the host");
    return -EINVAL;
}

return err;

While I don't think it's possible currently, there is nothing
inherently incorrect about having a non-zero process table entry for a
hash guest. And this saves a future update.

> +            err = kvmppc_configure_v3_mmu(cpu,
> SPAPR_PROC_TABLE_RADIX,
> +                ((cpu->env.spr[SPR_LPCR] & LPCR_GTSE) ?
> SPAPR_PROC_TABLE_GTSE :
> +                0), spapr->patb_entry);
> +        } else {
> +            error_report("Radix guest is unsupported by the host");
> +            return -EINVAL;
> +        }
> +    }
> +
>      return err;
>  }
>  

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH v2 4/4] spapr: Fix migration of Radix guests

[Bharata B Rao]

On Mon, May 22, 2017 at 04:30:50PM +1000, Suraj Jitindar Singh wrote:
> On Fri, 2017-05-19 at 11:10 +0530, Bharata B Rao wrote:
> > Fix migration of radix guests by ensuring that we issue
> > KVM_PPC_CONFIGURE_V3_MMU for radix case post migration.
> > 
> > Reported-by: Nageswara R Sastry <rnsastry@linux.vnet.ibm.com>
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index daf335c..8f20f14 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1400,6 +1400,18 @@ static int spapr_post_load(void *opaque, int
> > version_id)
> >          err = spapr_rtc_import_offset(&spapr->rtc, spapr-
> > >rtc_offset);
> >      }
> 
> This will break migration for tcg radix guests.
> 
> Given that there is essentially nothing special we need to do on
> incoming tcg migration, I suggest we make it:
> 
> if (spapr->patb_entry && kvm_enabled()) {
>     [snip]
> }
> 
> >  
> > +    if (spapr->patb_entry) {
> > +        PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> 
> Why not make this a bit more generic? Something like:
> 
> err = kvmppc_configure_v3_mmu(cpu, !!(spapr->patb_entry & PATBE1_GR),
> !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE), spapr->patb_entry);
> if (err) {
>     error_report("Process table config unsupported by the host");
>     return -EINVAL;
> }
> 
> return err;
> 
> While I don't think it's possible currently, there is nothing
> inherently incorrect about having a non-zero process table entry for a
> hash guest. And this saves a future update.

How about having this logic in spapr_post_load() ?

    if (spapr->patb_entry) {
        /* Can be Hash(in future?) or Radix guest (current) */
        PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
        bool radix = !!(spapr->patb_entry & PATBE1_GR);
        bool gtse = !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE);

        if (radix) {
            /* Radix guest, configure MMU */
            /* kvmppc_configure_v3_mmu() is NOP for TCG */
            err = kvmppc_configure_v3_mmu(cpu, radix, gtse, spapr->patb_entry);
            if (err) {
                error_report("Process table config unsupported by the host");
                return -EINVAL;
            }
        } else {
            /* Can be Hash guest (in future ?), nothing to do */
        }
    } else {
        /* Hash guest (current), nothing to do */
    }

Regards,
Bharata.

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH v2 4/4] spapr: Fix migration of Radix guests

[Suraj Jitindar Singh]

On Tue, 2017-05-23 at 10:18 +0530, Bharata B Rao wrote:
> On Mon, May 22, 2017 at 04:30:50PM +1000, Suraj Jitindar Singh wrote:
> > On Fri, 2017-05-19 at 11:10 +0530, Bharata B Rao wrote:
> > > Fix migration of radix guests by ensuring that we issue
> > > KVM_PPC_CONFIGURE_V3_MMU for radix case post migration.
> > > 
> > > Reported-by: Nageswara R Sastry <rnsastry@linux.vnet.ibm.com>
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index daf335c..8f20f14 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1400,6 +1400,18 @@ static int spapr_post_load(void *opaque,
> > > int
> > > version_id)
> > >          err = spapr_rtc_import_offset(&spapr->rtc, spapr-
> > > > rtc_offset);
> > > 
> > >      }
> > 
> > This will break migration for tcg radix guests.
> > 
> > Given that there is essentially nothing special we need to do on
> > incoming tcg migration, I suggest we make it:
> > 
> > if (spapr->patb_entry && kvm_enabled()) {
> >     [snip]
> > }
> > 
> > >  
> > > +    if (spapr->patb_entry) {
> > > +        PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > > +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> > 
> > Why not make this a bit more generic? Something like:
> > 
> > err = kvmppc_configure_v3_mmu(cpu, !!(spapr->patb_entry &
> > PATBE1_GR),
> > !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE), spapr->patb_entry);
> > if (err) {
> >     error_report("Process table config unsupported by the host");
> >     return -EINVAL;
> > }
> > 
> > return err;
> > 
> > While I don't think it's possible currently, there is nothing
> > inherently incorrect about having a non-zero process table entry
> > for a
> > hash guest. And this saves a future update.
> 
> How about having this logic in spapr_post_load() ?

Looks a lot better :)

> 
>     if (spapr->patb_entry) {
>         /* Can be Hash(in future?) or Radix guest (current) */
>         PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
>         bool radix = !!(spapr->patb_entry & PATBE1_GR);
>         bool gtse = !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE);
> 

Don't think we need this if statement though. When hash with patb entry
is possible it will still need to call kvmppc_configure_v3_mmu on
incoming migration, that isn't radix specific.

>         if (radix) {
>             /* Radix guest, configure MMU */
>             /* kvmppc_configure_v3_mmu() is NOP for TCG */
>             err = kvmppc_configure_v3_mmu(cpu, radix, gtse, spapr-
> >patb_entry);
>             if (err) {
>                 error_report("Process table config unsupported by the
> host");
>                 return -EINVAL;
>             }
>         } else {
>             /* Can be Hash guest (in future ?), nothing to do */
>         }
>     } else {

Don't need this else statement. Can just have the comment below if you
feel it's necessary.

>         /* Hash guest (current), nothing to do */
>     }
> 
> Regards,
> Bharata.
> 

Thanks,
Suraj

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH v2 4/4] spapr: Fix migration of Radix guests

[Bharata B Rao]

On Tue, May 23, 2017 at 06:42:28PM +1000, Suraj Jitindar Singh wrote:
> On Tue, 2017-05-23 at 10:18 +0530, Bharata B Rao wrote:
> > On Mon, May 22, 2017 at 04:30:50PM +1000, Suraj Jitindar Singh wrote:
> > > On Fri, 2017-05-19 at 11:10 +0530, Bharata B Rao wrote:
> > > > Fix migration of radix guests by ensuring that we issue
> > > > KVM_PPC_CONFIGURE_V3_MMU for radix case post migration.
> > > > 
> > > > Reported-by: Nageswara R Sastry <rnsastry@linux.vnet.ibm.com>
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  hw/ppc/spapr.c | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index daf335c..8f20f14 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1400,6 +1400,18 @@ static int spapr_post_load(void *opaque,
> > > > int
> > > > version_id)
> > > >          err = spapr_rtc_import_offset(&spapr->rtc, spapr-
> > > > > rtc_offset);
> > > > 
> > > >      }
> > > 
> > > This will break migration for tcg radix guests.
> > > 
> > > Given that there is essentially nothing special we need to do on
> > > incoming tcg migration, I suggest we make it:
> > > 
> > > if (spapr->patb_entry && kvm_enabled()) {
> > >     [snip]
> > > }
> > > 
> > > >  
> > > > +    if (spapr->patb_entry) {
> > > > +        PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > > > +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> > > 
> > > Why not make this a bit more generic? Something like:
> > > 
> > > err = kvmppc_configure_v3_mmu(cpu, !!(spapr->patb_entry &
> > > PATBE1_GR),
> > > !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE), spapr->patb_entry);
> > > if (err) {
> > >     error_report("Process table config unsupported by the host");
> > >     return -EINVAL;
> > > }
> > > 
> > > return err;
> > > 
> > > While I don't think it's possible currently, there is nothing
> > > inherently incorrect about having a non-zero process table entry
> > > for a
> > > hash guest. And this saves a future update.
> > 
> > How about having this logic in spapr_post_load() ?
> 
> Looks a lot better :)
> 
> > 
> >     if (spapr->patb_entry) {
> >         /* Can be Hash(in future?) or Radix guest (current) */
> >         PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> >         bool radix = !!(spapr->patb_entry & PATBE1_GR);
> >         bool gtse = !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE);
> > 
> 
> Don't think we need this if statement though. When hash with patb entry
> is possible it will still need to call kvmppc_configure_v3_mmu on
> incoming migration, that isn't radix specific.

Right.

> 
> >         if (radix) {
> >             /* Radix guest, configure MMU */
> >             /* kvmppc_configure_v3_mmu() is NOP for TCG */
> >             err = kvmppc_configure_v3_mmu(cpu, radix, gtse, spapr-
> > >patb_entry);
> >             if (err) {
> >                 error_report("Process table config unsupported by the
> > host");
> >                 return -EINVAL;
> >             }
> >         } else {
> >             /* Can be Hash guest (in future ?), nothing to do */
> >         }
> >     } else {
> 
> Don't need this else statement. Can just have the comment below if you
> feel it's necessary.

Was just being verbose with empty else blocks, will not have them in the
actual patch.

Thanks for the review.

Regards,
Bharata.