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

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

[Bharata B Rao]

This patchset fixes the migration of sPAPR radix guests.

Changes in v1:
--------------
- Added two patches to fix generic savevm unregistration issues.
- HTAB savevm handlers are now registered/unregistered when HTAB
  is created/destroyed instead of doing this in CAS call (as in v0).

TODO:
- I have checks in spapr_post_load() to detect and fail the migration
  of radix guest to a host that doesn't support radix. However I couldn't
  test this as I am hitting some other unrelated migration failure
  when testing this path.
- I have tested may scenarios like
  - tcg hash->hash, radix->radix, hash->radix and radix->hash reboot
  - kvm hash reboot and migration
  - kvm radix reboot and migration
  However boot->reboot->migration of radix guest doesn't complete
  and this seems to be a different issue to be fixed.

v0: https://lists.gnu.org/archive/html/qemu-ppc/2017-05/msg00197.html

Bharata B Rao (6):
  migration: Fix unregister_savevm()
  migration: Introduce unregister_savevm_live()
  spapr: Make h_register_process_table hcall flags global
  spapr: Consolidate HPT freeing code into a routine
  spapr: Unregister HPT savevm handlers for radix guests
  spapr: Fix migration of Radix guests

 hw/net/vmxnet3.c            |  2 +-
 hw/ppc/spapr.c              | 43 +++++++++++++++++++++++++++++++++++++------
 hw/ppc/spapr_hcall.c        | 38 +++++++++++++++++---------------------
 hw/s390x/s390-skeys.c       |  2 +-
 include/hw/ppc/spapr.h      | 14 ++++++++++++++
 include/migration/vmstate.h |  4 +++-
 migration/savevm.c          | 16 +++++++++++++---
 slirp/slirp.c               |  2 +-
 8 files changed, 87 insertions(+), 34 deletions(-)

-- 
2.7.4

[Qemu-devel] [RFC PATCH v1 1/6] migration: Fix unregister_savevm()

[Bharata B Rao]

In unregister_savevm(), free se->compat only if it was allocated earlier.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 migration/savevm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 352a8f2..7a268ec 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -648,7 +648,9 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
     QTAILQ_FOREACH_SAFE(se, &savevm_state.handlers, entry, new_se) {
         if (strcmp(se->idstr, id) == 0 && se->opaque == opaque) {
             QTAILQ_REMOVE(&savevm_state.handlers, se, entry);
-            g_free(se->compat);
+            if (dev) {
+                g_free(se->compat);
+            }
             g_free(se->ops);
             g_free(se);
         }
-- 
2.7.4

[Qemu-devel] [RFC PATCH v1 2/6] 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>
---
 hw/net/vmxnet3.c            |  2 +-
 hw/s390x/s390-skeys.c       |  2 +-
 include/migration/vmstate.h |  4 +++-
 migration/savevm.c          | 12 ++++++++++--
 slirp/slirp.c               |  2 +-
 5 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 8b1fab2..2b923be 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2350,7 +2350,7 @@ static void vmxnet3_pci_uninit(PCIDevice *pci_dev)
 
     VMW_CBPRN("Starting uninit...");
 
-    unregister_savevm(dev, "vmxnet3-msix", s);
+    unregister_savevm(dev, "vmxnet3-msix", s, false);
 
     vmxnet3_net_uninit(s);
 
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index e2d4e1a..32b6435 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -379,7 +379,7 @@ static inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
         register_savevm(NULL, TYPE_S390_SKEYS, 0, 1, s390_storage_keys_save,
                         s390_storage_keys_load, ss);
     } else {
-        unregister_savevm(DEVICE(ss), TYPE_S390_SKEYS, ss);
+        unregister_savevm(DEVICE(ss), TYPE_S390_SKEYS, ss, false);
     }
 }
 
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index f4bf3f1..ba81b3e 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -78,7 +78,9 @@ int register_savevm_live(DeviceState *dev,
                          SaveVMHandlers *ops,
                          void *opaque);
 
-void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque);
+void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque,
+                       bool live);
+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 7a268ec..fa7c3db 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)
+void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque,
+                       bool live)
 {
     SaveStateEntry *se, *new_se;
     char id[256] = "";
@@ -651,12 +652,19 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
             if (dev) {
                 g_free(se->compat);
             }
-            g_free(se->ops);
+            if (!live) {
+                g_free(se->ops);
+            }
             g_free(se);
         }
     }
 }
 
+void unregister_savevm_live(DeviceState *dev, const char *idstr, void *opaque)
+{
+    unregister_savevm(dev, idstr, opaque, true);
+}
+
 int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque, int alias_id,
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 2f2ec2c..108e669 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -333,7 +333,7 @@ void slirp_cleanup(Slirp *slirp)
 {
     QTAILQ_REMOVE(&slirp_instances, slirp, entry);
 
-    unregister_savevm(NULL, "slirp", slirp);
+    unregister_savevm(NULL, "slirp", slirp, false);
 
     ip_cleanup(slirp);
     ip6_cleanup(slirp);
-- 
2.7.4

[Qemu-devel] [RFC PATCH v1 3/6] 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 0d608d6..3600b0e 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -924,13 +924,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,
@@ -943,12 +936,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) {
@@ -958,7 +952,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;
@@ -981,7 +976,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 */
         }
@@ -996,13 +992,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 5802f88..a692e63 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -681,4 +681,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 v1 4/6] spapr: Consolidate HPT freeing code into a routine

[Bharata B Rao]

Consolidate the code that frees HPT into a separate routine
spapr_free_hpt() as the same chunk of code is called from two places.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c         | 13 +++++++++----
 hw/ppc/spapr_hcall.c   |  5 +----
 include/hw/ppc/spapr.h |  1 +
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 1b7cada..521eef1 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1222,16 +1222,21 @@ static int spapr_hpt_shift_for_ramsize(uint64_t ramsize)
     return shift;
 }
 
+void spapr_free_hpt(sPAPRMachineState *spapr)
+{
+    g_free(spapr->htab);
+    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 */
-    g_free(spapr->htab);
-    spapr->htab = NULL;
-    spapr->htab_shift = 0;
-    close_htab_fd(spapr);
+    spapr_free_hpt(spapr);
 
     rc = kvmppc_reset_htab(shift);
     if (rc < 0) {
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 3600b0e..be79e3d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -913,10 +913,7 @@ static void spapr_check_setup_free_hpt(sPAPRMachineState *spapr,
         /* We assume RADIX, so this catches all the "Do Nothing" cases */
     } else if (!(patbe_old & PATBE1_GR)) {
         /* HASH->RADIX : Free HPT */
-        g_free(spapr->htab);
-        spapr->htab = NULL;
-        spapr->htab_shift = 0;
-        close_htab_fd(spapr);
+        spapr_free_hpt(spapr);
     } else if (!(patbe_new & PATBE1_GR)) {
         /* RADIX->HASH || NOTHING->HASH : Allocate HPT */
         spapr_setup_hpt_and_vrma(spapr);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index a692e63..6f9cb85 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -610,6 +610,7 @@ int spapr_h_cas_compose_response(sPAPRMachineState *sm,
                                  sPAPROptionVector *ov5_updates);
 void close_htab_fd(sPAPRMachineState *spapr);
 void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr);
+void spapr_free_hpt(sPAPRMachineState *spapr);
 sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn);
 void spapr_tce_table_enable(sPAPRTCETable *tcet,
                             uint32_t page_shift, uint64_t bus_offset,
-- 
2.7.4

[Qemu-devel] [RFC PATCH v1 5/6] spapr: Unregister HPT savevm handlers for radix guests

[Bharata B Rao]

HPT gets created by default 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.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 521eef1..05abfc1 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1237,6 +1237,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
 
     /* Clean up any HPT info from a previous boot */
     spapr_free_hpt(spapr);
+    spapr_htab_savevm_unregister(spapr);
 
     rc = kvmppc_reset_htab(shift);
     if (rc < 0) {
@@ -1275,6 +1276,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
             DIRTY_HPTE(HPTE(spapr->htab, i));
         }
     }
+    spapr_htab_savevm_register(spapr);
 }
 
 void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
@@ -1874,6 +1876,17 @@ static SaveVMHandlers savevm_htab_handlers = {
     .load_state = htab_load,
 };
 
+void spapr_htab_savevm_register(sPAPRMachineState *spapr)
+{
+    register_savevm_live(NULL, "spapr/htab", -1, 1,
+                         &savevm_htab_handlers, spapr);
+}
+
+void spapr_htab_savevm_unregister(sPAPRMachineState *spapr)
+{
+    unregister_savevm_live(NULL, "spapr/htab", spapr);
+}
+
 static void spapr_boot_set(void *opaque, const char *boot_device,
                            Error **errp)
 {
@@ -2336,8 +2349,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/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index be79e3d..768aa57 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -914,6 +914,7 @@ static void spapr_check_setup_free_hpt(sPAPRMachineState *spapr,
     } else if (!(patbe_old & PATBE1_GR)) {
         /* HASH->RADIX : Free HPT */
         spapr_free_hpt(spapr);
+        spapr_htab_savevm_unregister(spapr);
     } else if (!(patbe_new & PATBE1_GR)) {
         /* RADIX->HASH || NOTHING->HASH : Allocate HPT */
         spapr_setup_hpt_and_vrma(spapr);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 6f9cb85..5b39a26 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -636,6 +636,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_htab_savevm_register(sPAPRMachineState *spapr);
+void spapr_htab_savevm_unregister(sPAPRMachineState *spapr);
 
 /* rtas-configure-connector state */
 struct sPAPRConfigureConnectorState {
-- 
2.7.4

[Qemu-devel] [RFC PATCH v1 6/6] 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         | 15 +++++++++++++++
 hw/ppc/spapr_hcall.c   |  1 +
 include/hw/ppc/spapr.h |  1 +
 3 files changed, 17 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 05abfc1..dd1d687 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1443,6 +1443,20 @@ static int spapr_post_load(void *opaque, int version_id)
         err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
     }
 
+    if (spapr->patb_entry) {
+        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
+            err = kvmppc_configure_v3_mmu(POWERPC_CPU(first_cpu),
+                                          spapr->patb_flags &
+                                          SPAPR_PROC_TABLE_RADIX,
+                                          spapr->patb_flags &
+                                          SPAPR_PROC_TABLE_GTSE,
+                                          spapr->patb_entry);
+        } else {
+            error_report("Radix guest is unsupported by the host");
+            return -EINVAL;
+        }
+    }
+
     return err;
 }
 
@@ -1527,6 +1541,7 @@ static const VMStateDescription vmstate_spapr_patb_entry = {
     .needed = spapr_patb_entry_needed,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(patb_entry, sPAPRMachineState),
+        VMSTATE_UINT64(patb_flags, sPAPRMachineState),
         VMSTATE_END_OF_LIST()
     },
 };
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 768aa57..b002fae 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -986,6 +986,7 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
     spapr_check_setup_free_hpt(spapr, spapr->patb_entry, cproc);
 
     spapr->patb_entry = cproc; /* Save new process table */
+    spapr->patb_flags = flags; /* Save the flags */
 
     /* Update the UPRT and GTSE bits in the LPCR for all cpus */
     CPU_FOREACH(cs) {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5b39a26..c25a32e 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -75,6 +75,7 @@ struct sPAPRMachineState {
     void *htab;
     uint32_t htab_shift;
     uint64_t patb_entry; /* Process tbl registed in H_REGISTER_PROCESS_TABLE */
+    uint64_t patb_flags;
     hwaddr rma_size;
     int vrma_adjust;
     ssize_t rtas_size;
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH v1 1/6] migration: Fix unregister_savevm()

[David Gibson]

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

On Wed, May 17, 2017 at 09:19:17AM +0530, Bharata B Rao wrote:
> In unregister_savevm(), free se->compat only if it was allocated earlier.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

I don't think this is necessary.  If se->compat was never allocated,
then it should be NULL (since se is allocated with g_new0()).
g_free() is explicitly safe to call on NULL, and we already rely on
that in qemu.

> ---
>  migration/savevm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 352a8f2..7a268ec 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -648,7 +648,9 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
>      QTAILQ_FOREACH_SAFE(se, &savevm_state.handlers, entry, new_se) {
>          if (strcmp(se->idstr, id) == 0 && se->opaque == opaque) {
>              QTAILQ_REMOVE(&savevm_state.handlers, se, entry);
> -            g_free(se->compat);
> +            if (dev) {
> +                g_free(se->compat);
> +            }
>              g_free(se->ops);
>              g_free(se);
>          }

-- 
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 v1 2/6] migration: Introduce unregister_savevm_live()

[David Gibson]

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

On Wed, May 17, 2017 at 09:19:18AM +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>

It's not my bailiwick, so I don't get final say, but I dislike
changing the signature of the existing unregister_savevm() interface.
I think it would be preferable to only add the 'live' paramter to a
new unregister_savevm_common() (or whatever) function.

> ---
>  hw/net/vmxnet3.c            |  2 +-
>  hw/s390x/s390-skeys.c       |  2 +-
>  include/migration/vmstate.h |  4 +++-
>  migration/savevm.c          | 12 ++++++++++--
>  slirp/slirp.c               |  2 +-
>  5 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 8b1fab2..2b923be 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2350,7 +2350,7 @@ static void vmxnet3_pci_uninit(PCIDevice *pci_dev)
>  
>      VMW_CBPRN("Starting uninit...");
>  
> -    unregister_savevm(dev, "vmxnet3-msix", s);
> +    unregister_savevm(dev, "vmxnet3-msix", s, false);
>  
>      vmxnet3_net_uninit(s);
>  
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index e2d4e1a..32b6435 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -379,7 +379,7 @@ static inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
>          register_savevm(NULL, TYPE_S390_SKEYS, 0, 1, s390_storage_keys_save,
>                          s390_storage_keys_load, ss);
>      } else {
> -        unregister_savevm(DEVICE(ss), TYPE_S390_SKEYS, ss);
> +        unregister_savevm(DEVICE(ss), TYPE_S390_SKEYS, ss, false);
>      }
>  }
>  
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index f4bf3f1..ba81b3e 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -78,7 +78,9 @@ int register_savevm_live(DeviceState *dev,
>                           SaveVMHandlers *ops,
>                           void *opaque);
>  
> -void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque);
> +void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque,
> +                       bool live);
> +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 7a268ec..fa7c3db 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)
> +void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque,
> +                       bool live)
>  {
>      SaveStateEntry *se, *new_se;
>      char id[256] = "";
> @@ -651,12 +652,19 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
>              if (dev) {
>                  g_free(se->compat);
>              }
> -            g_free(se->ops);
> +            if (!live) {
> +                g_free(se->ops);
> +            }
>              g_free(se);
>          }
>      }
>  }
>  
> +void unregister_savevm_live(DeviceState *dev, const char *idstr, void *opaque)
> +{
> +    unregister_savevm(dev, idstr, opaque, true);
> +}
> +
>  int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
>                                     const VMStateDescription *vmsd,
>                                     void *opaque, int alias_id,
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 2f2ec2c..108e669 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -333,7 +333,7 @@ void slirp_cleanup(Slirp *slirp)
>  {
>      QTAILQ_REMOVE(&slirp_instances, slirp, entry);
>  
> -    unregister_savevm(NULL, "slirp", slirp);
> +    unregister_savevm(NULL, "slirp", slirp, false);
>  
>      ip_cleanup(slirp);
>      ip6_cleanup(slirp);

-- 
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 v1 4/6] spapr: Consolidate HPT freeing code into a routine

[David Gibson]

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

On Wed, May 17, 2017 at 09:19:20AM +0530, Bharata B Rao wrote:
> Consolidate the code that frees HPT into a separate routine
> spapr_free_hpt() as the same chunk of code is called from two places.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Applied to ppc-for-2.10.  This is a good clean up in its own right.
Plus it will be useful to me to get my HPT resizing patches integrated
with radix support.

> ---
>  hw/ppc/spapr.c         | 13 +++++++++----
>  hw/ppc/spapr_hcall.c   |  5 +----
>  include/hw/ppc/spapr.h |  1 +
>  3 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1b7cada..521eef1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1222,16 +1222,21 @@ static int spapr_hpt_shift_for_ramsize(uint64_t ramsize)
>      return shift;
>  }
>  
> +void spapr_free_hpt(sPAPRMachineState *spapr)
> +{
> +    g_free(spapr->htab);
> +    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 */
> -    g_free(spapr->htab);
> -    spapr->htab = NULL;
> -    spapr->htab_shift = 0;
> -    close_htab_fd(spapr);
> +    spapr_free_hpt(spapr);
>  
>      rc = kvmppc_reset_htab(shift);
>      if (rc < 0) {
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 3600b0e..be79e3d 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -913,10 +913,7 @@ static void spapr_check_setup_free_hpt(sPAPRMachineState *spapr,
>          /* We assume RADIX, so this catches all the "Do Nothing" cases */
>      } else if (!(patbe_old & PATBE1_GR)) {
>          /* HASH->RADIX : Free HPT */
> -        g_free(spapr->htab);
> -        spapr->htab = NULL;
> -        spapr->htab_shift = 0;
> -        close_htab_fd(spapr);
> +        spapr_free_hpt(spapr);
>      } else if (!(patbe_new & PATBE1_GR)) {
>          /* RADIX->HASH || NOTHING->HASH : Allocate HPT */
>          spapr_setup_hpt_and_vrma(spapr);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index a692e63..6f9cb85 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -610,6 +610,7 @@ int spapr_h_cas_compose_response(sPAPRMachineState *sm,
>                                   sPAPROptionVector *ov5_updates);
>  void close_htab_fd(sPAPRMachineState *spapr);
>  void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr);
> +void spapr_free_hpt(sPAPRMachineState *spapr);
>  sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn);
>  void spapr_tce_table_enable(sPAPRTCETable *tcet,
>                              uint32_t page_shift, uint64_t bus_offset,

-- 
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 v1 5/6] spapr: Unregister HPT savevm handlers for radix guests

[David Gibson]

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

On Wed, May 17, 2017 at 09:19:21AM +0530, Bharata B Rao wrote:
> HPT gets created by default and later when the guest turns out to be
> a radix guest, the HPT is destroyed when guest does H_REGISTER_PROC_TBL
> hcall.

I don't think that's entirely accurate.  At least in some KVM
configurations, we assume radix first, and only allocate the HPT once
the guest confirms it is doing hash.

> 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.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c         | 15 +++++++++++++--
>  hw/ppc/spapr_hcall.c   |  1 +
>  include/hw/ppc/spapr.h |  2 ++
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 521eef1..05abfc1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1237,6 +1237,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
>  
>      /* Clean up any HPT info from a previous boot */
>      spapr_free_hpt(spapr);
> +    spapr_htab_savevm_unregister(spapr);

I'd prefer that the unregister be folded into spapr_free_hpt().
Basically we want calls that create or remove the HPT and handle
everything - allocation/freeing if necessary, informing KVM if
necessary, and registering/deregistering the savevm handlers if
necesary.

I think that will also remove the need for the trivial
spapr_htab_savevm_{un,}register() wrappers.

>  
>      rc = kvmppc_reset_htab(shift);
>      if (rc < 0) {
> @@ -1275,6 +1276,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
>              DIRTY_HPTE(HPTE(spapr->htab, i));
>          }
>      }
> +    spapr_htab_savevm_register(spapr);
>  }
>  
>  void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
> @@ -1874,6 +1876,17 @@ static SaveVMHandlers savevm_htab_handlers = {
>      .load_state = htab_load,
>  };
>  
> +void spapr_htab_savevm_register(sPAPRMachineState *spapr)
> +{
> +    register_savevm_live(NULL, "spapr/htab", -1, 1,
> +                         &savevm_htab_handlers, spapr);
> +}
> +
> +void spapr_htab_savevm_unregister(sPAPRMachineState *spapr)
> +{
> +    unregister_savevm_live(NULL, "spapr/htab", spapr);
> +}
> +
>  static void spapr_boot_set(void *opaque, const char *boot_device,
>                             Error **errp)
>  {
> @@ -2336,8 +2349,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/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index be79e3d..768aa57 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -914,6 +914,7 @@ static void spapr_check_setup_free_hpt(sPAPRMachineState *spapr,
>      } else if (!(patbe_old & PATBE1_GR)) {
>          /* HASH->RADIX : Free HPT */
>          spapr_free_hpt(spapr);
> +        spapr_htab_savevm_unregister(spapr);
>      } else if (!(patbe_new & PATBE1_GR)) {
>          /* RADIX->HASH || NOTHING->HASH : Allocate HPT */
>          spapr_setup_hpt_and_vrma(spapr);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 6f9cb85..5b39a26 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -636,6 +636,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_htab_savevm_register(sPAPRMachineState *spapr);
> +void spapr_htab_savevm_unregister(sPAPRMachineState *spapr);
>  
>  /* 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 v1 6/6] spapr: Fix migration of Radix guests

[David Gibson]

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

On Wed, May 17, 2017 at 09:19:22AM +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         | 15 +++++++++++++++
>  hw/ppc/spapr_hcall.c   |  1 +
>  include/hw/ppc/spapr.h |  1 +
>  3 files changed, 17 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 05abfc1..dd1d687 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1443,6 +1443,20 @@ static int spapr_post_load(void *opaque, int version_id)
>          err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
>      }
>  
> +    if (spapr->patb_entry) {
> +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> +            err = kvmppc_configure_v3_mmu(POWERPC_CPU(first_cpu),
> +                                          spapr->patb_flags &
> +                                          SPAPR_PROC_TABLE_RADIX,
> +                                          spapr->patb_flags &
> +                                          SPAPR_PROC_TABLE_GTSE,

You should be able to work out the things you need here from
patb_entry without adding the new patb_flags field.

> +                                          spapr->patb_entry);
> +        } else {
> +            error_report("Radix guest is unsupported by the host");
> +            return -EINVAL;
> +        }
> +    }
> +
>      return err;
>  }
>  
> @@ -1527,6 +1541,7 @@ static const VMStateDescription vmstate_spapr_patb_entry = {
>      .needed = spapr_patb_entry_needed,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT64(patb_entry, sPAPRMachineState),
> +        VMSTATE_UINT64(patb_flags, sPAPRMachineState),
>          VMSTATE_END_OF_LIST()
>      },
>  };
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 768aa57..b002fae 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -986,6 +986,7 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
>      spapr_check_setup_free_hpt(spapr, spapr->patb_entry, cproc);
>  
>      spapr->patb_entry = cproc; /* Save new process table */
> +    spapr->patb_flags = flags; /* Save the flags */
>  
>      /* Update the UPRT and GTSE bits in the LPCR for all cpus */
>      CPU_FOREACH(cs) {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5b39a26..c25a32e 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -75,6 +75,7 @@ struct sPAPRMachineState {
>      void *htab;
>      uint32_t htab_shift;
>      uint64_t patb_entry; /* Process tbl registed in H_REGISTER_PROCESS_TABLE */
> +    uint64_t patb_flags;
>      hwaddr rma_size;
>      int vrma_adjust;
>      ssize_t rtas_size;

-- 
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 v1 6/6] spapr: Fix migration of Radix guests

[Bharata B Rao]

On Wed, May 17, 2017 at 05:00:49PM +1000, David Gibson wrote:
> On Wed, May 17, 2017 at 09:19:22AM +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         | 15 +++++++++++++++
> >  hw/ppc/spapr_hcall.c   |  1 +
> >  include/hw/ppc/spapr.h |  1 +
> >  3 files changed, 17 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 05abfc1..dd1d687 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1443,6 +1443,20 @@ static int spapr_post_load(void *opaque, int version_id)
> >          err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
> >      }
> >  
> > +    if (spapr->patb_entry) {
> > +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> > +            err = kvmppc_configure_v3_mmu(POWERPC_CPU(first_cpu),
> > +                                          spapr->patb_flags &
> > +                                          SPAPR_PROC_TABLE_RADIX,
> > +                                          spapr->patb_flags &
> > +                                          SPAPR_PROC_TABLE_GTSE,
> 
> You should be able to work out the things you need here from
> patb_entry without adding the new patb_flags field.

kvmppc_configure_v3_mmu() needs two bools: radix and gtse. The radix
bit can be implied from patb_entry, I needed patb_flags to get the
gtse value. Not immediately obvious of how to get gtse bit from patb_entry,
but let me take a relook.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v1 5/6] spapr: Unregister HPT savevm handlers for radix guests

[Bharata B Rao]

On Wed, May 17, 2017 at 04:59:33PM +1000, David Gibson wrote:
> On Wed, May 17, 2017 at 09:19:21AM +0530, Bharata B Rao wrote:
> > HPT gets created by default and later when the guest turns out to be
> > a radix guest, the HPT is destroyed when guest does H_REGISTER_PROC_TBL
> > hcall.
> 
> I don't think that's entirely accurate.  At least in some KVM
> configurations, we assume radix first, and only allocate the HPT once
> the guest confirms it is doing hash.

Right, that statement is true for TCG radix guests, will rephrase it.

> 
> > 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.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c         | 15 +++++++++++++--
> >  hw/ppc/spapr_hcall.c   |  1 +
> >  include/hw/ppc/spapr.h |  2 ++
> >  3 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 521eef1..05abfc1 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1237,6 +1237,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> >  
> >      /* Clean up any HPT info from a previous boot */
> >      spapr_free_hpt(spapr);
> > +    spapr_htab_savevm_unregister(spapr);
> 
> I'd prefer that the unregister be folded into spapr_free_hpt().
> Basically we want calls that create or remove the HPT and handle
> everything - allocation/freeing if necessary, informing KVM if
> necessary, and registering/deregistering the savevm handlers if
> necesary.
> 
> I think that will also remove the need for the trivial
> spapr_htab_savevm_{un,}register() wrappers.

Sure, will consolidate this in the next version.

Regards,
Bharata.

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

[David Gibson]

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

On Wed, May 17, 2017 at 12:45:39PM +0530, Bharata B Rao wrote:
> On Wed, May 17, 2017 at 05:00:49PM +1000, David Gibson wrote:
> > On Wed, May 17, 2017 at 09:19:22AM +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         | 15 +++++++++++++++
> > >  hw/ppc/spapr_hcall.c   |  1 +
> > >  include/hw/ppc/spapr.h |  1 +
> > >  3 files changed, 17 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 05abfc1..dd1d687 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1443,6 +1443,20 @@ static int spapr_post_load(void *opaque, int version_id)
> > >          err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
> > >      }
> > >  
> > > +    if (spapr->patb_entry) {
> > > +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> > > +            err = kvmppc_configure_v3_mmu(POWERPC_CPU(first_cpu),
> > > +                                          spapr->patb_flags &
> > > +                                          SPAPR_PROC_TABLE_RADIX,
> > > +                                          spapr->patb_flags &
> > > +                                          SPAPR_PROC_TABLE_GTSE,
> > 
> > You should be able to work out the things you need here from
> > patb_entry without adding the new patb_flags field.
> 
> kvmppc_configure_v3_mmu() needs two bools: radix and gtse. The radix
> bit can be implied from patb_entry, I needed patb_flags to get the
> gtse value. Not immediately obvious of how to get gtse bit from patb_entry,
> but let me take a relook.

Oh, sorry, you can't get GTSE from the patb_entry, but you can get it
from the LPCR.

-- 
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 v1 2/6] migration: Introduce unregister_savevm_live()

[Bharata B Rao]

On Wed, May 17, 2017 at 04:45:44PM +1000, David Gibson wrote:
> On Wed, May 17, 2017 at 09:19:18AM +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>
> 
> It's not my bailiwick, so I don't get final say, but I dislike
> changing the signature of the existing unregister_savevm() interface.
> I think it would be preferable to only add the 'live' paramter to a
> new unregister_savevm_common() (or whatever) function.

Yeah, much better, let me change this in next version.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v1 5/6] spapr: Unregister HPT savevm handlers for radix guests

[David Gibson]

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

On Wed, May 17, 2017 at 12:48:21PM +0530, Bharata B Rao wrote:
> On Wed, May 17, 2017 at 04:59:33PM +1000, David Gibson wrote:
> > On Wed, May 17, 2017 at 09:19:21AM +0530, Bharata B Rao wrote:
> > > HPT gets created by default and later when the guest turns out to be
> > > a radix guest, the HPT is destroyed when guest does H_REGISTER_PROC_TBL
> > > hcall.
> > 
> > I don't think that's entirely accurate.  At least in some KVM
> > configurations, we assume radix first, and only allocate the HPT once
> > the guest confirms it is doing hash.
> 
> Right, that statement is true for TCG radix guests, will rephrase
> it.

And may not remain true even for TCG radix guests.  Since for TCG mode
we don't actually need either an HPT or an RPT while the guest is in
real mode, I've suggested postponing HPT allocation to CAS time for
TCG as well; I think that will make for slightly fewer differeneces
between the KVM and TCG paths.

> > > 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.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr.c         | 15 +++++++++++++--
> > >  hw/ppc/spapr_hcall.c   |  1 +
> > >  include/hw/ppc/spapr.h |  2 ++
> > >  3 files changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 521eef1..05abfc1 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1237,6 +1237,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> > >  
> > >      /* Clean up any HPT info from a previous boot */
> > >      spapr_free_hpt(spapr);
> > > +    spapr_htab_savevm_unregister(spapr);
> > 
> > I'd prefer that the unregister be folded into spapr_free_hpt().
> > Basically we want calls that create or remove the HPT and handle
> > everything - allocation/freeing if necessary, informing KVM if
> > necessary, and registering/deregistering the savevm handlers if
> > necesary.
> > 
> > I think that will also remove the need for the trivial
> > spapr_htab_savevm_{un,}register() wrappers.
> 
> Sure, will consolidate this in the next version.
> 
> Regards,
> Bharata.
> 

-- 
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 v1 1/6] migration: Fix unregister_savevm()

[Bharata B Rao]

On Wed, May 17, 2017 at 04:43:00PM +1000, David Gibson wrote:
> On Wed, May 17, 2017 at 09:19:17AM +0530, Bharata B Rao wrote:
> > In unregister_savevm(), free se->compat only if it was allocated earlier.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> I don't think this is necessary.  If se->compat was never allocated,
> then it should be NULL (since se is allocated with g_new0()).
> g_free() is explicitly safe to call on NULL, and we already rely on
> that in qemu.

Yeah, this is not necessary, will get rid of this in the next iteration.

Regards.,
Bharata.

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

[Bharata B Rao]

On Wed, May 17, 2017 at 05:20:31PM +1000, David Gibson wrote:
> On Wed, May 17, 2017 at 12:45:39PM +0530, Bharata B Rao wrote:
> > On Wed, May 17, 2017 at 05:00:49PM +1000, David Gibson wrote:
> > > On Wed, May 17, 2017 at 09:19:22AM +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         | 15 +++++++++++++++
> > > >  hw/ppc/spapr_hcall.c   |  1 +
> > > >  include/hw/ppc/spapr.h |  1 +
> > > >  3 files changed, 17 insertions(+)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 05abfc1..dd1d687 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1443,6 +1443,20 @@ static int spapr_post_load(void *opaque, int version_id)
> > > >          err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
> > > >      }
> > > >  
> > > > +    if (spapr->patb_entry) {
> > > > +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> > > > +            err = kvmppc_configure_v3_mmu(POWERPC_CPU(first_cpu),
> > > > +                                          spapr->patb_flags &
> > > > +                                          SPAPR_PROC_TABLE_RADIX,
> > > > +                                          spapr->patb_flags &
> > > > +                                          SPAPR_PROC_TABLE_GTSE,
> > > 
> > > You should be able to work out the things you need here from
> > > patb_entry without adding the new patb_flags field.
> > 
> > kvmppc_configure_v3_mmu() needs two bools: radix and gtse. The radix
> > bit can be implied from patb_entry, I needed patb_flags to get the
> > gtse value. Not immediately obvious of how to get gtse bit from patb_entry,
> > but let me take a relook.
> 
> Oh, sorry, you can't get GTSE from the patb_entry, but you can get it
> from the LPCR.

So I need to get GTSE from LPCR at the target after the LPCR has been updated
from the incoming stream (via vmstate_ppc_cpu vmsd). However we are also
in the middle of processing the incoming stream here (via spapr_post_load).
Can we be sure that spapr_post_load() processing happens after all
SPRs (and hence LPCR) have been updated via vmstate_ppc_cpu vmsd handlers ?

Also, in addition to issuing KVM_PPC_CONFIGURE_V3_MMU, should we be
walking all the CPUs and setting the LPCR_UPRT and LPCR_GTSE bits like how
H_REGISTER_PROC_TBL does ?

Regards,
Bharata.

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

[David Gibson]

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

On Thu, May 18, 2017 at 10:33:05AM +0530, Bharata B Rao wrote:
> On Wed, May 17, 2017 at 05:20:31PM +1000, David Gibson wrote:
> > On Wed, May 17, 2017 at 12:45:39PM +0530, Bharata B Rao wrote:
> > > On Wed, May 17, 2017 at 05:00:49PM +1000, David Gibson wrote:
> > > > On Wed, May 17, 2017 at 09:19:22AM +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         | 15 +++++++++++++++
> > > > >  hw/ppc/spapr_hcall.c   |  1 +
> > > > >  include/hw/ppc/spapr.h |  1 +
> > > > >  3 files changed, 17 insertions(+)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index 05abfc1..dd1d687 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -1443,6 +1443,20 @@ static int spapr_post_load(void *opaque, int version_id)
> > > > >          err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
> > > > >      }
> > > > >  
> > > > > +    if (spapr->patb_entry) {
> > > > > +        if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> > > > > +            err = kvmppc_configure_v3_mmu(POWERPC_CPU(first_cpu),
> > > > > +                                          spapr->patb_flags &
> > > > > +                                          SPAPR_PROC_TABLE_RADIX,
> > > > > +                                          spapr->patb_flags &
> > > > > +                                          SPAPR_PROC_TABLE_GTSE,
> > > > 
> > > > You should be able to work out the things you need here from
> > > > patb_entry without adding the new patb_flags field.
> > > 
> > > kvmppc_configure_v3_mmu() needs two bools: radix and gtse. The radix
> > > bit can be implied from patb_entry, I needed patb_flags to get the
> > > gtse value. Not immediately obvious of how to get gtse bit from patb_entry,
> > > but let me take a relook.
> > 
> > Oh, sorry, you can't get GTSE from the patb_entry, but you can get it
> > from the LPCR.
> 
> So I need to get GTSE from LPCR at the target after the LPCR has been updated
> from the incoming stream (via vmstate_ppc_cpu vmsd). However we are also
> in the middle of processing the incoming stream here (via spapr_post_load).
> Can we be sure that spapr_post_load() processing happens after all
> SPRs (and hence LPCR) have been updated via vmstate_ppc_cpu vmsd handlers ?

Yes, you can.  An object's post_load is called after all that object's
state - and that of all objects below it in the QOM composition tree -
is transferred.  Since the CPUs are below the machine in the
composition tree, you can rely on the LPCR being correct in the
machine post_load handler.

> Also, in addition to issuing KVM_PPC_CONFIGURE_V3_MMU, should we be
> walking all the CPUs and setting the LPCR_UPRT and LPCR_GTSE bits like how
> H_REGISTER_PROC_TBL does ?

Shouldn't be necessary - the LPCR state should already be transferred,
along with the rest of the SPRs.

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