Re: [Qemu-devel] [PATCH v2 1/2] spapr: introduce common helper to write HPT address to KVM PR

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7389 bytes --]

On Wed, Sep 13, 2017 at 12:24:53AM +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 with a hack
> which is open-coded in several places.
> 
> This patch consolidate the code in a dedicated helper that records
> the HPT address and size in the sPAPR context, and then does the
> magic for KVM PR.
> 
> Note that ppc_spapr_reset() now resets all devices and CPUs before
> allocating the HPT. This allows to drop the hack from spapr_cpu_reset().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

I like this more than the previous spin, but while discussing stuff
with SamB, I thought up a different approach, which I think will be
both cleaner and simpler.

It basically doesn't make sense to put the userspace HPT pointer into
env->spr[SDR1], we only do it to make kvmppc_put_books_sregs() do the
right thing.

Instead, we can have kvmppc_put_books_sregs() populate the "SDR1"
field in kvm_sregs from a vhyp hook.  We already have the reverse side
in that kvmppc_get_books_sregs() doesn't update the internal SDR1
value if vhyp is set.

In any case the spapr hook would compute the correct value direct from
spapr->htab.

After incoming migration I'm not sure we need to do anything - I think
we already do a pretty thorough register resync with KVM.

> ---
>  hw/ppc/spapr.c          |   31 ++++++++++++++++++++++++++-----
>  hw/ppc/spapr_cpu_core.c |   15 ---------------
>  hw/ppc/spapr_hcall.c    |   16 +---------------
>  include/hw/ppc/spapr.h  |    1 +
>  4 files changed, 28 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f680f28a15ea..97f8afdbd7fe 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1309,6 +1309,25 @@ void spapr_free_hpt(sPAPRMachineState *spapr)
>      close_htab_fd(spapr);
>  }
>  
> +void spapr_install_hpt(sPAPRMachineState *spapr, void *htab, uint32_t shift)
> +{
> +    assert(htab);
> +
> +    spapr->htab = htab;
> +    spapr->htab_shift = shift;
> +
> +    /*
> +     * This is a hack for the benefit of KVM PR - it abuses the SDR1
> +     * slot in kvm_sregs to communicate the userspace address of the
> +     * HPT
> +     */
> +    if (kvm_enabled()) {
> +        target_ulong sdr1 = (target_ulong)(uintptr_t)spapr->htab
> +            | (spapr->htab_shift - 18);
> +        kvmppc_update_sdr1(sdr1);
> +    }
> +}
> +
>  void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
>                            Error **errp)
>  {
> @@ -1339,16 +1358,17 @@ void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
>          /* kernel-side HPT not needed, allocate in userspace instead */
>          size_t size = 1ULL << shift;
>          int i;
> +        void *htab;
>  
> -        spapr->htab = qemu_memalign(size, size);
> -        if (!spapr->htab) {
> +        htab = qemu_memalign(size, size);
> +        if (!htab) {
>              error_setg_errno(errp, errno,
>                               "Could not allocate HPT of order %d", shift);
>              return;
>          }
>  
> -        memset(spapr->htab, 0, size);
> -        spapr->htab_shift = shift;
> +        memset(htab, 0, size);
> +        spapr_install_hpt(spapr, htab, shift);
>  
>          for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
>              DIRTY_HPTE(HPTE(spapr->htab, i));
> @@ -1405,6 +1425,8 @@ static void ppc_spapr_reset(void)
>      /* Check for unknown sysbus devices */
>      foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
>  
> +    qemu_devices_reset();
> +
>      if (kvm_enabled() && kvmppc_has_cap_mmu_radix()) {
>          /* If using KVM with radix mode available, VCPUs can be started
>           * without a HPT because KVM will start them in radix mode.
> @@ -1414,7 +1436,6 @@ static void ppc_spapr_reset(void)
>          spapr_setup_hpt_and_vrma(spapr);
>      }
>  
> -    qemu_devices_reset();
>      spapr_clear_pending_events(spapr);
>  
>      /*
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index c08ee7571a50..c20b5c64b045 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -73,7 +73,6 @@ void spapr_cpu_parse_features(sPAPRMachineState *spapr)
>  
>  static void spapr_cpu_reset(void *opaque)
>  {
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      PowerPCCPU *cpu = opaque;
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
> @@ -86,20 +85,6 @@ static void spapr_cpu_reset(void *opaque)
>      cs->halted = 1;
>  
>      env->spr[SPR_HIOR] = 0;
> -
> -    /*
> -     * This is a hack for the benefit of KVM PR - it abuses the SDR1
> -     * slot in kvm_sregs to communicate the userspace address of the
> -     * 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);
> -        }
> -    }
>  }
>  
>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 57bb411394ed..7892cd3e7ffa 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -730,15 +730,7 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu,
>                      pending->hpt, newsize);
>      if (rc == H_SUCCESS) {
>          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);
> -        }
> +        spapr_install_hpt(spapr, pending->hpt, pending->shift);
>  
>          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);
> -            }
>          }
>      }
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index c1b365f56431..30e5805acca4 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);
>  
> +void spapr_install_hpt(sPAPRMachineState *spapr, void *htab, uint32_t shift);
>  #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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]