[Qemu-devel] [PATCH v3 0/3] ppc/spapr: Fix migration of radix guests

[Qemu-devel] [PATCH v3 0/3] ppc/spapr: Fix migration of radix guests

[Bharata B Rao]

This patchset fixes the migration of sPAPR radix guests.

Changes in v3:
--------------
- Dropped the patch that made h_register_process_table flags global as
  it is not required with the changed logic.
- The post load logic is now same for both TCG as well as KVM radix guests.
  Also ensured that radix and future v3 hash migration post load is
  treated similarly.
- Ensure TCG Radix guest migration isn't broken.

v2: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg04491.html

Bharata B Rao (3):
  migration: Introduce unregister_savevm_live()
  spapr: Unregister HPT savevm handlers for radix guests
  spapr: Fix migration of Radix guests

 hw/ppc/spapr.c              | 111 +++++++++++++++++++++++++-------------------
 include/hw/ppc/spapr.h      |   2 +
 include/migration/vmstate.h |   1 +
 migration/savevm.c          |  17 ++++++-
 4 files changed, 80 insertions(+), 51 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v3 1/3] 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>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.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] [PATCH v3 2/3] 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>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 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 be2b3b8..1b3297c 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_reallocate_hpt(sPAPRMachineState *spapr, int shift,
+                                 Error **errp);
 
 /* rtas-configure-connector state */
 struct sPAPRConfigureConnectorState {
-- 
2.7.4

[Qemu-devel] [PATCH v3 3/3] 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..ea14bed 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);
+        bool radix = !!(spapr->patb_entry & PATBE1_GR);
+        bool gtse = !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE);
+
+        err = kvmppc_configure_v3_mmu(cpu, radix, gtse, spapr->patb_entry);
+        if (err) {
+            error_report("Process table config unsupported by the host");
+            return -EINVAL;
+        }
+    }
+
     return err;
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 3/3] spapr: Fix migration of Radix guests

[Suraj Jitindar Singh]

On Wed, 2017-05-24 at 10:23 +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>

Reviewed-by: Suraj Jitindar Singh <sjitindarsingh@gmail.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..ea14bed 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);
> +        bool radix = !!(spapr->patb_entry & PATBE1_GR);
> +        bool gtse = !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE);
> +
> +        err = kvmppc_configure_v3_mmu(cpu, radix, gtse, spapr-
> >patb_entry);
> +        if (err) {
> +            error_report("Process table config unsupported by the
> host");
> +            return -EINVAL;
> +        }
> +    }
> +
>      return err;
>  }
>  

Re: [Qemu-devel] [PATCH v3 1/3] migration: Introduce unregister_savevm_live()

[Juan Quintela]

Bharata B Rao <bharata@linux.vnet.ibm.com> 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>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>

Hi

How about this one?
I just test compiled it.

Advantage from my point of view is that we always do the right thing.
And as migration code already knows if it has to be freed or not, I
think it is a better API.

What do you think?

Later, Juan.

commit 6e71fac7a9c29cef9625135c58ce59ccfbb3b86f
Author: Juan Quintela <quintela@redhat.com>
Date:   Wed May 24 08:25:06 2017 +0200

    migration: Allow to free save_live handlers
    
    Migration save_live handlers have an ops member that is not dynamic.
    Add a new member is_allocated that remembers if ops has to be freed.
    
    Signed-off-by: Juan Quintela <quintela@redhat.com>

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index f97411d..1d20e30 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -57,6 +57,8 @@ typedef struct SaveVMHandlers {
                               uint64_t *non_postcopiable_pending,
                               uint64_t *postcopiable_pending);
     LoadStateHandler *load_state;
+    /* Has been allocated by migratation code */
+    bool is_allocated;
 } SaveVMHandlers;
 
 int register_savevm(DeviceState *dev,
diff --git a/migration/savevm.c b/migration/savevm.c
index d971e5e..187f386 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -628,6 +628,7 @@ int register_savevm(DeviceState *dev,
     SaveVMHandlers *ops = g_new0(SaveVMHandlers, 1);
     ops->save_state = save_state;
     ops->load_state = load_state;
+    ops->is_allocated = true;
     return register_savevm_live(dev, idstr, instance_id, version_id,
                                 ops, opaque);
 }
@@ -651,7 +652,9 @@ 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 (se->ops->is_allocated) {
+                g_free(se->ops);
+            }
             g_free(se);
         }
     }

Re: [Qemu-devel] [PATCH v3 1/3] migration: Introduce unregister_savevm_live()

[David Gibson]

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

On Wed, May 24, 2017 at 08:28:59AM +0200, Juan Quintela wrote:
> Bharata B Rao <bharata@linux.vnet.ibm.com> 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>
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Hi
> 
> How about this one?
> I just test compiled it.
> 
> Advantage from my point of view is that we always do the right thing.
> And as migration code already knows if it has to be freed or not, I
> think it is a better API.
> 
> What do you think?

I think this is a better approach.  Do you want to push this one
directly, Juan, or do you want me to take it through my tree?

> 
> Later, Juan.
> 
> commit 6e71fac7a9c29cef9625135c58ce59ccfbb3b86f
> Author: Juan Quintela <quintela@redhat.com>
> Date:   Wed May 24 08:25:06 2017 +0200
> 
>     migration: Allow to free save_live handlers
>     
>     Migration save_live handlers have an ops member that is not dynamic.
>     Add a new member is_allocated that remembers if ops has to be freed.
>     
>     Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index f97411d..1d20e30 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -57,6 +57,8 @@ typedef struct SaveVMHandlers {
>                                uint64_t *non_postcopiable_pending,
>                                uint64_t *postcopiable_pending);
>      LoadStateHandler *load_state;
> +    /* Has been allocated by migratation code */
> +    bool is_allocated;
>  } SaveVMHandlers;
>  
>  int register_savevm(DeviceState *dev,
> diff --git a/migration/savevm.c b/migration/savevm.c
> index d971e5e..187f386 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -628,6 +628,7 @@ int register_savevm(DeviceState *dev,
>      SaveVMHandlers *ops = g_new0(SaveVMHandlers, 1);
>      ops->save_state = save_state;
>      ops->load_state = load_state;
> +    ops->is_allocated = true;
>      return register_savevm_live(dev, idstr, instance_id, version_id,
>                                  ops, opaque);
>  }
> @@ -651,7 +652,9 @@ 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 (se->ops->is_allocated) {
> +                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] [PATCH v3 1/3] migration: Introduce unregister_savevm_live()

[Juan Quintela]

David Gibson <david@gibson.dropbear.id.au> wrote:
> On Wed, May 24, 2017 at 08:28:59AM +0200, Juan Quintela wrote:
>> Bharata B Rao <bharata@linux.vnet.ibm.com> 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>
>> > Cc: Juan Quintela <quintela@redhat.com>
>> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> 
>> Hi
>> 
>> How about this one?
>> I just test compiled it.
>> 
>> Advantage from my point of view is that we always do the right thing.
>> And as migration code already knows if it has to be freed or not, I
>> think it is a better API.
>> 
>> What do you think?
>
> I think this is a better approach.  Do you want to push this one
> directly, Juan, or do you want me to take it through my tree?

I will push it on my next pull request.  I mean, I will send it for
review on own top level.

Thanks, Juan.

Re: [Qemu-devel] [PATCH v3 2/3] spapr: Unregister HPT savevm handlers for radix guests

[Bharata B Rao]

On Wed, May 24, 2017 at 10:23:33AM +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>

With Laurent's patch that switches all callers to use register_savevm_live(),
this patch will get replaced by the below updated patch which uses
unregister_savevm() instead of unregister_savevm_live().

Regards,
Bharata.

>From 4c33af53b53b14c510ab745b69dcd52dadf65d4e Mon Sep 17 00:00:00 2001
From: Bharata B Rao <bharata@linux.vnet.ibm.com>
Date: Tue, 16 May 2017 10:33:40 +0530
Subject: [PATCH v3.1 2/3] spapr: Unregister HPT savevm handlers for radix
 guests

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>
---
 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..4cc3d8c 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(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 be2b3b8..1b3297c 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_reallocate_hpt(sPAPRMachineState *spapr, int shift,
+                                 Error **errp);
 
 /* rtas-configure-connector state */
 struct sPAPRConfigureConnectorState {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 0/3] ppc/spapr: Fix migration of radix guests

[David Gibson]

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

On Wed, May 24, 2017 at 10:23:31AM +0530, Bharata B Rao wrote:
> This patchset fixes the migration of sPAPR radix guests.

Since Juan has approved merging the latest spin on the
unregister_savevm() patch through my tree, I've merged that, along
with your update 2/3 and 3/3 here.  I did rework 2/3 a bit to remove
unnecessary code motion.

> 
> Changes in v3:
> --------------
> - Dropped the patch that made h_register_process_table flags global as
>   it is not required with the changed logic.
> - The post load logic is now same for both TCG as well as KVM radix guests.
>   Also ensured that radix and future v3 hash migration post load is
>   treated similarly.
> - Ensure TCG Radix guest migration isn't broken.
> 
> v2: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg04491.html
> 
> Bharata B Rao (3):
>   migration: Introduce unregister_savevm_live()
>   spapr: Unregister HPT savevm handlers for radix guests
>   spapr: Fix migration of Radix guests
> 
>  hw/ppc/spapr.c              | 111 +++++++++++++++++++++++++-------------------
>  include/hw/ppc/spapr.h      |   2 +
>  include/migration/vmstate.h |   1 +
>  migration/savevm.c          |  17 ++++++-
>  4 files changed, 80 insertions(+), 51 deletions(-)
> 

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