| Submitter | David Gibson |
|---|---|
| Date | Jan. 23, 2013, 5:05 a.m. |
| Message ID | <1358917501-1897-5-git-send-email-david@gibson.dropbear.id.au> |
| Download | mbox | patch |
| Permalink | /patch/214789/ |
| State | New |
| Headers | show |
Comments
On 01/22/2013 11:05:00 PM, David Gibson wrote: > Currently qemu does not get and put the state of the floating point > and > vector registers to KVM. This is obviously a problem for savevm, as > well > as possibly being problematic for debugging of FP-using guests. > > This patch fixes this by using new extensions to the ONE_REG > interface to > synchronize the qemu floating point state with KVM. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > target-ppc/kvm.c | 123 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 123 insertions(+) > > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > index e84b993..7ed76be 100644 > --- a/target-ppc/kvm.c > +++ b/target-ppc/kvm.c > @@ -518,6 +518,125 @@ static void kvm_put_one_spr(CPUState *cs, > uint64_t id, int spr) > } > } > > +static void kvm_put_fp(CPUState *cs) > +{ > + PowerPCCPU *cpu = POWERPC_CPU(cs); > + CPUPPCState *env = &cpu->env; > + struct kvm_one_reg reg; > + int i; > + int ret; > + > + if (env->insns_flags & PPC_FLOAT) { > + uint64_t fpscr = env->fpscr; > + bool vsx = !!(env->insns_flags2 & PPC2_VSX); > + > + reg.id = KVM_REG_PPC_FPSCR; > + reg.addr = (uintptr_t)&fpscr; > + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > + if (ret < 0) { > + fprintf(stderr, "Warning: Unable to set FPSCR to KVM: > %s\n", > + strerror(errno)); > + } > + for (i = 0; i < 32; i++) { > + uint64_t vsr[2]; > + > + vsr[0] = float64_val(env->fpr[i]); > + vsr[1] = env->vsr[i]; > + reg.addr = (uintptr_t) &vsr; > + reg.id = vsx ? KVM_REG_PPC_VSR(i) : KVM_REG_PPC_FPR(i); > + > + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > + if (ret < 0) { > + fprintf(stderr, "Warning Unable to set %s%d to KVM: > %s\n", > + vsx ? "VSR" : "FPR", i, strerror(errno)); > + } > + } > + } I don't see where you check a capability, so users of pre-one-reg KVM now get (at least) 33 warning messages every time context gets synchronized? -Scott
On Wed, Jan 23, 2013 at 11:56:04AM -0600, Scott Wood wrote: > On 01/22/2013 11:05:00 PM, David Gibson wrote: > >Currently qemu does not get and put the state of the floating > >point and > >vector registers to KVM. This is obviously a problem for savevm, > >as well > >as possibly being problematic for debugging of FP-using guests. > > > >This patch fixes this by using new extensions to the ONE_REG > >interface to > >synchronize the qemu floating point state with KVM. > > > >Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > >--- > > target-ppc/kvm.c | 123 > >++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 123 insertions(+) > > > >diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > >index e84b993..7ed76be 100644 > >--- a/target-ppc/kvm.c > >+++ b/target-ppc/kvm.c > >@@ -518,6 +518,125 @@ static void kvm_put_one_spr(CPUState *cs, > >uint64_t id, int spr) > > } > > } > > > >+static void kvm_put_fp(CPUState *cs) > >+{ > >+ PowerPCCPU *cpu = POWERPC_CPU(cs); > >+ CPUPPCState *env = &cpu->env; > >+ struct kvm_one_reg reg; > >+ int i; > >+ int ret; > >+ > >+ if (env->insns_flags & PPC_FLOAT) { > >+ uint64_t fpscr = env->fpscr; > >+ bool vsx = !!(env->insns_flags2 & PPC2_VSX); > >+ > >+ reg.id = KVM_REG_PPC_FPSCR; > >+ reg.addr = (uintptr_t)&fpscr; > >+ ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > >+ if (ret < 0) { > >+ fprintf(stderr, "Warning: Unable to set FPSCR to KVM: > >%s\n", > >+ strerror(errno)); > >+ } > >+ for (i = 0; i < 32; i++) { > >+ uint64_t vsr[2]; > >+ > >+ vsr[0] = float64_val(env->fpr[i]); > >+ vsr[1] = env->vsr[i]; > >+ reg.addr = (uintptr_t) &vsr; > >+ reg.id = vsx ? KVM_REG_PPC_VSR(i) : KVM_REG_PPC_FPR(i); > >+ > >+ ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > >+ if (ret < 0) { > >+ fprintf(stderr, "Warning Unable to set %s%d to > >KVM: %s\n", > >+ vsx ? "VSR" : "FPR", i, strerror(errno)); > >+ } > >+ } > >+ } > > I don't see where you check a capability, so users of pre-one-reg > KVM now get (at least) 33 warning messages every time context gets > synchronized? Given that a bunch of important state is missing from that synchronization, I think a slew of warnings is entirely appropriate. There are no per-register capabilities so there's nothing we can check here.
On 01/23/2013 04:05:56 PM, David Gibson wrote: > On Wed, Jan 23, 2013 at 11:56:04AM -0600, Scott Wood wrote: > > I don't see where you check a capability, so users of pre-one-reg > > KVM now get (at least) 33 warning messages every time context gets > > synchronized? > > Given that a bunch of important state is missing from that > synchronization, I think a slew of warnings is entirely appropriate. > There are no per-register capabilities so there's nothing we can check > here. "Important" is debatable. It should be obvious enough for debugging to see that the FP state is all zeroes and conclude that something's missing. Maybe print one warning on the first failure. As for savevm, those old kernels won't support migration anyway on PPC, right? -Scott
On 23 January 2013 22:05, David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Jan 23, 2013 at 11:56:04AM -0600, Scott Wood wrote: >> I don't see where you check a capability, so users of pre-one-reg >> KVM now get (at least) 33 warning messages every time context gets >> synchronized? > > Given that a bunch of important state is missing from that > synchronization, I think a slew of warnings is entirely appropriate. > There are no per-register capabilities so there's nothing we can check > here. FWIW, I think the correct way to detect whether KVM_GET/SET_ONE_REG supports a particular register or set of registers is to use KVM_GET_REG_LIST. I don't know if PPC KVM implements that ioctl yet, though (it may be new with ARM KVM's extensive use of GET/SET_ONE_REG). -- PMM
On 23.01.2013, at 23:16, Peter Maydell wrote: > On 23 January 2013 22:05, David Gibson <david@gibson.dropbear.id.au> wrote: >> On Wed, Jan 23, 2013 at 11:56:04AM -0600, Scott Wood wrote: >>> I don't see where you check a capability, so users of pre-one-reg >>> KVM now get (at least) 33 warning messages every time context gets >>> synchronized? >> >> Given that a bunch of important state is missing from that >> synchronization, I think a slew of warnings is entirely appropriate. >> There are no per-register capabilities so there's nothing we can check >> here. > > FWIW, I think the correct way to detect whether KVM_GET/SET_ONE_REG > supports a particular register or set of registers is to use > KVM_GET_REG_LIST. I don't know if PPC KVM implements that ioctl yet, > though (it may be new with ARM KVM's extensive use of GET/SET_ONE_REG). No, we don't support it yet. The idea is that you poke GET/SET_ONE_REG and if that returns -EINVAL, you know that it's not supported. I figured back then that this is enough of a test if you know the REGs you want to poke. Alex
Patch
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index e84b993..7ed76be 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -518,6 +518,125 @@ static void kvm_put_one_spr(CPUState *cs, uint64_t id, int spr) } } +static void kvm_put_fp(CPUState *cs) +{ + PowerPCCPU *cpu = POWERPC_CPU(cs); + CPUPPCState *env = &cpu->env; + struct kvm_one_reg reg; + int i; + int ret; + + if (env->insns_flags & PPC_FLOAT) { + uint64_t fpscr = env->fpscr; + bool vsx = !!(env->insns_flags2 & PPC2_VSX); + + reg.id = KVM_REG_PPC_FPSCR; + reg.addr = (uintptr_t)&fpscr; + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); + if (ret < 0) { + fprintf(stderr, "Warning: Unable to set FPSCR to KVM: %s\n", + strerror(errno)); + } + + for (i = 0; i < 32; i++) { + uint64_t vsr[2]; + + vsr[0] = float64_val(env->fpr[i]); + vsr[1] = env->vsr[i]; + reg.addr = (uintptr_t) &vsr; + reg.id = vsx ? KVM_REG_PPC_VSR(i) : KVM_REG_PPC_FPR(i); + + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); + if (ret < 0) { + fprintf(stderr, "Warning Unable to set %s%d to KVM: %s\n", + vsx ? "VSR" : "FPR", i, strerror(errno)); + } + } + } + + if (env->insns_flags & PPC_ALTIVEC) { + reg.id = KVM_REG_PPC_VSCR; + reg.addr = (uintptr_t)&env->vscr; + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); + if (ret < 0) { + fprintf(stderr, "Warning: Unable to set VSCR to KVM: %s\n", + strerror(errno)); + } + + for (i = 0; i < 32; i++) { + reg.id = KVM_REG_PPC_VR(i); + reg.addr = (uintptr_t)&env->avr[i]; + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); + if (ret < 0) { + fprintf(stderr, "Warning Unable to set VR%d to KVM: %s\n", + i, strerror(errno)); + } + } + } +} + +static void kvm_get_fp(CPUState *cs) +{ + PowerPCCPU *cpu = POWERPC_CPU(cs); + CPUPPCState *env = &cpu->env; + struct kvm_one_reg reg; + int i; + int ret; + + if (env->insns_flags & PPC_FLOAT) { + uint64_t fpscr; + bool vsx = !!(env->insns_flags2 & PPC2_VSX); + + reg.id = KVM_REG_PPC_FPSCR; + reg.addr = (uintptr_t)&fpscr; + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); + if (ret < 0) { + fprintf(stderr, "Warning: Unable to get FPSCR from KVM: %s\n", + strerror(errno)); + } else { + env->fpscr = fpscr; + } + + for (i = 0; i < 32; i++) { + uint64_t vsr[2]; + + reg.addr = (uintptr_t) &vsr; + reg.id = vsx ? KVM_REG_PPC_VSR(i) : KVM_REG_PPC_FPR(i); + + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); + if (ret < 0) { + fprintf(stderr, "Warning Unable to get %s%d from KVM: %s\n", + vsx ? "VSR" : "FPR", i, strerror(errno)); + } else { + env->fpr[i] = vsr[0]; + if (vsx) { + env->vsr[i] = vsr[1]; + } + } + } + } + + if (env->insns_flags & PPC_ALTIVEC) { + reg.id = KVM_REG_PPC_VSCR; + reg.addr = (uintptr_t)&env->vscr; + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); + if (ret < 0) { + fprintf(stderr, "Warning: Unable to get VSCR from KVM: %s\n", + strerror(errno)); + } + + for (i = 0; i < 32; i++) { + reg.id = KVM_REG_PPC_VR(i); + reg.addr = (uintptr_t)&env->avr[i]; + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); + if (ret < 0) { + fprintf(stderr, "Warning Unable to get VR%d from KVM: %s\n", + i, strerror(errno)); + } + } + } +} + int kvm_arch_put_registers(CPUState *cs, int level) { PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -558,6 +677,8 @@ int kvm_arch_put_registers(CPUState *cs, int level) if (ret < 0) return ret; + kvm_put_fp(cs); + if (env->tlb_dirty) { kvm_sw_tlb_put(cpu); env->tlb_dirty = false; @@ -675,6 +796,8 @@ int kvm_arch_get_registers(CPUState *cs) for (i = 0;i < 32; i++) env->gpr[i] = regs.gpr[i]; + kvm_get_fp(cs); + if (cap_booke_sregs) { ret = kvm_vcpu_ioctl(cs, KVM_GET_SREGS, &sregs); if (ret < 0) {
Currently qemu does not get and put the state of the floating point and vector registers to KVM. This is obviously a problem for savevm, as well as possibly being problematic for debugging of FP-using guests. This patch fixes this by using new extensions to the ONE_REG interface to synchronize the qemu floating point state with KVM. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- target-ppc/kvm.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+)