On Mon, Sep 11, 2017 at 02:04:37PM +0200, Greg Kurz wrote: > On Sun, 10 Sep 2017 13:41:41 +1000 > David Gibson <da...@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 <gr...@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 */
> > >
> >
>
--
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
signature.asc
Description: PGP signature
