Re: [Qemu-devel] [RFC PATCH v2 08/12] spapr: Only setup HTP if necessary.

[David Gibson]

s/HTP/HPT/ in subject line.


On Thu, Feb 23, 2017 at 05:00:01PM +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.
> 
> Signed-off-by: Sam Bobroff <address@hidden>

So, IIRC, we discussed previously that the logical way to do things
was to, by default, delay HPT allocation until CAS time, and just do
it at reset time for the case that needs it: hash host with KVM.

Did you hit a problem with that approach, or is there still work to be
done here?

> ---
> v2:
> 
> * This patch has been mostly rewritten to move the late HPT allocation to CAS.
> This allows a guest to start in radix mode (when it's in real mode) and then
> change to hash, even if it is a legacy guest and will not call
> h_register_process_table().
> * Added an exported function to spapr.c to perform HPT allocation and adjust
> the vrma if necessary. This makes it possible to allocate the HPT from
> h_client_architecture_support() in spapr_hcall.c.
> 
>  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 ca3812555f..dfee0f685f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1123,6 +1123,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(qdev_get_machine())->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;
> @@ -1151,15 +1162,10 @@ static void ppc_spapr_reset(void)
>      /* Check for unknown sysbus devices */
>      foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
>  
> -    /* 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 42d20e0b92..cea34073aa 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1002,6 +1002,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()) {
> +            /* If the HPT hasn't yet been set up (see
> +             * ppc_spapr_reset()), and it's needed, do it now: */

I think it's a bit fragile to have here it explicitly mirror the logic
which determines whether the HPT is allocated early.  I'd prefer to
explicitly test here whether we have allocated an HPT - adding a flag,
if we have to.

> +            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 f9b17d860a..a30cbc485c 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -590,6 +590,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

signature.asc
Description: PGP signature