[Qemu-devel] [PATCH v2 0/2] spapr: fix migration with nested KVM PR

[Qemu-devel] [PATCH v2 0/2] spapr: fix migration with nested KVM PR

[Greg Kurz]

A guest running with KVM PR ends up irresponsive after migration most of the
time. This happens because the HPT allocated by QEMU is likely to have a
different address on the destination than it had on the source, but we push
the source address to KVM.

This v2 is a total rewrite.

Cheers,

--
Greg

---

Greg Kurz (2):
      spapr: introduce common helper to write HPT address to KVM PR
      spapr: preserve SDR1 during migration


 hw/ppc/spapr.c          |   61 +++++++++++++++++++++++++++++++++++++++++++----
 hw/ppc/spapr_cpu_core.c |   15 ------------
 hw/ppc/spapr_hcall.c    |   16 +-----------
 include/hw/ppc/spapr.h  |    1 +
 target/ppc/cpu.h        |    5 ++++
 target/ppc/machine.c    |   18 ++++++++++++++
 6 files changed, 81 insertions(+), 35 deletions(-)

[Qemu-devel] [PATCH v2 1/2] spapr: introduce common helper to write HPT address to KVM PR

[Greg Kurz]

When running with KVM PR, if a new HPT is allocated we need to inform
KVM about the HPT address and size. This is currently done with a hack
which is open-coded in several places.

This patch consolidate the code in a dedicated helper that records
the HPT address and size in the sPAPR context, and then does the
magic for KVM PR.

Note that ppc_spapr_reset() now resets all devices and CPUs before
allocating the HPT. This allows to drop the hack from spapr_cpu_reset().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c          |   31 ++++++++++++++++++++++++++-----
 hw/ppc/spapr_cpu_core.c |   15 ---------------
 hw/ppc/spapr_hcall.c    |   16 +---------------
 include/hw/ppc/spapr.h  |    1 +
 4 files changed, 28 insertions(+), 35 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f680f28a15ea..97f8afdbd7fe 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1309,6 +1309,25 @@ void spapr_free_hpt(sPAPRMachineState *spapr)
     close_htab_fd(spapr);
 }
 
+void spapr_install_hpt(sPAPRMachineState *spapr, void *htab, uint32_t shift)
+{
+    assert(htab);
+
+    spapr->htab = htab;
+    spapr->htab_shift = shift;
+
+    /*
+     * This is a hack for the benefit of KVM PR - it abuses the SDR1
+     * slot in kvm_sregs to communicate the userspace address of the
+     * HPT
+     */
+    if (kvm_enabled()) {
+        target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
+            | (spapr->htab_shift - 18);
+        kvmppc_update_sdr1(sdr1);
+    }
+}
+
 void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
                           Error **errp)
 {
@@ -1339,16 +1358,17 @@ void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
         /* kernel-side HPT not needed, allocate in userspace instead */
         size_t size = 1ULL << shift;
         int i;
+        void *htab;
 
-        spapr->htab = qemu_memalign(size, size);
-        if (!spapr->htab) {
+        htab = qemu_memalign(size, size);
+        if (!htab) {
             error_setg_errno(errp, errno,
                              "Could not allocate HPT of order %d", shift);
             return;
         }
 
-        memset(spapr->htab, 0, size);
-        spapr->htab_shift = shift;
+        memset(htab, 0, size);
+        spapr_install_hpt(spapr, htab, shift);
 
         for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
             DIRTY_HPTE(HPTE(spapr->htab, i));
@@ -1405,6 +1425,8 @@ static void ppc_spapr_reset(void)
     /* Check for unknown sysbus devices */
     foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
 
+    qemu_devices_reset();
+
     if (kvm_enabled() && kvmppc_has_cap_mmu_radix()) {
         /* If using KVM with radix mode available, VCPUs can be started
          * without a HPT because KVM will start them in radix mode.
@@ -1414,7 +1436,6 @@ static void ppc_spapr_reset(void)
         spapr_setup_hpt_and_vrma(spapr);
     }
 
-    qemu_devices_reset();
     spapr_clear_pending_events(spapr);
 
     /*
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index c08ee7571a50..c20b5c64b045 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -73,7 +73,6 @@ void spapr_cpu_parse_features(sPAPRMachineState *spapr)
 
 static void spapr_cpu_reset(void *opaque)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     PowerPCCPU *cpu = opaque;
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
@@ -86,20 +85,6 @@ static void spapr_cpu_reset(void *opaque)
     cs->halted = 1;
 
     env->spr[SPR_HIOR] = 0;
-
-    /*
-     * This is a hack for the benefit of KVM PR - it abuses the SDR1
-     * slot in kvm_sregs to communicate the userspace address of the
-     * HPT
-     */
-    if (kvm_enabled()) {
-        env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab
-            | (spapr->htab_shift - 18);
-        if (kvmppc_put_books_sregs(cpu) < 0) {
-            error_report("Unable to update SDR1 in KVM");
-            exit(1);
-        }
-    }
 }
 
 static void spapr_cpu_destroy(PowerPCCPU *cpu)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 57bb411394ed..7892cd3e7ffa 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -730,15 +730,7 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu,
                     pending->hpt, newsize);
     if (rc == H_SUCCESS) {
         qemu_vfree(spapr->htab);
-        spapr->htab = pending->hpt;
-        spapr->htab_shift = pending->shift;
-
-        if (kvm_enabled()) {
-            /* For KVM PR, update the HPT pointer */
-            target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
-                | (spapr->htab_shift - 18);
-            kvmppc_update_sdr1(sdr1);
-        }
+        spapr_install_hpt(spapr, pending->hpt, pending->shift);
 
         pending->hpt = NULL; /* so it's not free()d */
     }
@@ -1564,12 +1556,6 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
              * the point this is called, nothing should have been
              * entered into the existing HPT */
             spapr_reallocate_hpt(spapr, maxshift, &error_fatal);
-            if (kvm_enabled()) {
-                /* For KVM PR, update the HPT pointer */
-                target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
-                    | (spapr->htab_shift - 18);
-                kvmppc_update_sdr1(sdr1);
-            }
         }
     }
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index c1b365f56431..30e5805acca4 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -709,4 +709,5 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
 int spapr_vcpu_id(PowerPCCPU *cpu);
 PowerPCCPU *spapr_find_cpu(int vcpu_id);
 
+void spapr_install_hpt(sPAPRMachineState *spapr, void *htab, uint32_t shift);
 #endif /* HW_SPAPR_H */

[Qemu-devel] [PATCH v2 2/2] spapr: preserve SDR1 during migration

[Greg Kurz]

When running with KVM PR, a pseries machine needs to allocate an HPT
in userspace and pass its address and size too KVM. This is done at
machine reset time by hijacking SDR1.

It is very likely that the destination QEMU will allocate the HPT at
a different address, ie, the SDR1 value we get from the migration
stream is wrong and the guest ends up badly broken.

Let's fix this by preserving the pre-load value of SDR1. Since this is
a spapr specific hack, this is achieved by extending the PPC virtual
hypervisor interface.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c       |   30 ++++++++++++++++++++++++++++++
 target/ppc/cpu.h     |    5 +++++
 target/ppc/machine.c |   18 ++++++++++++++++++
 3 files changed, 53 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 97f8afdbd7fe..aa280c9d767f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1200,6 +1200,34 @@ static uint64_t spapr_get_patbe(PPCVirtualHypervisor *vhyp)
     return spapr->patb_entry;
 }
 
+static void spapr_cpu_pre_load(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(vhyp);
+
+    /* This is a hack for KVM PR: SDR1 contains the address and size of the HPT
+     * allocated by QEMU. We must preserve it.
+     */
+    if (kvm_enabled() && spapr->htab) {
+        CPUPPCState *env = &cpu->env;
+
+        cpu->sdr1_kvm_pr = env->spr[SPR_SDR1];
+    }
+}
+
+static void spapr_cpu_post_load(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(vhyp);
+
+    /* This is a hack for KVM PR: SDR1 contains the address and size of the HPT
+     * allocated by QEMU. We must preserve it.
+     */
+    if (kvm_enabled() && spapr->htab) {
+        CPUPPCState *env = &cpu->env;
+
+        env->spr[SPR_SDR1] = cpu->sdr1_kvm_pr;
+    }
+}
+
 #define HPTE(_table, _i)   (void *)(((uint64_t *)(_table)) + ((_i) * 2))
 #define HPTE_VALID(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
 #define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
@@ -3624,6 +3652,8 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     vhc->unmap_hptes = spapr_unmap_hptes;
     vhc->store_hpte = spapr_store_hpte;
     vhc->get_patbe = spapr_get_patbe;
+    vhc->cpu_pre_load = spapr_cpu_pre_load;
+    vhc->cpu_post_load = spapr_cpu_post_load;
     xic->ics_get = spapr_ics_get;
     xic->ics_resend = spapr_ics_resend;
     xic->icp_get = spapr_icp_get;
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index c9d3ffa89bcb..22ea2538d923 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1212,6 +1212,9 @@ struct PowerPCCPU {
     uint64_t mig_insns_flags2;
     uint32_t mig_nb_BATs;
     bool pre_2_10_migration;
+
+    /* KVM PR hack to preserve SDR1 (HPT) */
+    target_ulong sdr1_kvm_pr;
 };
 
 static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
@@ -1243,6 +1246,8 @@ struct PPCVirtualHypervisorClass {
     void (*store_hpte)(PPCVirtualHypervisor *vhyp, hwaddr ptex,
                        uint64_t pte0, uint64_t pte1);
     uint64_t (*get_patbe)(PPCVirtualHypervisor *vhyp);
+    void (*cpu_pre_load)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu);
+    void (*cpu_post_load)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu);
 };
 
 #define TYPE_PPC_VIRTUAL_HYPERVISOR "ppc-virtual-hypervisor"
diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index e36b7100cb66..c4d32c886367 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -218,6 +218,19 @@ static bool pvr_match(PowerPCCPU *cpu, uint32_t pvr)
     return pcc->pvr_match(pcc, pvr);
 }
 
+static int cpu_pre_load(void *opaque)
+{
+    PowerPCCPU *cpu = opaque;
+
+    if (cpu->vhyp) {
+        PPCVirtualHypervisorClass *vhc =
+            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
+        vhc->cpu_pre_load(cpu->vhyp, cpu);
+    }
+
+    return 0;
+}
+
 static int cpu_post_load(void *opaque, int version_id)
 {
     PowerPCCPU *cpu = opaque;
@@ -294,6 +307,10 @@ static int cpu_post_load(void *opaque, int version_id)
 
     if (!cpu->vhyp) {
         ppc_store_sdr1(env, env->spr[SPR_SDR1]);
+    } else {
+        PPCVirtualHypervisorClass *vhc =
+            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
+        vhc->cpu_post_load(cpu->vhyp, cpu);
     }
 
     /* Invalidate all msr bits except MSR_TGPR/MSR_HVB before restoring */
@@ -650,6 +667,7 @@ const VMStateDescription vmstate_ppc_cpu = {
     .minimum_version_id_old = 4,
     .load_state_old = cpu_load_old,
     .pre_save = cpu_pre_save,
+    .pre_load = cpu_pre_load,
     .post_load = cpu_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UNUSED(sizeof(target_ulong)), /* was _EQUAL(env.spr[SPR_PVR]) */

Re: [Qemu-devel] [PATCH v2 1/2] spapr: introduce common helper to write HPT address to KVM PR

[David Gibson]

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

On Wed, Sep 13, 2017 at 12:24:53AM +0200, Greg Kurz wrote:
> When running with KVM PR, if a new HPT is allocated we need to inform
> KVM about the HPT address and size. This is currently done with a hack
> which is open-coded in several places.
> 
> This patch consolidate the code in a dedicated helper that records
> the HPT address and size in the sPAPR context, and then does the
> magic for KVM PR.
> 
> Note that ppc_spapr_reset() now resets all devices and CPUs before
> allocating the HPT. This allows to drop the hack from spapr_cpu_reset().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

I like this more than the previous spin, but while discussing stuff
with SamB, I thought up a different approach, which I think will be
both cleaner and simpler.

It basically doesn't make sense to put the userspace HPT pointer into
env->spr[SDR1], we only do it to make kvmppc_put_books_sregs() do the
right thing.

Instead, we can have kvmppc_put_books_sregs() populate the "SDR1"
field in kvm_sregs from a vhyp hook.  We already have the reverse side
in that kvmppc_get_books_sregs() doesn't update the internal SDR1
value if vhyp is set.

In any case the spapr hook would compute the correct value direct from
spapr->htab.

After incoming migration I'm not sure we need to do anything - I think
we already do a pretty thorough register resync with KVM.

> ---
>  hw/ppc/spapr.c          |   31 ++++++++++++++++++++++++++-----
>  hw/ppc/spapr_cpu_core.c |   15 ---------------
>  hw/ppc/spapr_hcall.c    |   16 +---------------
>  include/hw/ppc/spapr.h  |    1 +
>  4 files changed, 28 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f680f28a15ea..97f8afdbd7fe 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1309,6 +1309,25 @@ void spapr_free_hpt(sPAPRMachineState *spapr)
>      close_htab_fd(spapr);
>  }
>  
> +void spapr_install_hpt(sPAPRMachineState *spapr, void *htab, uint32_t shift)
> +{
> +    assert(htab);
> +
> +    spapr->htab = htab;
> +    spapr->htab_shift = shift;
> +
> +    /*
> +     * This is a hack for the benefit of KVM PR - it abuses the SDR1
> +     * slot in kvm_sregs to communicate the userspace address of the
> +     * HPT
> +     */
> +    if (kvm_enabled()) {
> +        target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
> +            | (spapr->htab_shift - 18);
> +        kvmppc_update_sdr1(sdr1);
> +    }
> +}
> +
>  void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
>                            Error **errp)
>  {
> @@ -1339,16 +1358,17 @@ void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
>          /* kernel-side HPT not needed, allocate in userspace instead */
>          size_t size = 1ULL << shift;
>          int i;
> +        void *htab;
>  
> -        spapr->htab = qemu_memalign(size, size);
> -        if (!spapr->htab) {
> +        htab = qemu_memalign(size, size);
> +        if (!htab) {
>              error_setg_errno(errp, errno,
>                               "Could not allocate HPT of order %d", shift);
>              return;
>          }
>  
> -        memset(spapr->htab, 0, size);
> -        spapr->htab_shift = shift;
> +        memset(htab, 0, size);
> +        spapr_install_hpt(spapr, htab, shift);
>  
>          for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
>              DIRTY_HPTE(HPTE(spapr->htab, i));
> @@ -1405,6 +1425,8 @@ static void ppc_spapr_reset(void)
>      /* Check for unknown sysbus devices */
>      foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
>  
> +    qemu_devices_reset();
> +
>      if (kvm_enabled() && kvmppc_has_cap_mmu_radix()) {
>          /* If using KVM with radix mode available, VCPUs can be started
>           * without a HPT because KVM will start them in radix mode.
> @@ -1414,7 +1436,6 @@ static void ppc_spapr_reset(void)
>          spapr_setup_hpt_and_vrma(spapr);
>      }
>  
> -    qemu_devices_reset();
>      spapr_clear_pending_events(spapr);
>  
>      /*
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index c08ee7571a50..c20b5c64b045 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -73,7 +73,6 @@ void spapr_cpu_parse_features(sPAPRMachineState *spapr)
>  
>  static void spapr_cpu_reset(void *opaque)
>  {
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      PowerPCCPU *cpu = opaque;
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
> @@ -86,20 +85,6 @@ static void spapr_cpu_reset(void *opaque)
>      cs->halted = 1;
>  
>      env->spr[SPR_HIOR] = 0;
> -
> -    /*
> -     * This is a hack for the benefit of KVM PR - it abuses the SDR1
> -     * slot in kvm_sregs to communicate the userspace address of the
> -     * HPT
> -     */
> -    if (kvm_enabled()) {
> -        env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab
> -            | (spapr->htab_shift - 18);
> -        if (kvmppc_put_books_sregs(cpu) < 0) {
> -            error_report("Unable to update SDR1 in KVM");
> -            exit(1);
> -        }
> -    }
>  }
>  
>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 57bb411394ed..7892cd3e7ffa 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -730,15 +730,7 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu,
>                      pending->hpt, newsize);
>      if (rc == H_SUCCESS) {
>          qemu_vfree(spapr->htab);
> -        spapr->htab = pending->hpt;
> -        spapr->htab_shift = pending->shift;
> -
> -        if (kvm_enabled()) {
> -            /* For KVM PR, update the HPT pointer */
> -            target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
> -                | (spapr->htab_shift - 18);
> -            kvmppc_update_sdr1(sdr1);
> -        }
> +        spapr_install_hpt(spapr, pending->hpt, pending->shift);
>  
>          pending->hpt = NULL; /* so it's not free()d */
>      }
> @@ -1564,12 +1556,6 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>               * the point this is called, nothing should have been
>               * entered into the existing HPT */
>              spapr_reallocate_hpt(spapr, maxshift, &error_fatal);
> -            if (kvm_enabled()) {
> -                /* For KVM PR, update the HPT pointer */
> -                target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
> -                    | (spapr->htab_shift - 18);
> -                kvmppc_update_sdr1(sdr1);
> -            }
>          }
>      }
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index c1b365f56431..30e5805acca4 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -709,4 +709,5 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
>  int spapr_vcpu_id(PowerPCCPU *cpu);
>  PowerPCCPU *spapr_find_cpu(int vcpu_id);
>  
> +void spapr_install_hpt(sPAPRMachineState *spapr, void *htab, uint32_t shift);
>  #endif /* HW_SPAPR_H */
> 

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