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

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

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

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

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