On Wed, Sep 13, 2017 at 02:48:10PM +0200, Greg Kurz wrote: > On Wed, 13 Sep 2017 22:14:03 +1000 > David Gibson wrote: > > > On Wed, Sep 13, 2017 at 12:41:07PM +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 by hacking > > > the value of SDR1 and pushing it to KVM in several places. > > > > > > Also, migration breaks the guest since it is very unlikely the HPT has > > > the same address in source and destination, but we push the incoming > > > value of SDR1 to KVM anyway. > > > > > > This patch introduces a new virtual hypervisor hook so that the spapr > > > code can provide the correct value of SDR1 to be pushed to KVM each > > > time kvmppc_put_books_sregs() is called. > > > > > > It allows to get rid of all the hacking in the spapr/kvmppc code and > > > it fixes migration of nested KVM PR. > > > > > > Suggested-by: David Gibson > > > Signed-off-by: Greg Kurz > > > --- > > > > > > Unfortunately, this doesn't fix migration when using KVM PR on baremetal. > > > I get tons of instruction emulation errors in the console when the guest > > > resumes execution at the destination: > > > > > > [ 833.585957] Couldn't emulate instruction 0x00000000 (op 0 xop 0) > > > [ 833.586818] kvmppc_exit_pr_progint: emulation at 60000000600e5bfc failed > > > (00000000) > > > [ 833.674684] Couldn't emulate instruction 0x00000000 (op 0 xop 0) > > > [ 833.675370] kvmppc_exit_pr_progint: emulation at 60000000600e5bfc failed > > > (00000000) > > > [ 833.676297] Couldn't emulate instruction 0x00000000 (op 0 xop 0) > > > > > > I still don't know what's happening here but I guess this is an issue in > > > KVM. > > > > Not necessarily, those could just be because we don't have meaningful > > hash table. It's possible we're just not pushing the sregs info to > > KVM after the incoming migration. > > I'll check this out. > > > I still think whatever fix we'll > > need for that will be simpler than the earlier versions. > > > > I'll need to test this with HPT resizing (unless you already have). > > I had tested with the previous version and it worked. I'll try again with > this patch. > > > I think there is a small problem.. > > > > [snip] > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > > > index 57bb411394ed..840abff0e371 100644 > > > --- a/hw/ppc/spapr_hcall.c > > > +++ b/hw/ppc/spapr_hcall.c > > > @@ -732,14 +732,6 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu, > > > 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); > > > - } > > > - > > > > I don't think we can completely eliminate this for KVM PR. The sregs > > aren't written back to KVM on every re-entry, only rarely. So we'll > > still need to force a writeback of all the sregs stuff in order to get > > PR updated properly. > > > > Oops you're right. Since this isn't a hot path, would it be acceptable to > call kvmppc_put_books_sregs() for all CPUs instead of keeping all the sdr1 > specific hack ? Yeah, should be fine. I think the old one ended up doing that after setting spr[SDR1] anyway. > > > > 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); > > > - } > > Same here. Yes, good catch. > > > > } > > > } > > > > > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > > > index c9d3ffa89bcb..12e0b6e74876 100644 > > > --- a/target/ppc/cpu.h > > > +++ b/target/ppc/cpu.h > > > @@ -1243,6 +1243,8 @@ struct PPCVirtualHypervisorClass { > > > void (*store_hpte)(PPCVirtualHypervisor *vhyp, hwaddr ptex, > > > uint64_t pte0, uint64_t pte1); > > > uint64_t (*get_patbe)(PPCVirtualHypervisor *vhyp); > > > + void (*encode_hpt_for_kvm_pr)(PPCVirtualHypervisor *vhyp, > > > + target_ulong *hpt); > > > }; > > > > > > #define TYPE_PPC_VIRTUAL_HYPERVISOR "ppc-virtual-hypervisor" > > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > > > index 6442dfcb95b3..8edc05fc6a83 100644 > > > --- a/target/ppc/kvm.c > > > +++ b/target/ppc/kvm.c > > > @@ -941,7 +941,11 @@ int kvmppc_put_books_sregs(PowerPCCPU *cpu) > > > > > > sregs.pvr = env->spr[SPR_PVR]; > > > > > > - sregs.u.s.sdr1 = env->spr[SPR_SDR1]; > > > + if (cpu->vhyp) { > > > + PPCVirtualHypervisorClass *vhc = > > > + PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp); > > > + vhc->encode_hpt_for_kvm_pr(cpu->vhyp, &sregs.u.s.sdr1); > > > + } > > > > > > /* Sync SLB */ > > > #ifdef TARGET_PPC64 > > > @@ -2806,30 +2810,6 @@ int kvmppc_resize_hpt_commit(PowerPCCPU *cpu, target_ulong flags, int shift) > > > return kvm_vm_ioctl(cs->kvm_state, KVM_PPC_RESIZE_HPT_COMMIT, &rhpt); > > > } > > > > > > -static void kvmppc_pivot_hpt_cpu(CPUState *cs, run_on_cpu_data arg) > > > -{ > > > - target_ulong sdr1 = arg.target_ptr; > > > - PowerPCCPU *cpu = POWERPC_CPU(cs); > > > - CPUPPCState *env = &cpu->env; > > > - > > > - /* This is just for the benefit of PR KVM */ > > > - cpu_synchronize_state(cs); > > > - env->spr[SPR_SDR1] = sdr1; > > > - if (kvmppc_put_books_sregs(cpu) < 0) { > > > - error_report("Unable to update SDR1 in KVM"); > > > - exit(1); > > > - } > > > -} > > > - > > > -void kvmppc_update_sdr1(target_ulong sdr1) > > > -{ > > > - CPUState *cs; > > > - > > > - CPU_FOREACH(cs) { > > > - run_on_cpu(cs, kvmppc_pivot_hpt_cpu, RUN_ON_CPU_TARGET_PTR(sdr1)); > > > - } > > > -} > > > - > > > /* > > > * This is a helper function to detect a post migration scenario > > > * in which a guest, running as KVM-HV, freezes in cpu_post_load because > > > diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h > > > index f780e6ec7b72..21571edd7910 100644 > > > --- a/target/ppc/kvm_ppc.h > > > +++ b/target/ppc/kvm_ppc.h > > > @@ -68,7 +68,6 @@ PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void); > > > void kvmppc_check_papr_resize_hpt(Error **errp); > > > int kvmppc_resize_hpt_prepare(PowerPCCPU *cpu, target_ulong flags, int shift); > > > int kvmppc_resize_hpt_commit(PowerPCCPU *cpu, target_ulong flags, int shift); > > > -void kvmppc_update_sdr1(target_ulong sdr1); > > > bool kvmppc_pvr_workaround_required(PowerPCCPU *cpu); > > > > > > bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path); > > > @@ -331,11 +330,6 @@ static inline int kvmppc_resize_hpt_commit(PowerPCCPU *cpu, > > > return -ENOSYS; > > > } > > > > > > -static inline void kvmppc_update_sdr1(target_ulong sdr1) > > > -{ > > > - abort(); > > > -} > > > - > > > #endif > > > > > > #ifndef CONFIG_KVM > > > > > > -- 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