| Message ID | 1358917501-1897-4-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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
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
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, ®); + 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, ®); + 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, ®); - 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; }
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(-)