Patchwork [2/7] ppc: Make kvm_arch_put_registers() put *all* the registers

login
register
mail settings
Submitter David Gibson
Date Aug. 15, 2012, 4:33 a.m.
Message ID <1345005228-4380-3-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/177543/
State New
Headers show

Comments

David Gibson - Aug. 15, 2012, 4:33 a.m.
At least when invoked with high enough 'level' arguments,
kvm_arch_put_registers() is supposed to copy essentially all the cpu state
as encoded in qemu's internal structures into the kvm state.  Currently
the ppc version does not do this - it never calls KVM_SET_SREGS, for
example, and therefore never sets the SDR1 and various other important
though rarely changed registers.

Instead, the code paths which need to set these registers need to
explicitly make (conditional) kvm calls which transfer the changes to kvm.
This breaks the usual model of handling state updates in qemu, where code
just changes the internal model and has it flushed out to kvm automatically
at some later point.

This patch fixes this for Book S ppc CPUs by adding a suitable call to
KVM_SET_SREGS and als to KVM_SET_ONE_REG to set the HIOR (the only register
that is set with that call so far).  This lets us remove the hacks to
explicitly set these registers from the kvmppc_set_papr() function.

The problem still exists for Book E CPUs (which use a different version of
the kvm_sregs structure).  But fixing that has some complications of its
own so can be left to another day.

Lkewise, there is still some ugly code for setting the PVR through special
calls to SET_SREGS which is left in for now.  The PVR needs to be set
especially early because it can affect what other features are available
on the CPU, so I need to do more thinking to see if it can be integrated
into the normal paths or not.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target-ppc/kvm.c |   89 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 50 insertions(+), 39 deletions(-)
Alexander Graf - Aug. 17, 2012, 1:58 p.m.
On 08/15/2012 06:33 AM, David Gibson wrote:
> At least when invoked with high enough 'level' arguments,
> kvm_arch_put_registers() is supposed to copy essentially all the cpu state
> as encoded in qemu's internal structures into the kvm state.  Currently
> the ppc version does not do this - it never calls KVM_SET_SREGS, for
> example, and therefore never sets the SDR1 and various other important
> though rarely changed registers.
>
> Instead, the code paths which need to set these registers need to
> explicitly make (conditional) kvm calls which transfer the changes to kvm.
> This breaks the usual model of handling state updates in qemu, where code
> just changes the internal model and has it flushed out to kvm automatically
> at some later point.
>
> This patch fixes this for Book S ppc CPUs by adding a suitable call to
> KVM_SET_SREGS and als to KVM_SET_ONE_REG to set the HIOR (the only register
> that is set with that call so far).  This lets us remove the hacks to
> explicitly set these registers from the kvmppc_set_papr() function.

HIOR is a read-only register from the guest's point of view when running 
in PAPR mode, so we don't need to sync it back again. The same goes for 
SDR1, though resetting that is valid for non-PAPR guests.

Overall, does a normal system reset on PPC guarantee that the SRs and 
SLBs are reset? At least OpenBIOS boots up in real mode and overwrites 
all SR/SLB entries while still in real mode.


Alex

>
> The problem still exists for Book E CPUs (which use a different version of
> the kvm_sregs structure).  But fixing that has some complications of its
> own so can be left to another day.
>
> Lkewise, there is still some ugly code for setting the PVR through special
> calls to SET_SREGS which is left in for now.  The PVR needs to be set
> especially early because it can affect what other features are available
> on the CPU, so I need to do more thinking to see if it can be integrated
> into the normal paths or not.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>   target-ppc/kvm.c |   89 ++++++++++++++++++++++++++++++------------------------
>   1 file changed, 50 insertions(+), 39 deletions(-)
>
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index a31d278..1a7489b 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -60,6 +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;
>   
>   /* 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
> @@ -86,6 +87,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_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
>   
>       if (!cap_interrupt_level) {
>           fprintf(stderr, "KVM: Couldn't find level irq capability. Expect the "
> @@ -469,6 +471,53 @@ int kvm_arch_put_registers(CPUPPCState *env, int level)
>           env->tlb_dirty = false;
>       }
>   
> +    if (cap_segstate && (level >= KVM_PUT_RESET_STATE)) {
> +        struct kvm_sregs sregs;
> +
> +        sregs.pvr = env->spr[SPR_PVR];
> +
> +        sregs.u.s.sdr1 = env->spr[SPR_SDR1];
> +
> +        /* Sync SLB */
> +#ifdef TARGET_PPC64
> +        for (i = 0; i < 64; i++) {
> +            sregs.u.s.ppc64.slb[i].slbe = env->slb[i].esid;
> +            sregs.u.s.ppc64.slb[i].slbv = env->slb[i].vsid;
> +        }
> +#endif
> +
> +        /* Sync SRs */
> +        for (i = 0; i < 16; i++) {
> +            sregs.u.s.ppc32.sr[i] = env->sr[i];
> +        }
> +
> +        /* Sync BATs */
> +        for (i = 0; i < 8; i++) {
> +            sregs.u.s.ppc32.dbat[i] = ((uint64_t)env->DBAT[1][i] << 32)
> +                | env->DBAT[0][i];
> +            sregs.u.s.ppc32.ibat[i] = ((uint64_t)env->IBAT[1][i] << 32)
> +                | env->IBAT[0][i];
> +        }
> +
> +        ret = kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs);
> +        if (ret) {
> +            return ret;
> +        }
> +    }
> +
> +    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(env, KVM_SET_ONE_REG, &reg);
> +        if (ret) {
> +            return ret;
> +        }
> +    }
> +
>       return ret;
>   }
>   
> @@ -946,52 +995,14 @@ int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len)
>   void kvmppc_set_papr(CPUPPCState *env)
>   {
>       struct kvm_enable_cap cap = {};
> -    struct kvm_one_reg reg = {};
> -    struct kvm_sregs sregs = {};
>       int ret;
> -    uint64_t hior = env->spr[SPR_HIOR];
>   
>       cap.cap = KVM_CAP_PPC_PAPR;
>       ret = kvm_vcpu_ioctl(env, KVM_ENABLE_CAP, &cap);
>   
>       if (ret) {
> -        goto fail;
> -    }
> -
> -    /*
> -     * XXX We set HIOR here. It really should be a qdev property of
> -     *     the CPU node, but we don't have CPUs converted to qdev yet.
> -     *
> -     *     Once we have qdev CPUs, move HIOR to a qdev property and
> -     *     remove this chunk.
> -     */
> -    reg.id = KVM_REG_PPC_HIOR;
> -    reg.addr = (uintptr_t)&hior;
> -    ret = kvm_vcpu_ioctl(env, KVM_SET_ONE_REG, &reg);
> -    if (ret) {
> -        fprintf(stderr, "Couldn't set HIOR. Maybe you're running an old \n"
> -                        "kernel with support for HV KVM but no PAPR PR \n"
> -                        "KVM in which case things will work. If they don't \n"
> -                        "please update your host kernel!\n");
> -    }
> -
> -    /* Set SDR1 so kernel space finds the HTAB */
> -    ret = kvm_vcpu_ioctl(env, KVM_GET_SREGS, &sregs);
> -    if (ret) {
> -        goto fail;
> -    }
> -
> -    sregs.u.s.sdr1 = env->spr[SPR_SDR1];
> -
> -    ret = kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs);
> -    if (ret) {
> -        goto fail;
> +        cpu_abort(env, "This KVM version does not support PAPR\n");
>       }
> -
> -    return;
> -
> -fail:
> -    cpu_abort(env, "This KVM version does not support PAPR\n");
>   }
>   
>   int kvmppc_smt_threads(void)
David Gibson - Aug. 17, 2012, 4:24 p.m.
On Fri, Aug 17, 2012 at 03:58:08PM +0200, Alexander Graf wrote:
> On 08/15/2012 06:33 AM, David Gibson wrote:
> >At least when invoked with high enough 'level' arguments,
> >kvm_arch_put_registers() is supposed to copy essentially all the cpu state
> >as encoded in qemu's internal structures into the kvm state.  Currently
> >the ppc version does not do this - it never calls KVM_SET_SREGS, for
> >example, and therefore never sets the SDR1 and various other important
> >though rarely changed registers.
> >
> >Instead, the code paths which need to set these registers need to
> >explicitly make (conditional) kvm calls which transfer the changes to kvm.
> >This breaks the usual model of handling state updates in qemu, where code
> >just changes the internal model and has it flushed out to kvm automatically
> >at some later point.
> >
> >This patch fixes this for Book S ppc CPUs by adding a suitable call to
> >KVM_SET_SREGS and als to KVM_SET_ONE_REG to set the HIOR (the only register
> >that is set with that call so far).  This lets us remove the hacks to
> >explicitly set these registers from the kvmppc_set_papr() function.
> 
> HIOR is a read-only register from the guest's point of view when
> running in PAPR mode, so we don't need to sync it back again. The
> same goes for SDR1, though resetting that is valid for non-PAPR
> guests.

*When running in PAPR mode*, which we aren't always.  System resets
are so rare that there's really no point optimizing register sets out
of it, so we might as well set HIOR and SDR1 on every reset, then it's
correct both for PAPR and non-PAPR mode.

> Overall, does a normal system reset on PPC guarantee that the SRs
> and SLBs are reset? At least OpenBIOS boots up in real mode and
> overwrites all SR/SLB entries while still in real mode.

I don't know, but it's not really relevant here.  What this does is
make sure that KVM state is synced with qemu state on reset.  That
means that whatever reset handlers do to the qemu state - and that's
what they usually operate on - will get reflected in KVM state when
the guest executes again.

Patch

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index a31d278..1a7489b 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -60,6 +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;
 
 /* 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
@@ -86,6 +87,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_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
 
     if (!cap_interrupt_level) {
         fprintf(stderr, "KVM: Couldn't find level irq capability. Expect the "
@@ -469,6 +471,53 @@  int kvm_arch_put_registers(CPUPPCState *env, int level)
         env->tlb_dirty = false;
     }
 
+    if (cap_segstate && (level >= KVM_PUT_RESET_STATE)) {
+        struct kvm_sregs sregs;
+
+        sregs.pvr = env->spr[SPR_PVR];
+
+        sregs.u.s.sdr1 = env->spr[SPR_SDR1];
+
+        /* Sync SLB */
+#ifdef TARGET_PPC64
+        for (i = 0; i < 64; i++) {
+            sregs.u.s.ppc64.slb[i].slbe = env->slb[i].esid;
+            sregs.u.s.ppc64.slb[i].slbv = env->slb[i].vsid;
+        }
+#endif
+
+        /* Sync SRs */
+        for (i = 0; i < 16; i++) {
+            sregs.u.s.ppc32.sr[i] = env->sr[i];
+        }
+
+        /* Sync BATs */
+        for (i = 0; i < 8; i++) {
+            sregs.u.s.ppc32.dbat[i] = ((uint64_t)env->DBAT[1][i] << 32)
+                | env->DBAT[0][i];
+            sregs.u.s.ppc32.ibat[i] = ((uint64_t)env->IBAT[1][i] << 32)
+                | env->IBAT[0][i];
+        }
+
+        ret = kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs);
+        if (ret) {
+            return ret;
+        }
+    }
+
+    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(env, KVM_SET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        }
+    }
+
     return ret;
 }
 
@@ -946,52 +995,14 @@  int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len)
 void kvmppc_set_papr(CPUPPCState *env)
 {
     struct kvm_enable_cap cap = {};
-    struct kvm_one_reg reg = {};
-    struct kvm_sregs sregs = {};
     int ret;
-    uint64_t hior = env->spr[SPR_HIOR];
 
     cap.cap = KVM_CAP_PPC_PAPR;
     ret = kvm_vcpu_ioctl(env, KVM_ENABLE_CAP, &cap);
 
     if (ret) {
-        goto fail;
-    }
-
-    /*
-     * XXX We set HIOR here. It really should be a qdev property of
-     *     the CPU node, but we don't have CPUs converted to qdev yet.
-     *
-     *     Once we have qdev CPUs, move HIOR to a qdev property and
-     *     remove this chunk.
-     */
-    reg.id = KVM_REG_PPC_HIOR;
-    reg.addr = (uintptr_t)&hior;
-    ret = kvm_vcpu_ioctl(env, KVM_SET_ONE_REG, &reg);
-    if (ret) {
-        fprintf(stderr, "Couldn't set HIOR. Maybe you're running an old \n"
-                        "kernel with support for HV KVM but no PAPR PR \n"
-                        "KVM in which case things will work. If they don't \n"
-                        "please update your host kernel!\n");
-    }
-
-    /* Set SDR1 so kernel space finds the HTAB */
-    ret = kvm_vcpu_ioctl(env, KVM_GET_SREGS, &sregs);
-    if (ret) {
-        goto fail;
-    }
-
-    sregs.u.s.sdr1 = env->spr[SPR_SDR1];
-
-    ret = kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs);
-    if (ret) {
-        goto fail;
+        cpu_abort(env, "This KVM version does not support PAPR\n");
     }
-
-    return;
-
-fail:
-    cpu_abort(env, "This KVM version does not support PAPR\n");
 }
 
 int kvmppc_smt_threads(void)