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

[Alexander Graf]

Am 26.02.2013 um 03:01 schrieb David Gibson <address@hidden>:

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

No, they use dprintf :). You can also switch VPA to dprintf and thus get rid of 
the problem.

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

Yup, sounds good :).

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