Patchwork [4/5] target-ppc: Synchronize FPU state with KVM

login
register
mail settings
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

David Gibson - Jan. 23, 2013, 5:05 a.m.
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(+)
Scott Wood - Jan. 23, 2013, 5:56 p.m.
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, &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, &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
David Gibson - Jan. 23, 2013, 10:05 p.m.
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, &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, &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.
Scott Wood - Jan. 23, 2013, 10:12 p.m.
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
Peter Maydell - Jan. 23, 2013, 10:16 p.m.
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
Alexander Graf - Jan. 23, 2013, 11:57 p.m.
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, &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, &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, &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, &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, &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, &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, &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, &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) {