Re: [Qemu-ppc] [PATCH v3 07/12] spapr: Only setup HTP if necessary.

[Sam Bobroff]

On Fri, Mar 03, 2017 at 02:39:47PM +1100, David Gibson wrote:
> On Thu, Mar 02, 2017 at 04:39:02PM +1100, Sam Bobroff wrote:
> > If QEMU is using KVM, and KVM is capable of running in radix mode,
> > guests can be run in real-mode without allocating a HPT (because KVM
> > will use a minimal RPT). So in this case, we avoid creating the HPT
> > at reset time and later (during CAS) create it if it is necessary.
> 
> Your subject line still has a typo.
> 
> > Signed-off-by: Sam Bobroff <address@hidden>
> > ---
> > Changes in v3:
> > * in spapr_setup_hpt_and_vrma() replaced MACHINE(qdev_get_machine()) with 
> > MACHINE(spapr).
> > 
> >  hw/ppc/spapr.c         | 24 +++++++++++++++---------
> >  hw/ppc/spapr_hcall.c   | 10 ++++++++++
> >  include/hw/ppc/spapr.h |  1 +
> >  3 files changed, 26 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index d154fa2112..0b266e96f1 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1207,6 +1207,17 @@ static void spapr_reallocate_hpt(sPAPRMachineState 
> > *spapr, int shift,
> >      }
> >  }
> >  
> > +void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
> > +{
> > +    spapr_reallocate_hpt(spapr,
> > +                     
> > spapr_hpt_shift_for_ramsize(MACHINE(spapr)->maxram_size),
> > +                     &error_fatal);
> > +    if (spapr->vrma_adjust) {
> > +        spapr->rma_size = kvmppc_rma_size(spapr_node0_size(),
> > +                                          spapr->htab_shift);
> > +    }
> > +}
> > +
> >  static void find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
> >  {
> >      bool matched = false;
> > @@ -1237,15 +1248,10 @@ static void ppc_spapr_reset(void)
> >  
> >      spapr->patb_entry = 0;
> >  
> > -    /* Allocate and/or reset the hash page table */
> > -    spapr_reallocate_hpt(spapr,
> > -                         spapr_hpt_shift_for_ramsize(machine->maxram_size),
> > -                         &error_fatal);
> > -
> > -    /* Update the RMA size if necessary */
> > -    if (spapr->vrma_adjust) {
> > -        spapr->rma_size = kvmppc_rma_size(spapr_node0_size(),
> > -                                          spapr->htab_shift);
> > +    /* If using KVM with radix mode available, VCPUs can be started
> > +     * without a HPT because KVM will start them in radix mode. */
> > +    if (!(kvm_enabled() && kvmppc_has_cap_mmu_radix())) {
> > +        spapr_setup_hpt_and_vrma(spapr);
> >      }
> >  
> >      qemu_devices_reset();
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index f05a90ed2c..0c1b0326ad 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -999,6 +999,16 @@ static target_ulong 
> > h_client_architecture_support(PowerPCCPU *cpu,
> >      ov5_updates = spapr_ovec_new();
> >      spapr->cas_reboot = spapr_ovec_diff(ov5_updates,
> >                                          ov5_cas_old, spapr->ov5_cas);
> > +    if (kvm_enabled()) {
> > +        if (kvmppc_has_cap_mmu_radix()) {
> 
> I'd still like to see this changed so that it is based on whether an
> HPT *has* been set up, rather than on whether we expect it would have
> been set up.  That will be less fragile against future changes.

I'm not particularly happy with this either but I'm not sure exactly
what you're suggesting.

Do you mean adding some new VM IOCTL to allow allow QEMU to query KVM
and ask what sort of page table has been set up, or would be for a new
VM? I think it would be easy to add that separately so I could follow
this series with one to do it.

> > +            /* If the HPT hasn't yet been set up (see
> > +             * ppc_spapr_reset()), and it's needed, do it now: */
> > +            if (!spapr_ovec_test(ov5_updates, OV5_MMU_RADIX)) {
> > +                /* legacy hash or new hash: */
> > +                spapr_setup_hpt_and_vrma(spapr);
> 
> > +            }
> > +        }
> > +    }
> >  
> >      if (!spapr->cas_reboot) {
> >          spapr->cas_reboot =
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 2975967c07..24da5dc27e 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -591,6 +591,7 @@ void spapr_dt_events(sPAPRMachineState *sm, void *fdt);
> >  int spapr_h_cas_compose_response(sPAPRMachineState *sm,
> >                                   target_ulong addr, target_ulong size,
> >                                   sPAPROptionVector *ov5_updates);
> > +void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr);
> >  sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn);
> >  void spapr_tce_table_enable(sPAPRTCETable *tcet,
> >                              uint32_t page_shift, uint64_t bus_offset,
> 
> -- 
> 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