| Message ID | 150456162500.17000.8195671755736683016.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | ppc: fix migration with KVM PR (nested) | expand |
On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote: > The formula used to compute the address of the HPT allocated by QEMU is > open-coded in several places. This patch moves the magic to a dedicated > helper. While here, we also patch the callers to only pass the address > to KVM if we indeed have a userland HPT (ie, KVM PR). The helper function seems reasonable, though I'm not sure about the name (a. it's not just a pointer, since it includes the encoded size and b. the name doesn't indicate this is basically KVM PR specific). THe "only pass the address to KVM if we indeed have a userland HPT (ie, KVM PR)" bit doesn't really work. You're doing it by testing for (sdr1 != 0), but that can only be true if the HPT is minimum size, which doesn't have much to do with anything meaningful. > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/ppc/spapr.c | 9 +++++++++ > hw/ppc/spapr_cpu_core.c | 12 +++++++----- > hw/ppc/spapr_hcall.c | 14 ++++++++------ > include/hw/ppc/spapr.h | 1 + > 4 files changed, 25 insertions(+), 11 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index caffa1276328..bf24c26b756d 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1290,6 +1290,15 @@ static void spapr_store_hpte(PPCVirtualHypervisor *vhyp, hwaddr ptex, > } > } > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr) > +{ > + if (!spapr->htab) { > + return 0; > + } > + > + return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18); > +} > + > int spapr_hpt_shift_for_ramsize(uint64_t ramsize) > { > int shift; > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index 85037ef71e27..581eb4d92de9 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -93,11 +93,13 @@ static void spapr_cpu_reset(void *opaque) > * 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); > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr); > + if (sdr1) { > + env->spr[SPR_SDR1] = sdr1; > + if (kvmppc_put_books_sregs(cpu) < 0) { > + error_report("Unable to update SDR1 in KVM"); > + exit(1); > + } > } > } > } > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index 6ab8c188f381..06059b44ab40 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -735,9 +735,10 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu, > > 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); > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr); > + if (sdr1) { > + kvmppc_update_sdr1(sdr1); > + } > } > > pending->hpt = NULL; /* so it's not free()d */ > @@ -1566,9 +1567,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > 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); > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr); > + if (sdr1) { > + kvmppc_update_sdr1(sdr1); > + } > } > } > } > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index c1b365f56431..a1f5edc15018 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); > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr); > #endif /* HW_SPAPR_H */ >
On Sun, 10 Sep 2017 13:41:41 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote: > > The formula used to compute the address of the HPT allocated by QEMU is > > open-coded in several places. This patch moves the magic to a dedicated > > helper. While here, we also patch the callers to only pass the address > > to KVM if we indeed have a userland HPT (ie, KVM PR). > > The helper function seems reasonable, though I'm not sure about the > name (a. it's not just a pointer, since it includes the encoded size > and b. the name doesn't indicate this is basically KVM PR specific). > Sure, I'll come up with a better name. > THe "only pass the address to KVM if we indeed have a userland HPT > (ie, KVM PR)" bit doesn't really work. You're doing it by testing for > (sdr1 != 0), but that can only be true if the HPT is minimum size, > which doesn't have much to do with anything meaningful. > Hmmm... if QEMU has allocated an HPT in userspace then the helper necessarily returns a non-null value, no matter the HPT size. Am I missing something ? > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/ppc/spapr.c | 9 +++++++++ > > hw/ppc/spapr_cpu_core.c | 12 +++++++----- > > hw/ppc/spapr_hcall.c | 14 ++++++++------ > > include/hw/ppc/spapr.h | 1 + > > 4 files changed, 25 insertions(+), 11 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index caffa1276328..bf24c26b756d 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1290,6 +1290,15 @@ static void spapr_store_hpte(PPCVirtualHypervisor *vhyp, hwaddr ptex, > > } > > } > > > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr) > > +{ > > + if (!spapr->htab) { > > + return 0; > > + } > > + > > + return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18); > > +} > > + > > int spapr_hpt_shift_for_ramsize(uint64_t ramsize) > > { > > int shift; > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index 85037ef71e27..581eb4d92de9 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -93,11 +93,13 @@ static void spapr_cpu_reset(void *opaque) > > * 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); > > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr); > > + if (sdr1) { > > + env->spr[SPR_SDR1] = sdr1; > > + if (kvmppc_put_books_sregs(cpu) < 0) { > > + error_report("Unable to update SDR1 in KVM"); > > + exit(1); > > + } > > } > > } > > } > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > > index 6ab8c188f381..06059b44ab40 100644 > > --- a/hw/ppc/spapr_hcall.c > > +++ b/hw/ppc/spapr_hcall.c > > @@ -735,9 +735,10 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu, > > > > 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); > > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr); > > + if (sdr1) { > > + kvmppc_update_sdr1(sdr1); > > + } > > } > > > > pending->hpt = NULL; /* so it's not free()d */ > > @@ -1566,9 +1567,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > > 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); > > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr); > > + if (sdr1) { > > + kvmppc_update_sdr1(sdr1); > > + } > > } > > } > > } > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > index c1b365f56431..a1f5edc15018 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); > > > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr); > > #endif /* HW_SPAPR_H */ > > >
On Mon, Sep 11, 2017 at 02:04:37PM +0200, Greg Kurz wrote: > On Sun, 10 Sep 2017 13:41:41 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote: > > > The formula used to compute the address of the HPT allocated by QEMU is > > > open-coded in several places. This patch moves the magic to a dedicated > > > helper. While here, we also patch the callers to only pass the address > > > to KVM if we indeed have a userland HPT (ie, KVM PR). > > > > The helper function seems reasonable, though I'm not sure about the > > name (a. it's not just a pointer, since it includes the encoded size > > and b. the name doesn't indicate this is basically KVM PR specific). > > > > Sure, I'll come up with a better name. > > > THe "only pass the address to KVM if we indeed have a userland HPT > > (ie, KVM PR)" bit doesn't really work. You're doing it by testing for > > (sdr1 != 0), but that can only be true if the HPT is minimum size, > > which doesn't have much to do with anything meaningful. > > > > Hmmm... if QEMU has allocated an HPT in userspace then the helper > necessarily returns a non-null value, no matter the HPT size. Am > I missing something ? Yes, but the reverse is not true. Even if qemu *hasn't* allocated an HPT in userspace, it will usually return a non-zero value - the only case it won't is when the HPT is minimum size. That makes the test pretty pointless. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > hw/ppc/spapr.c | 9 +++++++++ > > > hw/ppc/spapr_cpu_core.c | 12 +++++++----- > > > hw/ppc/spapr_hcall.c | 14 ++++++++------ > > > include/hw/ppc/spapr.h | 1 + > > > 4 files changed, 25 insertions(+), 11 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index caffa1276328..bf24c26b756d 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -1290,6 +1290,15 @@ static void spapr_store_hpte(PPCVirtualHypervisor *vhyp, hwaddr ptex, > > > } > > > } > > > > > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr) > > > +{ > > > + if (!spapr->htab) { > > > + return 0; > > > + } > > > + > > > + return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18); > > > +} > > > + > > > int spapr_hpt_shift_for_ramsize(uint64_t ramsize) > > > { > > > int shift; > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > index 85037ef71e27..581eb4d92de9 100644 > > > --- a/hw/ppc/spapr_cpu_core.c > > > +++ b/hw/ppc/spapr_cpu_core.c > > > @@ -93,11 +93,13 @@ static void spapr_cpu_reset(void *opaque) > > > * 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); > > > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr); > > > + if (sdr1) { > > > + env->spr[SPR_SDR1] = sdr1; > > > + if (kvmppc_put_books_sregs(cpu) < 0) { > > > + error_report("Unable to update SDR1 in KVM"); > > > + exit(1); > > > + } > > > } > > > } > > > } > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > > > index 6ab8c188f381..06059b44ab40 100644 > > > --- a/hw/ppc/spapr_hcall.c > > > +++ b/hw/ppc/spapr_hcall.c > > > @@ -735,9 +735,10 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu, > > > > > > 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); > > > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr); > > > + if (sdr1) { > > > + kvmppc_update_sdr1(sdr1); > > > + } > > > } > > > > > > pending->hpt = NULL; /* so it's not free()d */ > > > @@ -1566,9 +1567,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > > > 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); > > > + target_ulong sdr1 = spapr_get_hpt_pointer(spapr); > > > + if (sdr1) { > > > + kvmppc_update_sdr1(sdr1); > > > + } > > > } > > > } > > > } > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > > index c1b365f56431..a1f5edc15018 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); > > > > > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr); > > > #endif /* HW_SPAPR_H */ > > > > > >
On Mon, 11 Sep 2017 22:50:45 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Sep 11, 2017 at 02:04:37PM +0200, Greg Kurz wrote: > > On Sun, 10 Sep 2017 13:41:41 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote: > > > > The formula used to compute the address of the HPT allocated by QEMU is > > > > open-coded in several places. This patch moves the magic to a dedicated > > > > helper. While here, we also patch the callers to only pass the address > > > > to KVM if we indeed have a userland HPT (ie, KVM PR). > > > > > > The helper function seems reasonable, though I'm not sure about the > > > name (a. it's not just a pointer, since it includes the encoded size > > > and b. the name doesn't indicate this is basically KVM PR specific). > > > > > > > Sure, I'll come up with a better name. > > > > > THe "only pass the address to KVM if we indeed have a userland HPT > > > (ie, KVM PR)" bit doesn't really work. You're doing it by testing for > > > (sdr1 != 0), but that can only be true if the HPT is minimum size, > > > which doesn't have much to do with anything meaningful. > > > > > > > Hmmm... if QEMU has allocated an HPT in userspace then the helper > > necessarily returns a non-null value, no matter the HPT size. Am > > I missing something ? > > Yes, but the reverse is not true. Even if qemu *hasn't* allocated an > HPT in userspace, it will usually return a non-zero value - the only > case it won't is when the HPT is minimum size. That makes the test > pretty pointless. > The helper does return 0 if QEMU hasn't allocated an HPT in userspace. [...] > > > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr) > > > > +{ > > > > + if (!spapr->htab) { > > > > + return 0; > > > > + } > > > > + > > > > + return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18); > > > > +} > > > > + but I agree the patch isn't good... for the comprehension at least. I'll rework the patchset. Also this causes a behavior change: we won't update SDR1 anymore with KVM HV, which already handles it BTW. void kvmppc_set_hpt(struct kvm *kvm, struct kvm_hpt_info *info) { atomic64_set(&kvm->arch.mmio_update, 0); kvm->arch.hpt = *info; kvm->arch.sdr1 = __pa(info->virt) | (info->order - 18); Maybe I should push this behavior change to a separate patch anyway...
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index caffa1276328..bf24c26b756d 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1290,6 +1290,15 @@ static void spapr_store_hpte(PPCVirtualHypervisor *vhyp, hwaddr ptex, } } +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr) +{ + if (!spapr->htab) { + return 0; + } + + return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18); +} + int spapr_hpt_shift_for_ramsize(uint64_t ramsize) { int shift; diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c index 85037ef71e27..581eb4d92de9 100644 --- a/hw/ppc/spapr_cpu_core.c +++ b/hw/ppc/spapr_cpu_core.c @@ -93,11 +93,13 @@ static void spapr_cpu_reset(void *opaque) * 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); + target_ulong sdr1 = spapr_get_hpt_pointer(spapr); + if (sdr1) { + env->spr[SPR_SDR1] = sdr1; + if (kvmppc_put_books_sregs(cpu) < 0) { + error_report("Unable to update SDR1 in KVM"); + exit(1); + } } } } diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index 6ab8c188f381..06059b44ab40 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -735,9 +735,10 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu, 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); + target_ulong sdr1 = spapr_get_hpt_pointer(spapr); + if (sdr1) { + kvmppc_update_sdr1(sdr1); + } } pending->hpt = NULL; /* so it's not free()d */ @@ -1566,9 +1567,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, 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); + target_ulong sdr1 = spapr_get_hpt_pointer(spapr); + if (sdr1) { + kvmppc_update_sdr1(sdr1); + } } } } diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index c1b365f56431..a1f5edc15018 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); +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr); #endif /* HW_SPAPR_H */
The formula used to compute the address of the HPT allocated by QEMU is open-coded in several places. This patch moves the magic to a dedicated helper. While here, we also patch the callers to only pass the address to KVM if we indeed have a userland HPT (ie, KVM PR). Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr.c | 9 +++++++++ hw/ppc/spapr_cpu_core.c | 12 +++++++----- hw/ppc/spapr_hcall.c | 14 ++++++++------ include/hw/ppc/spapr.h | 1 + 4 files changed, 25 insertions(+), 11 deletions(-)