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 <david@gibson.dropbear.id.au>
Signed-off-by: Greg Kurz <groug@kaod.org>
---

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.

 hw/ppc/spapr.c          |   13 +++++++++++++
 hw/ppc/spapr_cpu_core.c |   15 ---------------
 hw/ppc/spapr_hcall.c    |   14 --------------
 target/ppc/cpu.h        |    2 ++
 target/ppc/kvm.c        |   30 +++++-------------------------
 target/ppc/kvm_ppc.h    |    6 ------
 6 files changed, 20 insertions(+), 60 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f680f28a15ea..7e7ae585a1b6 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1239,6 +1239,18 @@ static hwaddr spapr_hpt_mask(PPCVirtualHypervisor *vhyp)
     return HTAB_SIZE(spapr) / HASH_PTEG_SIZE_64 - 1;
 }
 
+static void spapr_encode_hpt_for_kvm_pr(PPCVirtualHypervisor *vhyp,
+                                        target_ulong *hpt)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(vhyp);
+
+    assert(kvm_enabled());
+
+    if (spapr->htab) {
+        *hpt = (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift - 18);
+    }
+}
+
 static const ppc_hash_pte64_t *spapr_map_hptes(PPCVirtualHypervisor *vhyp,
                                                 hwaddr ptex, int n)
 {
@@ -3603,6 +3615,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     vhc->unmap_hptes = spapr_unmap_hptes;
     vhc->store_hpte = spapr_store_hpte;
     vhc->get_patbe = spapr_get_patbe;
+    vhc->encode_hpt_for_kvm_pr = spapr_encode_hpt_for_kvm_pr;
     xic->ics_get = spapr_ics_get;
     xic->ics_resend = spapr_ics_resend;
     xic->icp_get = spapr_icp_get;
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..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);
-        }
-
         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

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 <david@gibson.dropbear.id.au>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> 
> 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

On Wed, 13 Sep 2017 22:14:03 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> 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 <david@gibson.dropbear.id.au>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > 
> > 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'll check this out.

> 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 had tested with the previous version and it worked. I'll try again with
this patch.

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

Oops you're right. Since this isn't a hot path, would it be acceptable to
call kvmppc_put_books_sregs() for all CPUs instead of keeping all the sdr1
specific hack ?

> >          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);
> > -            }

Same here.

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

On Wed, Sep 13, 2017 at 02:48:10PM +0200, Greg Kurz wrote:
> On Wed, 13 Sep 2017 22:14:03 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > 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 <david@gibson.dropbear.id.au>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > 
> > > 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'll check this out.
> 
> > 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 had tested with the previous version and it worked. I'll try again with
> this patch.
> 
> > 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.
> > 
> 
> Oops you're right. Since this isn't a hot path, would it be acceptable to
> call kvmppc_put_books_sregs() for all CPUs instead of keeping all the sdr1
> specific hack ?

Yeah, should be fine.  I think the old one ended up doing that after
setting spr[SDR1] anyway.

> 
> > >          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);
> > > -            }
> 
> Same here.

Yes, good catch.

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