Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/3] target-ppc: Synchronize VPA state with KVM

[David Gibson]

On Mon, Feb 25, 2013 at 01:19:48PM +0100, Alexander Graf wrote:
> 
> On 21.02.2013, at 03:41, David Gibson wrote:
> 
> > For PAPR guests, KVM tracks the various areas registered with the
> > H_REGISTER_VPA hypercall.  For full emulation, of course, these are tracked
> > within qemu.  At present these values are not synchronized.  This is a
> > problem for reset (qemu's reset of the VPA address is not pushed to KVM)
> > and will also be a problem for savevm / migration.
> > 
> > The kernel now supports accessing the VPA state via the ONE_REG interface,
> > this patch adds code to qemu to use that interface to keep the qemu and
> > KVM ideas of the VPA state synchronized.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> > target-ppc/kvm.c |  122 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 122 insertions(+)
> > 
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index 5a6f608..ac310c1 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -62,6 +62,7 @@ static int cap_ppc_rma;
> > static int cap_spapr_tce;
> > static int cap_hior;
> > static int cap_one_reg;
> > +static int cap_papr;
> > 
> > /* XXX We have a race condition where we actually have a level triggered
> >  *     interrupt, but the infrastructure can't expose that yet, so the guest
> > @@ -92,6 +93,8 @@ int kvm_arch_init(KVMState *s)
> >     cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
> >     cap_one_reg = kvm_check_extension(s, KVM_CAP_ONE_REG);
> >     cap_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
> > +    /* Note: we don't set cap_papr here, because this capability is
> > +     * only activated after this by kvmppc_set_papr() */
> > 
> >     if (!cap_interrupt_level) {
> >         fprintf(stderr, "KVM: Couldn't find level irq capability. Expect 
> > the "
> > @@ -647,6 +650,103 @@ static int kvm_get_fp(CPUState *cs)
> >     return 0;
> > }
> > 
> > +#if defined(TARGET_PPC64)
> > +static int kvm_get_vpa(CPUState *cs)
> > +{
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUPPCState *env = &cpu->env;
> > +    struct kvm_one_reg reg;
> > +    int ret;
> > +
> > +    reg.id = KVM_REG_PPC_VPA_ADDR;
> > +    reg.addr = (uintptr_t)&env->vpa_addr;
> > +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> > +    if (ret < 0) {
> > +        dprintf("Unable to get VPA address from KVM: %s\n", 
> > strerror(errno));
> > +        return ret;
> > +    }
> > +
> > +    assert((uintptr_t)&env->slb_shadow_size
> > +           == ((uintptr_t)&env->slb_shadow_addr + 8));
> > +    reg.id = KVM_REG_PPC_VPA_SLB;
> > +    reg.addr = (uintptr_t)&env->slb_shadow_addr;
> > +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> > +    if (ret < 0) {
> > +        dprintf("Unable to get SLB shadow state from KVM: %s\n",
> > +                strerror(errno));
> > +        return ret;
> > +    }
> > +
> > +    assert((uintptr_t)&env->dtl_size == ((uintptr_t)&env->dtl_addr + 8));
> > +    reg.id = KVM_REG_PPC_VPA_DTL;
> > +    reg.addr = (uintptr_t)&env->dtl_addr;
> > +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> > +    if (ret < 0) {
> > +        dprintf("Unable to get dispatch trace log state from KVM: %s\n",
> > +                strerror(errno));
> > +        return ret;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int kvm_put_vpa(CPUState *cs)
> > +{
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUPPCState *env = &cpu->env;
> > +    struct kvm_one_reg reg;
> > +    int ret;
> > +
> > +    /* SLB shadow or DTL can't be registered unless a master VPA is
> > +     * registered.  That means when restoring state, if a VPA *is*
> > +     * registered, we need to set that up first.  If not, we need to
> > +     * deregister the others before deregistering the master VPA */
> > +    assert(env->vpa_addr || !(env->slb_shadow_addr || env->dtl_addr));
> > +
> > +    if (env->vpa_addr) {
> > +        reg.id = KVM_REG_PPC_VPA_ADDR;
> > +        reg.addr = (uintptr_t)&env->vpa_addr;
> > +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> > +        if (ret < 0) {
> > +            dprintf("Unable to set VPA address to KVM: %s\n", 
> > strerror(errno));
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    assert((uintptr_t)&env->slb_shadow_size
> > +           == ((uintptr_t)&env->slb_shadow_addr + 8));
> > +    reg.id = KVM_REG_PPC_VPA_SLB;
> > +    reg.addr = (uintptr_t)&env->slb_shadow_addr;
> > +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> > +    if (ret < 0) {
> > +        dprintf("Unable to set SLB shadow state to KVM: %s\n", 
> > strerror(errno));
> > +        return ret;
> > +    }
> > +
> > +    assert((uintptr_t)&env->dtl_size == ((uintptr_t)&env->dtl_addr + 8));
> > +    reg.id = KVM_REG_PPC_VPA_DTL;
> > +    reg.addr = (uintptr_t)&env->dtl_addr;
> > +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> > +    if (ret < 0) {
> > +        dprintf("Unable to set dispatch trace log state to KVM: %s\n",
> > +                strerror(errno));
> > +        return ret;
> > +    }
> > +
> > +    if (!env->vpa_addr) {
> > +        reg.id = KVM_REG_PPC_VPA_ADDR;
> > +        reg.addr = (uintptr_t)&env->vpa_addr;
> > +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> > +        if (ret < 0) {
> > +            dprintf("Unable to set VPA address to KVM: %s\n", 
> > strerror(errno));
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +#endif /* TARGET_PPC64 */
> > +
> > int kvm_arch_put_registers(CPUState *cs, int level)
> > {
> >     PowerPCCPU *cpu = POWERPC_CPU(cs);
> > @@ -747,6 +847,15 @@ int kvm_arch_put_registers(CPUState *cs, int level)
> >                 kvm_put_one_spr(cs, id, i);
> >             }
> >         }
> > +
> > +#ifdef TARGET_PPC64
> > +        if (cap_papr) {
> > +            if (kvm_put_vpa(cs) < 0) {
> > +                fprintf(stderr,
> > +                        "Warning: Unable to set VPA information to KVM\n");
> 
> This makes newer QEMU on older host kernels unusable, right? It
> would spill that warning all over the place on every register
> synchronization. Are you sure this is what you want?

Um.. I'm not sure why you single out this patch.  The SPR and FPU sync
patches introcued similar warnings, which can also spew on older
hosts.  The VPA is arguably the most harmful not to sync, since reset
will fail without it.

> How about adding a global to indicate that you've already emitted
> the warning before? Only print it when you didn't.

Yeah, I could.  Doing that for FPU and SPRs as well starts to add up
to a lot of global flags, though.  Actually for SPRs, I guess I could
just zero out the id field in the ppc_spr_t.  Bit of a hack, but does
the job.

-- 
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: Digital signature