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

[Alexander Graf]

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 <address@hidden>
>>> ---
>>> 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