* [Qemu-devel] [PATCH v2 0/2] spapr: fix migration with nested KVM PR
@ 2017-09-12 22:24 Greg Kurz
2017-09-12 22:24 ` [Qemu-devel] [PATCH v2 1/2] spapr: introduce common helper to write HPT address to " Greg Kurz
2017-09-12 22:25 ` [Qemu-devel] [PATCH v2 2/2] spapr: preserve SDR1 during migration Greg Kurz
0 siblings, 2 replies; 4+ messages in thread
From: Greg Kurz @ 2017-09-12 22:24 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-ppc, David Gibson
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(-)
^ permalink raw reply [flat|nested] 4+ messages in thread* [Qemu-devel] [PATCH v2 1/2] spapr: introduce common helper to write HPT address to KVM PR 2017-09-12 22:24 [Qemu-devel] [PATCH v2 0/2] spapr: fix migration with nested KVM PR Greg Kurz @ 2017-09-12 22:24 ` Greg Kurz 2017-09-13 4:40 ` David Gibson 2017-09-12 22:25 ` [Qemu-devel] [PATCH v2 2/2] spapr: preserve SDR1 during migration Greg Kurz 1 sibling, 1 reply; 4+ messages in thread From: Greg Kurz @ 2017-09-12 22:24 UTC (permalink / raw) To: qemu-devel; +Cc: qemu-ppc, David Gibson 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 */ ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] spapr: introduce common helper to write HPT address to KVM PR 2017-09-12 22:24 ` [Qemu-devel] [PATCH v2 1/2] spapr: introduce common helper to write HPT address to " Greg Kurz @ 2017-09-13 4:40 ` David Gibson 0 siblings, 0 replies; 4+ messages in thread From: David Gibson @ 2017-09-13 4:40 UTC (permalink / raw) To: Greg Kurz; +Cc: qemu-devel, qemu-ppc [-- 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 --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] spapr: preserve SDR1 during migration 2017-09-12 22:24 [Qemu-devel] [PATCH v2 0/2] spapr: fix migration with nested KVM PR Greg Kurz 2017-09-12 22:24 ` [Qemu-devel] [PATCH v2 1/2] spapr: introduce common helper to write HPT address to " Greg Kurz @ 2017-09-12 22:25 ` Greg Kurz 1 sibling, 0 replies; 4+ messages in thread From: Greg Kurz @ 2017-09-12 22:25 UTC (permalink / raw) To: qemu-devel; +Cc: qemu-ppc, David Gibson 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]) */ ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-09-13 5:35 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-09-12 22:24 [Qemu-devel] [PATCH v2 0/2] spapr: fix migration with nested KVM PR Greg Kurz 2017-09-12 22:24 ` [Qemu-devel] [PATCH v2 1/2] spapr: introduce common helper to write HPT address to " Greg Kurz 2017-09-13 4:40 ` David Gibson 2017-09-12 22:25 ` [Qemu-devel] [PATCH v2 2/2] spapr: preserve SDR1 during migration Greg Kurz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).