Patchwork [3/6] target-ppc: Synchronize more SPRs to KVM using ONE_REG interface

login
register
mail settings
Submitter David Gibson
Date Jan. 24, 2013, 3:20 a.m.
Message ID <1358997643-20728-4-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/215133/
State New
Headers show

Comments

David Gibson - Jan. 24, 2013, 3:20 a.m.
There are currently a batch of occasionally used SPRs whose state we do
not synchronize with KVM.  This might be a problem for debugging, and will
definitely be a problem for savevm / migration.  KVM now supports accessing
these registers via the KVM_{GET,SET}_ONE_REG interface, so this patch
wires up the code to use this on the qemu side.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target-ppc/cpu.h |    4 ++
 target-ppc/kvm.c |  142 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 136 insertions(+), 10 deletions(-)
Alexander Graf - Jan. 24, 2013, 4:32 p.m.
On 24.01.2013, at 04:20, David Gibson wrote:

> There are currently a batch of occasionally used SPRs whose state we do
> not synchronize with KVM.  This might be a problem for debugging, and will
> definitely be a problem for savevm / migration.  KVM now supports accessing
> these registers via the KVM_{GET,SET}_ONE_REG interface, so this patch
> wires up the code to use this on the qemu side.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> target-ppc/cpu.h |    4 ++
> target-ppc/kvm.c |  142 ++++++++++++++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 136 insertions(+), 10 deletions(-)
> 
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 953146e..6cb79fe 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1319,6 +1319,7 @@ static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
> #define SPR_MPC_CMPH          (0x09B)
> #define SPR_MPC_LCTRL1        (0x09C)
> #define SPR_MPC_LCTRL2        (0x09D)
> +#define SPR_UAMOR             (0x09D)
> #define SPR_MPC_ICTRL         (0x09E)
> #define SPR_MPC_BAR           (0x09F)
> #define SPR_VRSAVE            (0x100)
> @@ -1535,6 +1536,7 @@ static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
> #define SPR_UPERF0            (0x310)
> #define SPR_UPERF1            (0x311)
> #define SPR_UPERF2            (0x312)
> +#define SPR_MMCRA             (0x312)
> #define SPR_UPERF3            (0x313)
> #define SPR_620_PMC1W         (0x313)
> #define SPR_UPERF4            (0x314)
> @@ -1544,7 +1546,9 @@ static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
> #define SPR_UPERF7            (0x317)
> #define SPR_UPERF8            (0x318)
> #define SPR_UPERF9            (0x319)
> +#define SPR_PMC7              (0x319)
> #define SPR_UPERFA            (0x31A)
> +#define SPR_PMC8              (0x31A)
> #define SPR_UPERFB            (0x31B)
> #define SPR_620_MMCR0W        (0x31B)
> #define SPR_UPERFC            (0x31C)
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 2f4f068..7604d0b 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -61,6 +61,7 @@ static int cap_ppc_smt;
> static int cap_ppc_rma;
> static int cap_spapr_tce;
> static int cap_hior;
> +static int cap_one_reg;
> 
> /* 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
> @@ -89,6 +90,7 @@ int kvm_arch_init(KVMState *s)
>     cap_ppc_smt = kvm_check_extension(s, KVM_CAP_PPC_SMT);
>     cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
>     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);
> 
>     if (!cap_interrupt_level) {
> @@ -444,6 +446,76 @@ static void kvm_sw_tlb_put(PowerPCCPU *cpu)
>     g_free(bitmap);
> }
> 
> +static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    union {
> +        uint32_t u32;
> +        uint64_t u64;
> +    } val;
> +    struct kvm_one_reg reg = {
> +        .id = id,
> +        .addr = (uintptr_t) &val,
> +    };
> +    int ret;
> +
> +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +    if (ret != 0) {
> +        fprintf(stderr, "Warning: Unable to retrieve SPR %d from KVM: %s\n",
> +                spr, strerror(errno));

Any way you could make this a WARN_ONCE style message?

> +    } else {
> +        switch (id & KVM_REG_SIZE_MASK) {
> +        case KVM_REG_SIZE_U32:
> +            env->spr[spr] = val.u32;
> +            break;
> +
> +        case KVM_REG_SIZE_U64:
> +            env->spr[spr] = val.u64;
> +            break;
> +
> +        default:
> +            /* Don't handle this size yet */
> +            abort();
> +        }
> +    }
> +}
> +
> +static void kvm_put_one_spr(CPUState *cs, uint64_t id, int spr)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    union {
> +        uint32_t u32;
> +        uint64_t u64;
> +    } val;
> +    struct kvm_one_reg reg = {
> +        .id = id,
> +        .addr = (uintptr_t) &val,
> +    };
> +    int ret;
> +
> +    switch (id & KVM_REG_SIZE_MASK) {
> +    case KVM_REG_SIZE_U32:
> +        val.u32 = env->spr[spr];
> +        break;
> +
> +    case KVM_REG_SIZE_U64:
> +        val.u64 = env->spr[spr];
> +        break;
> +
> +    default:
> +        /* Don't handle this size yet */
> +        abort();
> +    }
> +
> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +    if (ret != 0) {
> +        fprintf(stderr, "Warning: Unable to set SPR %d to KVM: %s\n",
> +                spr, strerror(errno));
> +    }
> +}
> +
> int kvm_arch_put_registers(CPUState *cs, int level)
> {
>     PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -524,16 +596,35 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>         }
>     }
> 
> -    if (cap_hior && (level >= KVM_PUT_RESET_STATE)) {
> -        uint64_t hior = env->spr[SPR_HIOR];
> -        struct kvm_one_reg reg = {
> -            .id = KVM_REG_PPC_HIOR,
> -            .addr = (uintptr_t) &hior,
> -        };
> -
> -        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> -        if (ret) {
> -            return ret;
> +    if (cap_one_reg) {

This needs a check that we're on book3s. Otherwise you just spam booke users.


Alex

> +        /* We deliberately ignore errors here, for kernels which have
> +         * the ONE_REG calls, but don't support the specific
> +         * registers.  There's still a good chance things will work,
> +         * as long as we don't try to migrate */
> +        kvm_put_one_spr(cs, KVM_REG_PPC_DABR, SPR_DABR);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_DSCR, SPR_DSCR);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_PURR, SPR_PURR);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_SPURR, SPR_SPURR);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_DAR, SPR_DAR);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_DSISR, SPR_DSISR);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_AMR, SPR_AMR);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_UAMOR, SPR_UAMOR);
> +
> +        kvm_put_one_spr(cs, KVM_REG_PPC_MMCR0, SPR_MMCR0);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_MMCR1, SPR_MMCR1);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_MMCRA, SPR_MMCRA);
> +
> +        kvm_put_one_spr(cs, KVM_REG_PPC_PMC1, SPR_PMC1);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_PMC2, SPR_PMC2);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_PMC3, SPR_PMC3);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_PMC4, SPR_PMC4);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_PMC5, SPR_PMC5);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_PMC6, SPR_PMC6);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_PMC7, SPR_PMC7);
> +        kvm_put_one_spr(cs, KVM_REG_PPC_PMC8, SPR_PMC8);
> +
> +        if (cap_hior && (level >= KVM_PUT_RESET_STATE)) {
> +            kvm_put_one_spr(cs, KVM_REG_PPC_HIOR, SPR_HIOR);
>         }
>     }
> 
> @@ -716,6 +807,37 @@ int kvm_arch_get_registers(CPUState *cs)
>         }
>     }
> 
> +    if (cap_one_reg) {
> +        /* We deliberately ignore errors here, for kernels which have
> +         * the ONE_REG calls, but don't support the specific
> +         * registers.  There's still a good chance things will work,
> +         * as long as we don't try to migrate */
> +        if (cap_hior) {
> +            kvm_get_one_spr(cs, KVM_REG_PPC_HIOR, SPR_HIOR);
> +        }
> +        kvm_get_one_spr(cs, KVM_REG_PPC_DABR, SPR_DABR);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_DSCR, SPR_DSCR);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_PURR, SPR_PURR);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_SPURR, SPR_SPURR);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_DAR, SPR_DAR);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_DSISR, SPR_DSISR);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_AMR, SPR_AMR);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_UAMOR, SPR_UAMOR);
> +
> +        kvm_get_one_spr(cs, KVM_REG_PPC_MMCR0, SPR_MMCR0);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_MMCR1, SPR_MMCR1);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_MMCRA, SPR_MMCRA);
> +
> +        kvm_get_one_spr(cs, KVM_REG_PPC_PMC1, SPR_PMC1);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_PMC2, SPR_PMC2);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_PMC3, SPR_PMC3);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_PMC4, SPR_PMC4);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_PMC5, SPR_PMC5);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_PMC6, SPR_PMC6);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_PMC7, SPR_PMC7);
> +        kvm_get_one_spr(cs, KVM_REG_PPC_PMC8, SPR_PMC8);
> +    }
> +
>     return 0;
> }
> 
> -- 
> 1.7.10.4
>
David Gibson - Jan. 25, 2013, 2:39 a.m.
On Thu, Jan 24, 2013 at 05:32:42PM +0100, Alexander Graf wrote:
> 
> On 24.01.2013, at 04:20, David Gibson wrote:
> 
> > There are currently a batch of occasionally used SPRs whose state we do
> > not synchronize with KVM.  This might be a problem for debugging, and will
> > definitely be a problem for savevm / migration.  KVM now supports accessing
> > these registers via the KVM_{GET,SET}_ONE_REG interface, so this patch
> > wires up the code to use this on the qemu side.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > target-ppc/cpu.h |    4 ++
> > target-ppc/kvm.c |  142 ++++++++++++++++++++++++++++++++++++++++++++++++++----
> > 2 files changed, 136 insertions(+), 10 deletions(-)
> > 
> > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> > index 953146e..6cb79fe 100644
> > --- a/target-ppc/cpu.h
> > +++ b/target-ppc/cpu.h
> > @@ -1319,6 +1319,7 @@ static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
> > #define SPR_MPC_CMPH          (0x09B)
> > #define SPR_MPC_LCTRL1        (0x09C)
> > #define SPR_MPC_LCTRL2        (0x09D)
> > +#define SPR_UAMOR             (0x09D)
> > #define SPR_MPC_ICTRL         (0x09E)
> > #define SPR_MPC_BAR           (0x09F)
> > #define SPR_VRSAVE            (0x100)
> > @@ -1535,6 +1536,7 @@ static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
> > #define SPR_UPERF0            (0x310)
> > #define SPR_UPERF1            (0x311)
> > #define SPR_UPERF2            (0x312)
> > +#define SPR_MMCRA             (0x312)
> > #define SPR_UPERF3            (0x313)
> > #define SPR_620_PMC1W         (0x313)
> > #define SPR_UPERF4            (0x314)
> > @@ -1544,7 +1546,9 @@ static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
> > #define SPR_UPERF7            (0x317)
> > #define SPR_UPERF8            (0x318)
> > #define SPR_UPERF9            (0x319)
> > +#define SPR_PMC7              (0x319)
> > #define SPR_UPERFA            (0x31A)
> > +#define SPR_PMC8              (0x31A)
> > #define SPR_UPERFB            (0x31B)
> > #define SPR_620_MMCR0W        (0x31B)
> > #define SPR_UPERFC            (0x31C)
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index 2f4f068..7604d0b 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -61,6 +61,7 @@ static int cap_ppc_smt;
> > static int cap_ppc_rma;
> > static int cap_spapr_tce;
> > static int cap_hior;
> > +static int cap_one_reg;
> > 
> > /* 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
> > @@ -89,6 +90,7 @@ int kvm_arch_init(KVMState *s)
> >     cap_ppc_smt = kvm_check_extension(s, KVM_CAP_PPC_SMT);
> >     cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
> >     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);
> > 
> >     if (!cap_interrupt_level) {
> > @@ -444,6 +446,76 @@ static void kvm_sw_tlb_put(PowerPCCPU *cpu)
> >     g_free(bitmap);
> > }
> > 
> > +static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr)
> > +{
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUPPCState *env = &cpu->env;
> > +    union {
> > +        uint32_t u32;
> > +        uint64_t u64;
> > +    } val;
> > +    struct kvm_one_reg reg = {
> > +        .id = id,
> > +        .addr = (uintptr_t) &val,
> > +    };
> > +    int ret;
> > +
> > +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> > +    if (ret != 0) {
> > +        fprintf(stderr, "Warning: Unable to retrieve SPR %d from KVM: %s\n",
> > +                spr, strerror(errno));
> 
> Any way you could make this a WARN_ONCE style message?

Uh.. I guess.  Afaict, qemu doesn't have any infrastructure for that,
so I'd have to roll my own.
Alexander Graf - Jan. 25, 2013, 11:24 a.m.
On 25.01.2013, at 03:39, David Gibson wrote:

> On Thu, Jan 24, 2013 at 05:32:42PM +0100, Alexander Graf wrote:
>> 
>> On 24.01.2013, at 04:20, David Gibson wrote:
>> 
>>> There are currently a batch of occasionally used SPRs whose state we do
>>> not synchronize with KVM.  This might be a problem for debugging, and will
>>> definitely be a problem for savevm / migration.  KVM now supports accessing
>>> these registers via the KVM_{GET,SET}_ONE_REG interface, so this patch
>>> wires up the code to use this on the qemu side.
>>> 
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>> target-ppc/cpu.h |    4 ++
>>> target-ppc/kvm.c |  142 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>>> 2 files changed, 136 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>> index 953146e..6cb79fe 100644
>>> --- a/target-ppc/cpu.h
>>> +++ b/target-ppc/cpu.h
>>> @@ -1319,6 +1319,7 @@ static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
>>> #define SPR_MPC_CMPH          (0x09B)
>>> #define SPR_MPC_LCTRL1        (0x09C)
>>> #define SPR_MPC_LCTRL2        (0x09D)
>>> +#define SPR_UAMOR             (0x09D)
>>> #define SPR_MPC_ICTRL         (0x09E)
>>> #define SPR_MPC_BAR           (0x09F)
>>> #define SPR_VRSAVE            (0x100)
>>> @@ -1535,6 +1536,7 @@ static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
>>> #define SPR_UPERF0            (0x310)
>>> #define SPR_UPERF1            (0x311)
>>> #define SPR_UPERF2            (0x312)
>>> +#define SPR_MMCRA             (0x312)
>>> #define SPR_UPERF3            (0x313)
>>> #define SPR_620_PMC1W         (0x313)
>>> #define SPR_UPERF4            (0x314)
>>> @@ -1544,7 +1546,9 @@ static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
>>> #define SPR_UPERF7            (0x317)
>>> #define SPR_UPERF8            (0x318)
>>> #define SPR_UPERF9            (0x319)
>>> +#define SPR_PMC7              (0x319)
>>> #define SPR_UPERFA            (0x31A)
>>> +#define SPR_PMC8              (0x31A)
>>> #define SPR_UPERFB            (0x31B)
>>> #define SPR_620_MMCR0W        (0x31B)
>>> #define SPR_UPERFC            (0x31C)
>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>> index 2f4f068..7604d0b 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -61,6 +61,7 @@ static int cap_ppc_smt;
>>> static int cap_ppc_rma;
>>> static int cap_spapr_tce;
>>> static int cap_hior;
>>> +static int cap_one_reg;
>>> 
>>> /* 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
>>> @@ -89,6 +90,7 @@ int kvm_arch_init(KVMState *s)
>>>    cap_ppc_smt = kvm_check_extension(s, KVM_CAP_PPC_SMT);
>>>    cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
>>>    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);
>>> 
>>>    if (!cap_interrupt_level) {
>>> @@ -444,6 +446,76 @@ static void kvm_sw_tlb_put(PowerPCCPU *cpu)
>>>    g_free(bitmap);
>>> }
>>> 
>>> +static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr)
>>> +{
>>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>>> +    CPUPPCState *env = &cpu->env;
>>> +    union {
>>> +        uint32_t u32;
>>> +        uint64_t u64;
>>> +    } val;
>>> +    struct kvm_one_reg reg = {
>>> +        .id = id,
>>> +        .addr = (uintptr_t) &val,
>>> +    };
>>> +    int ret;
>>> +
>>> +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
>>> +    if (ret != 0) {
>>> +        fprintf(stderr, "Warning: Unable to retrieve SPR %d from KVM: %s\n",
>>> +                spr, strerror(errno));
>> 
>> Any way you could make this a WARN_ONCE style message?
> 
> Uh.. I guess.  Afaict, qemu doesn't have any infrastructure for that,
> so I'd have to roll my own.

As I stated in a later patch review, I think it makes sense to just not print anything by default.

Usually, you would do

if (cap_foo) {
    get_one_spr(...);
}

right? With one_reg I consciously omitted caps for every register, because you can just as well read and push the reg itself and thus know whether it works.

So if you translate that to the above code, it would mean

   get_one_spr(...);

without error message would be semantically identical to the first version.

It would however make sense to have a debug print version in case you want to know why something goes wrong. It might also make sense to have an initial get_one_spr() or so for a FULL register push to warn the user that live migration will not work on his machine.


Alex
David Gibson - Jan. 29, 2013, 1:32 a.m.
On Fri, Jan 25, 2013 at 12:24:06PM +0100, Alexander Graf wrote:
> On 25.01.2013, at 03:39, David Gibson wrote:
> > On Thu, Jan 24, 2013 at 05:32:42PM +0100, Alexander Graf wrote:
[snip]
> >>> +static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr)
> >>> +{
> >>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> >>> +    CPUPPCState *env = &cpu->env;
> >>> +    union {
> >>> +        uint32_t u32;
> >>> +        uint64_t u64;
> >>> +    } val;
> >>> +    struct kvm_one_reg reg = {
> >>> +        .id = id,
> >>> +        .addr = (uintptr_t) &val,
> >>> +    };
> >>> +    int ret;
> >>> +
> >>> +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> >>> +    if (ret != 0) {
> >>> +        fprintf(stderr, "Warning: Unable to retrieve SPR %d from KVM: %s\n",
> >>> +                spr, strerror(errno));
> >> 
> >> Any way you could make this a WARN_ONCE style message?
> > 
> > Uh.. I guess.  Afaict, qemu doesn't have any infrastructure for that,
> > so I'd have to roll my own.
> 
> As I stated in a later patch review, I think it makes sense to just
> not print anything by default.
> 
> Usually, you would do
> 
> if (cap_foo) {
>     get_one_spr(...);
> }
> 
> right? With one_reg I consciously omitted caps for every register,
> because you can just as well read and push the reg itself and thus
> know whether it works.
> 
> So if you translate that to the above code, it would mean
> 
>    get_one_spr(...);
> 
> without error message would be semantically identical to the first
> version.
> 
> It would however make sense to have a debug print version in case
> you want to know why something goes wrong. It might also make sense
> to have an initial get_one_spr() or so for a FULL register push to
> warn the user that live migration will not work on his machine.

So, I realised I need to rework this substantially anyway.  As you
say, I need to not attempt to sync these various SPRs for BookE.  But
more generally, different CPUs will have a different set of supported
SPRs.

I have a draft in the works that adds a ONE_REG id code to the
ppc_spr_t structure, which is (optionally) initialized by
spr_register() calls in the cpu initialization.  Then the kvm call
goes through the SPR table and just syncs all those with registered
ONE_REG ids.  I'm at linux.conf.au this week, so I don't expect to
finish that off until next week.  There are some uglies in that that I
still need to see if can be resolved yet.

Patch

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 953146e..6cb79fe 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1319,6 +1319,7 @@  static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
 #define SPR_MPC_CMPH          (0x09B)
 #define SPR_MPC_LCTRL1        (0x09C)
 #define SPR_MPC_LCTRL2        (0x09D)
+#define SPR_UAMOR             (0x09D)
 #define SPR_MPC_ICTRL         (0x09E)
 #define SPR_MPC_BAR           (0x09F)
 #define SPR_VRSAVE            (0x100)
@@ -1535,6 +1536,7 @@  static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
 #define SPR_UPERF0            (0x310)
 #define SPR_UPERF1            (0x311)
 #define SPR_UPERF2            (0x312)
+#define SPR_MMCRA             (0x312)
 #define SPR_UPERF3            (0x313)
 #define SPR_620_PMC1W         (0x313)
 #define SPR_UPERF4            (0x314)
@@ -1544,7 +1546,9 @@  static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
 #define SPR_UPERF7            (0x317)
 #define SPR_UPERF8            (0x318)
 #define SPR_UPERF9            (0x319)
+#define SPR_PMC7              (0x319)
 #define SPR_UPERFA            (0x31A)
+#define SPR_PMC8              (0x31A)
 #define SPR_UPERFB            (0x31B)
 #define SPR_620_MMCR0W        (0x31B)
 #define SPR_UPERFC            (0x31C)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 2f4f068..7604d0b 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -61,6 +61,7 @@  static int cap_ppc_smt;
 static int cap_ppc_rma;
 static int cap_spapr_tce;
 static int cap_hior;
+static int cap_one_reg;
 
 /* 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
@@ -89,6 +90,7 @@  int kvm_arch_init(KVMState *s)
     cap_ppc_smt = kvm_check_extension(s, KVM_CAP_PPC_SMT);
     cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
     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);
 
     if (!cap_interrupt_level) {
@@ -444,6 +446,76 @@  static void kvm_sw_tlb_put(PowerPCCPU *cpu)
     g_free(bitmap);
 }
 
+static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    union {
+        uint32_t u32;
+        uint64_t u64;
+    } val;
+    struct kvm_one_reg reg = {
+        .id = id,
+        .addr = (uintptr_t) &val,
+    };
+    int ret;
+
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret != 0) {
+        fprintf(stderr, "Warning: Unable to retrieve SPR %d from KVM: %s\n",
+                spr, strerror(errno));
+    } else {
+        switch (id & KVM_REG_SIZE_MASK) {
+        case KVM_REG_SIZE_U32:
+            env->spr[spr] = val.u32;
+            break;
+
+        case KVM_REG_SIZE_U64:
+            env->spr[spr] = val.u64;
+            break;
+
+        default:
+            /* Don't handle this size yet */
+            abort();
+        }
+    }
+}
+
+static void kvm_put_one_spr(CPUState *cs, uint64_t id, int spr)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    union {
+        uint32_t u32;
+        uint64_t u64;
+    } val;
+    struct kvm_one_reg reg = {
+        .id = id,
+        .addr = (uintptr_t) &val,
+    };
+    int ret;
+
+    switch (id & KVM_REG_SIZE_MASK) {
+    case KVM_REG_SIZE_U32:
+        val.u32 = env->spr[spr];
+        break;
+
+    case KVM_REG_SIZE_U64:
+        val.u64 = env->spr[spr];
+        break;
+
+    default:
+        /* Don't handle this size yet */
+        abort();
+    }
+
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret != 0) {
+        fprintf(stderr, "Warning: Unable to set SPR %d to KVM: %s\n",
+                spr, strerror(errno));
+    }
+}
+
 int kvm_arch_put_registers(CPUState *cs, int level)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
@@ -524,16 +596,35 @@  int kvm_arch_put_registers(CPUState *cs, int level)
         }
     }
 
-    if (cap_hior && (level >= KVM_PUT_RESET_STATE)) {
-        uint64_t hior = env->spr[SPR_HIOR];
-        struct kvm_one_reg reg = {
-            .id = KVM_REG_PPC_HIOR,
-            .addr = (uintptr_t) &hior,
-        };
-
-        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
-        if (ret) {
-            return ret;
+    if (cap_one_reg) {
+        /* We deliberately ignore errors here, for kernels which have
+         * the ONE_REG calls, but don't support the specific
+         * registers.  There's still a good chance things will work,
+         * as long as we don't try to migrate */
+        kvm_put_one_spr(cs, KVM_REG_PPC_DABR, SPR_DABR);
+        kvm_put_one_spr(cs, KVM_REG_PPC_DSCR, SPR_DSCR);
+        kvm_put_one_spr(cs, KVM_REG_PPC_PURR, SPR_PURR);
+        kvm_put_one_spr(cs, KVM_REG_PPC_SPURR, SPR_SPURR);
+        kvm_put_one_spr(cs, KVM_REG_PPC_DAR, SPR_DAR);
+        kvm_put_one_spr(cs, KVM_REG_PPC_DSISR, SPR_DSISR);
+        kvm_put_one_spr(cs, KVM_REG_PPC_AMR, SPR_AMR);
+        kvm_put_one_spr(cs, KVM_REG_PPC_UAMOR, SPR_UAMOR);
+
+        kvm_put_one_spr(cs, KVM_REG_PPC_MMCR0, SPR_MMCR0);
+        kvm_put_one_spr(cs, KVM_REG_PPC_MMCR1, SPR_MMCR1);
+        kvm_put_one_spr(cs, KVM_REG_PPC_MMCRA, SPR_MMCRA);
+
+        kvm_put_one_spr(cs, KVM_REG_PPC_PMC1, SPR_PMC1);
+        kvm_put_one_spr(cs, KVM_REG_PPC_PMC2, SPR_PMC2);
+        kvm_put_one_spr(cs, KVM_REG_PPC_PMC3, SPR_PMC3);
+        kvm_put_one_spr(cs, KVM_REG_PPC_PMC4, SPR_PMC4);
+        kvm_put_one_spr(cs, KVM_REG_PPC_PMC5, SPR_PMC5);
+        kvm_put_one_spr(cs, KVM_REG_PPC_PMC6, SPR_PMC6);
+        kvm_put_one_spr(cs, KVM_REG_PPC_PMC7, SPR_PMC7);
+        kvm_put_one_spr(cs, KVM_REG_PPC_PMC8, SPR_PMC8);
+
+        if (cap_hior && (level >= KVM_PUT_RESET_STATE)) {
+            kvm_put_one_spr(cs, KVM_REG_PPC_HIOR, SPR_HIOR);
         }
     }
 
@@ -716,6 +807,37 @@  int kvm_arch_get_registers(CPUState *cs)
         }
     }
 
+    if (cap_one_reg) {
+        /* We deliberately ignore errors here, for kernels which have
+         * the ONE_REG calls, but don't support the specific
+         * registers.  There's still a good chance things will work,
+         * as long as we don't try to migrate */
+        if (cap_hior) {
+            kvm_get_one_spr(cs, KVM_REG_PPC_HIOR, SPR_HIOR);
+        }
+        kvm_get_one_spr(cs, KVM_REG_PPC_DABR, SPR_DABR);
+        kvm_get_one_spr(cs, KVM_REG_PPC_DSCR, SPR_DSCR);
+        kvm_get_one_spr(cs, KVM_REG_PPC_PURR, SPR_PURR);
+        kvm_get_one_spr(cs, KVM_REG_PPC_SPURR, SPR_SPURR);
+        kvm_get_one_spr(cs, KVM_REG_PPC_DAR, SPR_DAR);
+        kvm_get_one_spr(cs, KVM_REG_PPC_DSISR, SPR_DSISR);
+        kvm_get_one_spr(cs, KVM_REG_PPC_AMR, SPR_AMR);
+        kvm_get_one_spr(cs, KVM_REG_PPC_UAMOR, SPR_UAMOR);
+
+        kvm_get_one_spr(cs, KVM_REG_PPC_MMCR0, SPR_MMCR0);
+        kvm_get_one_spr(cs, KVM_REG_PPC_MMCR1, SPR_MMCR1);
+        kvm_get_one_spr(cs, KVM_REG_PPC_MMCRA, SPR_MMCRA);
+
+        kvm_get_one_spr(cs, KVM_REG_PPC_PMC1, SPR_PMC1);
+        kvm_get_one_spr(cs, KVM_REG_PPC_PMC2, SPR_PMC2);
+        kvm_get_one_spr(cs, KVM_REG_PPC_PMC3, SPR_PMC3);
+        kvm_get_one_spr(cs, KVM_REG_PPC_PMC4, SPR_PMC4);
+        kvm_get_one_spr(cs, KVM_REG_PPC_PMC5, SPR_PMC5);
+        kvm_get_one_spr(cs, KVM_REG_PPC_PMC6, SPR_PMC6);
+        kvm_get_one_spr(cs, KVM_REG_PPC_PMC7, SPR_PMC7);
+        kvm_get_one_spr(cs, KVM_REG_PPC_PMC8, SPR_PMC8);
+    }
+
     return 0;
 }