Re: [Qemu-devel] [PATCH] spapr: fix the value of SDR1 in kvmppc_put_books_sregs()

[David Gibson]

On Wed, Sep 13, 2017 at 12:41:07PM +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 by hacking
> the value of SDR1 and pushing it to KVM in several places.
> 
> Also, migration breaks the guest since it is very unlikely the HPT has
> the same address in source and destination, but we push the incoming
> value of SDR1 to KVM anyway.
> 
> This patch introduces a new virtual hypervisor hook so that the spapr
> code can provide the correct value of SDR1 to be pushed to KVM each
> time kvmppc_put_books_sregs() is called.
> 
> It allows to get rid of all the hacking in the spapr/kvmppc code and
> it fixes migration of nested KVM PR.
> 
> Suggested-by: David Gibson <address@hidden>
> Signed-off-by: Greg Kurz <address@hidden>
> ---
> 
> Unfortunately, this doesn't fix migration when using KVM PR on baremetal.
> I get tons of instruction emulation errors in the console when the guest
> resumes execution at the destination:
> 
> [  833.585957] Couldn't emulate instruction 0x00000000 (op 0 xop 0)
> [  833.586818] kvmppc_exit_pr_progint: emulation at 60000000600e5bfc failed
>  (00000000)
> [  833.674684] Couldn't emulate instruction 0x00000000 (op 0 xop 0)
> [  833.675370] kvmppc_exit_pr_progint: emulation at 60000000600e5bfc failed
>  (00000000)
> [  833.676297] Couldn't emulate instruction 0x00000000 (op 0 xop 0)
> 
> I still don't know what's happening here but I guess this is an issue in
> KVM.

Not necessarily, those could just be because we don't have meaningful
hash table.  It's possible we're just not pushing the sregs info to
KVM after the incoming migration.  I still think whatever fix we'll
need for that will be simpler than the earlier versions.

I'll need to test this with HPT resizing (unless you already have).  I
think there is a small problem..

[snip]
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 57bb411394ed..840abff0e371 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -732,14 +732,6 @@ static target_ulong h_resize_hpt_commit(PowerPCCPU *cpu,
>          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);
> -        }
> -

I don't think we can completely eliminate this for KVM PR.  The sregs
aren't written back to KVM on every re-entry, only rarely.  So we'll
still need to force a writeback of all the sregs stuff in order to get
PR updated properly.

>          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/target/ppc/cpu.h b/target/ppc/cpu.h
> index c9d3ffa89bcb..12e0b6e74876 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1243,6 +1243,8 @@ struct PPCVirtualHypervisorClass {
>      void (*store_hpte)(PPCVirtualHypervisor *vhyp, hwaddr ptex,
>                         uint64_t pte0, uint64_t pte1);
>      uint64_t (*get_patbe)(PPCVirtualHypervisor *vhyp);
> +    void (*encode_hpt_for_kvm_pr)(PPCVirtualHypervisor *vhyp,
> +                                  target_ulong *hpt);
>  };
>  
>  #define TYPE_PPC_VIRTUAL_HYPERVISOR "ppc-virtual-hypervisor"
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 6442dfcb95b3..8edc05fc6a83 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -941,7 +941,11 @@ int kvmppc_put_books_sregs(PowerPCCPU *cpu)
>  
>      sregs.pvr = env->spr[SPR_PVR];
>  
> -    sregs.u.s.sdr1 = env->spr[SPR_SDR1];
> +    if (cpu->vhyp) {
> +        PPCVirtualHypervisorClass *vhc =
> +            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> +        vhc->encode_hpt_for_kvm_pr(cpu->vhyp, &sregs.u.s.sdr1);
> +    }
>  
>      /* Sync SLB */
>  #ifdef TARGET_PPC64
> @@ -2806,30 +2810,6 @@ int kvmppc_resize_hpt_commit(PowerPCCPU *cpu, 
> target_ulong flags, int shift)
>      return kvm_vm_ioctl(cs->kvm_state, KVM_PPC_RESIZE_HPT_COMMIT, &rhpt);
>  }
>  
> -static void kvmppc_pivot_hpt_cpu(CPUState *cs, run_on_cpu_data arg)
> -{
> -    target_ulong sdr1 = arg.target_ptr;
> -    PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    CPUPPCState *env = &cpu->env;
> -
> -    /* This is just for the benefit of PR KVM */
> -    cpu_synchronize_state(cs);
> -    env->spr[SPR_SDR1] = sdr1;
> -    if (kvmppc_put_books_sregs(cpu) < 0) {
> -        error_report("Unable to update SDR1 in KVM");
> -        exit(1);
> -    }
> -}
> -
> -void kvmppc_update_sdr1(target_ulong sdr1)
> -{
> -    CPUState *cs;
> -
> -    CPU_FOREACH(cs) {
> -        run_on_cpu(cs, kvmppc_pivot_hpt_cpu, RUN_ON_CPU_TARGET_PTR(sdr1));
> -    }
> -}
> -
>  /*
>   * This is a helper function to detect a post migration scenario
>   * in which a guest, running as KVM-HV, freezes in cpu_post_load because
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index f780e6ec7b72..21571edd7910 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -68,7 +68,6 @@ PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void);
>  void kvmppc_check_papr_resize_hpt(Error **errp);
>  int kvmppc_resize_hpt_prepare(PowerPCCPU *cpu, target_ulong flags, int 
> shift);
>  int kvmppc_resize_hpt_commit(PowerPCCPU *cpu, target_ulong flags, int shift);
> -void kvmppc_update_sdr1(target_ulong sdr1);
>  bool kvmppc_pvr_workaround_required(PowerPCCPU *cpu);
>  
>  bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path);
> @@ -331,11 +330,6 @@ static inline int kvmppc_resize_hpt_commit(PowerPCCPU 
> *cpu,
>      return -ENOSYS;
>  }
>  
> -static inline void kvmppc_update_sdr1(target_ulong sdr1)
> -{
> -    abort();
> -}
> -
>  #endif
>  
>  #ifndef CONFIG_KVM
> 

-- 
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