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

[Greg Kurz]

On Wed, 13 Sep 2017 22:14:03 +1000
David Gibson <address@hidden> 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 <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'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
> >   
>

pgptDNbFMCw03.pgp
Description: OpenPGP digital signature