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

login
register
mail settings
Submitter David Gibson
Date Jan. 23, 2013, 5:04 a.m.
Message ID <1358917501-1897-4-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/214790/
State New
Headers show

Comments

David Gibson - Jan. 23, 2013, 5:04 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 |  146 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 138 insertions(+), 12 deletions(-)
Scott Wood - Jan. 23, 2013, 5:52 p.m.
On 01/22/2013 11:04:59 PM, David Gibson wrote:
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 2f4f068..e84b993 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -60,7 +60,7 @@ static int cap_booke_sregs;
>  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,7 +89,11 @@ 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_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
> +    /* This capability is misnamed - it was introduced with the
> +     * KVM_SET_ONE_REG ioctl(), which at the time only supported the
> +     * HIOR.  We don't want a different capability for every register
> +     * the interface can support though. */
> +    cap_one_reg = kvm_check_extension(s, KVM_CAP_PPC_HIOR);

So what happens when we want to use onereg for booke, which doesn't  
have KVM_CAP_PPC_HIOR?  Into what variable would we put a check for  
KVM_CAP_ONE_REG?

IMHO this should stay as cap_hior and merge the above comment with the  
comment where you check cap_hior, regarding not all registers  
necessarily being supported.

-Scott
David Gibson - Jan. 23, 2013, 10:41 p.m.
On Wed, Jan 23, 2013 at 11:52:54AM -0600, Scott Wood wrote:
> On 01/22/2013 11:04:59 PM, David Gibson wrote:
> >diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> >index 2f4f068..e84b993 100644
> >--- a/target-ppc/kvm.c
> >+++ b/target-ppc/kvm.c
> >@@ -60,7 +60,7 @@ static int cap_booke_sregs;
> > 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,7 +89,11 @@ 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_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
> >+    /* This capability is misnamed - it was introduced with the
> >+     * KVM_SET_ONE_REG ioctl(), which at the time only supported the
> >+     * HIOR.  We don't want a different capability for every register
> >+     * the interface can support though. */
> >+    cap_one_reg = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
> 
> So what happens when we want to use onereg for booke, which doesn't
> have KVM_CAP_PPC_HIOR?  Into what variable would we put a check for
> KVM_CAP_ONE_REG?

Drat, good point.  There is no KVM_CAP_ONE_REG, that's the problem.  I
guess I'll have to leave cap_hior as it is, and just not have a
capability check.

> IMHO this should stay as cap_hior and merge the above comment with
> the comment where you check cap_hior, regarding not all registers
> necessarily being supported.

I'm not sure quite what you mean by this.
Scott Wood - Jan. 23, 2013, 11:20 p.m.
On 01/23/2013 04:41:27 PM, David Gibson wrote:
> On Wed, Jan 23, 2013 at 11:52:54AM -0600, Scott Wood wrote:
> > On 01/22/2013 11:04:59 PM, David Gibson wrote:
> > >-    cap_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
> > >+    /* This capability is misnamed - it was introduced with the
> > >+     * KVM_SET_ONE_REG ioctl(), which at the time only supported  
> the
> > >+     * HIOR.  We don't want a different capability for every  
> register
> > >+     * the interface can support though. */
> > >+    cap_one_reg = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
> >
> > So what happens when we want to use onereg for booke, which doesn't
> > have KVM_CAP_PPC_HIOR?  Into what variable would we put a check for
> > KVM_CAP_ONE_REG?
> 
> Drat, good point.  There is no KVM_CAP_ONE_REG, that's the problem.  I
> guess I'll have to leave cap_hior as it is, and just not have a
> capability check.

Hmm?  There is a KVM_CAP_ONE_REG.  Its value is 70.  It was introduced  
in Linux commit e24ed81fedd551e80378be62fa0b0532480ea7d4, at the same  
time as the ONE_REG ioctls themselves.

> > IMHO this should stay as cap_hior and merge the above comment with
> > the comment where you check cap_hior, regarding not all registers
> > necessarily being supported.
> 
> I'm not sure quite what you mean by this.

Later on you say that you don't check for failure when accessing those  
registers -- that seems to be the place for a comment about not wanting  
to check a different capability for each one.

-Scott
Alexander Graf - Jan. 23, 2013, 11:55 p.m.
On 23.01.2013, at 23:41, David Gibson wrote:

> On Wed, Jan 23, 2013 at 11:52:54AM -0600, Scott Wood wrote:
>> On 01/22/2013 11:04:59 PM, David Gibson wrote:
>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>> index 2f4f068..e84b993 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -60,7 +60,7 @@ static int cap_booke_sregs;
>>> 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,7 +89,11 @@ 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_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
>>> +    /* This capability is misnamed - it was introduced with the
>>> +     * KVM_SET_ONE_REG ioctl(), which at the time only supported the
>>> +     * HIOR.  We don't want a different capability for every register
>>> +     * the interface can support though. */
>>> +    cap_one_reg = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
>> 
>> So what happens when we want to use onereg for booke, which doesn't
>> have KVM_CAP_PPC_HIOR?  Into what variable would we put a check for
>> KVM_CAP_ONE_REG?
> 
> Drat, good point.  There is no KVM_CAP_ONE_REG

Of course there is a KVM_CAP_ONE_REG :).

Alex
David Gibson - Jan. 24, 2013, 12:23 a.m.
On Wed, Jan 23, 2013 at 05:20:43PM -0600, Scott Wood wrote:
> On 01/23/2013 04:41:27 PM, David Gibson wrote:
> >On Wed, Jan 23, 2013 at 11:52:54AM -0600, Scott Wood wrote:
> >> On 01/22/2013 11:04:59 PM, David Gibson wrote:
> >> >-    cap_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
> >> >+    /* This capability is misnamed - it was introduced with the
> >> >+     * KVM_SET_ONE_REG ioctl(), which at the time only
> >supported the
> >> >+     * HIOR.  We don't want a different capability for every
> >register
> >> >+     * the interface can support though. */
> >> >+    cap_one_reg = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
> >>
> >> So what happens when we want to use onereg for booke, which doesn't
> >> have KVM_CAP_PPC_HIOR?  Into what variable would we put a check for
> >> KVM_CAP_ONE_REG?
> >
> >Drat, good point.  There is no KVM_CAP_ONE_REG, that's the problem.  I
> >guess I'll have to leave cap_hior as it is, and just not have a
> >capability check.
> 
> Hmm?  There is a KVM_CAP_ONE_REG.  Its value is 70.  It was
> introduced in Linux commit e24ed81fedd551e80378be62fa0b0532480ea7d4,
> at the same time as the ONE_REG ioctls themselves.

*facepalm* So there is.  I'm an idiot, I don't know how I missed that,
sorry.  I'll rework accordingly.

> >> IMHO this should stay as cap_hior and merge the above comment with
> >> the comment where you check cap_hior, regarding not all registers
> >> necessarily being supported.
> >
> >I'm not sure quite what you mean by this.
> 
> Later on you say that you don't check for failure when accessing
> those registers -- that seems to be the place for a comment about
> not wanting to check a different capability for each one.
> 
> -Scott

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..e84b993 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -60,7 +60,7 @@  static int cap_booke_sregs;
 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,7 +89,11 @@  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_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
+    /* This capability is misnamed - it was introduced with the
+     * KVM_SET_ONE_REG ioctl(), which at the time only supported the
+     * HIOR.  We don't want a different capability for every register
+     * the interface can support though. */
+    cap_one_reg = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
 
     if (!cap_interrupt_level) {
         fprintf(stderr, "KVM: Couldn't find level irq capability. Expect the "
@@ -444,6 +448,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 +598,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 (level >= KVM_PUT_RESET_STATE) {
+            kvm_put_one_spr(cs, KVM_REG_PPC_HIOR, SPR_HIOR);
         }
     }
 
@@ -716,6 +809,35 @@  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 */
+        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;
 }