Re: [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

2015-03-09 Thread Peter Maydell 
On 9 March 2015 at 21:56, Christoffer Dall christoffer.d...@linaro.org wrote:
 this function, however, is not used only when migration, but should
 generally cover the case where you want to synchronize QEMU's state into
 KVM's state, right?  So while it may not be harmful in currently
 supported use cases, is there ever a situation where (is_a64(env)  el
 == 0) and env-spsr != banked_spsr[el], and where env-spsr is
 out-of-date?

If EL == 0 then you can't access any SPSR, so env-spsr is by
definition out of date.

-- PMM
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

2015-03-09 Thread Christoffer Dall 
On Tue, Mar 03, 2015 at 11:28:21AM +, Alex Bennée wrote:
 
 Christoffer Dall christoffer.d...@linaro.org writes:
 
  Hi Alex,
 
  Seems like you accidentally sent out two copies of this patch, hopefully
  I'm reviewing the right one...
 
 Oops, perils of not wiping your output directory. I think it was just a
 title tweak!
 
  On Wed, Feb 25, 2015 at 04:02:38PM +, Alex Bennée wrote:
  From: Christoffer Dall christoffer.d...@linaro.org
  
  The current code was negatively indexing the cpu state array and not
  synchronizing banked spsr register state with the current mode's spsr
  state, causing occasional failures with migration.
  
  Some munging is done to take care of the aarch64 mapping and also to
  ensure the most current value of the spsr is updated to the banked
  registers (relevant for KVM-TCG migration).
  
  Signed-off-by: Christoffer Dall christoffer.d...@linaro.org
  Signed-off-by: Alex Bennée alex.ben...@linaro.org
  
  ---
  v2 (ajb)
- minor tweaks and clarifications
  
  diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
  index c60e989..1e36b0a 100644
  --- a/target-arm/kvm64.c
  +++ b/target-arm/kvm64.c
  @@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
   uint64_t val;
   int i;
   int ret;
  +unsigned int el;
   
   ARMCPU *cpu = ARM_CPU(cs);
   CPUARMState *env = cpu-env;
  @@ -206,9 +207,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
   return ret;
   }
   
  +/* Saved Program State Registers
  + *
  + * Before we restore from the banked_spsr[] array we need to
  + * ensure that any modifications to env-spsr are correctly
  + * reflected and map aarch64 exception levels if required.
  + */
  +el = arm_current_el(env);
  +if (is_a64(env)  el  0) {
  +g_assert(el == 1);
  +/* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
  +env-banked_spsr[1] = env-banked_spsr[0];
  +env-banked_spsr[aarch64_banked_spsr_index(el)] = env-spsr;
  +} else {
  +env-banked_spsr[el] = env-spsr;
 
  is this valid if (is_a64(env)  el == 0)) ?  I thought that if you're
  in el == 0, then env-banked_spsr[x] is the most up-to-date one, not
  env-spsr ?
 
 Actually they will both be the same (at least for KVM). In the end both:
 
 VMSTATE_UINT32(env.spsr, ARMCPU),
 VMSTATE_UINT64_ARRAY(env.banked_spsr, ARMCPU, 8),
 
 get serialised in the stream and when we save the stream out we make
 sure everything is in sync (i.e. env-spsr is correct). In reality this
 makes more sense for TCG than KVM which is the only reason env-spsr
 exists.
 

this function, however, is not used only when migration, but should
generally cover the case where you want to synchronize QEMU's state into
KVM's state, right?  So while it may not be harmful in currently
supported use cases, is there ever a situation where (is_a64(env)  el
== 0) and env-spsr != banked_spsr[el], and where env-spsr is
out-of-date?

If that's the case, I think it would be better to have an assert that
says don't call the code in this situation than relying on limited use
cases for callers of this function.

 
  for !is_a64(env) this looks wrong, because of the same as above if el ==
  0, but also because I think you need
  bank_number(env-uncached_cpsr  CPSR_M) to index into the array.
 
 
 Good catch. For some reason I was treating the number from
 arm_current_el() as equivalent. How about:
 
 el = arm_current_el(env);
 if (is_a64(env)  el  0) {
 g_assert(el == 1);
 /* KVM only maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 ATM */
 env-banked_spsr[1] = env-banked_spsr[0];
 }
 i = is_a64(env) ?
 aarch64_banked_spsr_index(el) : bank_number(env-unached_cpsr  
 CPSR_M);

I think that will fail due to the assert in aarch64_banked_spsr_index()
for el == 0.


-Christoffer
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

2015-03-09 Thread Alex Bennée 

Peter Maydell peter.mayd...@linaro.org writes:

 On 9 March 2015 at 21:56, Christoffer Dall christoffer.d...@linaro.org 
 wrote:
 this function, however, is not used only when migration, but should
 generally cover the case where you want to synchronize QEMU's state into
 KVM's state, right?  So while it may not be harmful in currently
 supported use cases, is there ever a situation where (is_a64(env)  el
 == 0) and env-spsr != banked_spsr[el], and where env-spsr is
 out-of-date?

 If EL == 0 then you can't access any SPSR, so env-spsr is by
 definition out of date.

Indeed and in v2 the whole thing is wrapped in if (el  0)


 -- PMM

-- 
Alex Bennée
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

2015-03-09 Thread Christoffer Dall 
On Mon, Mar 09, 2015 at 10:31:19PM +0900, Peter Maydell wrote:
 On 9 March 2015 at 21:56, Christoffer Dall christoffer.d...@linaro.org 
 wrote:
  this function, however, is not used only when migration, but should
  generally cover the case where you want to synchronize QEMU's state into
  KVM's state, right?  So while it may not be harmful in currently
  supported use cases, is there ever a situation where (is_a64(env)  el
  == 0) and env-spsr != banked_spsr[el], and where env-spsr is
  out-of-date?
 
 If EL == 0 then you can't access any SPSR, so env-spsr is by
 definition out of date.
 
ok, never mind then.

-Christoffer
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

2015-03-03 Thread Alex Bennée 

Christoffer Dall christoffer.d...@linaro.org writes:

 Hi Alex,

 Seems like you accidentally sent out two copies of this patch, hopefully
 I'm reviewing the right one...

Oops, perils of not wiping your output directory. I think it was just a
title tweak!

 On Wed, Feb 25, 2015 at 04:02:38PM +, Alex Bennée wrote:
 From: Christoffer Dall christoffer.d...@linaro.org
 
 The current code was negatively indexing the cpu state array and not
 synchronizing banked spsr register state with the current mode's spsr
 state, causing occasional failures with migration.
 
 Some munging is done to take care of the aarch64 mapping and also to
 ensure the most current value of the spsr is updated to the banked
 registers (relevant for KVM-TCG migration).
 
 Signed-off-by: Christoffer Dall christoffer.d...@linaro.org
 Signed-off-by: Alex Bennée alex.ben...@linaro.org
 
 ---
 v2 (ajb)
   - minor tweaks and clarifications
 
 diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
 index c60e989..1e36b0a 100644
 --- a/target-arm/kvm64.c
 +++ b/target-arm/kvm64.c
 @@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
  uint64_t val;
  int i;
  int ret;
 +unsigned int el;
  
  ARMCPU *cpu = ARM_CPU(cs);
  CPUARMState *env = cpu-env;
 @@ -206,9 +207,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
  return ret;
  }
  
 +/* Saved Program State Registers
 + *
 + * Before we restore from the banked_spsr[] array we need to
 + * ensure that any modifications to env-spsr are correctly
 + * reflected and map aarch64 exception levels if required.
 + */
 +el = arm_current_el(env);
 +if (is_a64(env)  el  0) {
 +g_assert(el == 1);
 +/* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
 +env-banked_spsr[1] = env-banked_spsr[0];
 +env-banked_spsr[aarch64_banked_spsr_index(el)] = env-spsr;
 +} else {
 +env-banked_spsr[el] = env-spsr;

 is this valid if (is_a64(env)  el == 0)) ?  I thought that if you're
 in el == 0, then env-banked_spsr[x] is the most up-to-date one, not
 env-spsr ?

Actually they will both be the same (at least for KVM). In the end both:

VMSTATE_UINT32(env.spsr, ARMCPU),
VMSTATE_UINT64_ARRAY(env.banked_spsr, ARMCPU, 8),

get serialised in the stream and when we save the stream out we make
sure everything is in sync (i.e. env-spsr is correct). In reality this
makes more sense for TCG than KVM which is the only reason env-spsr
exists.


 for !is_a64(env) this looks wrong, because of the same as above if el ==
 0, but also because I think you need
 bank_number(env-uncached_cpsr  CPSR_M) to index into the array.


Good catch. For some reason I was treating the number from
arm_current_el() as equivalent. How about:

el = arm_current_el(env);
if (is_a64(env)  el  0) {
g_assert(el == 1);
/* KVM only maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 ATM */
env-banked_spsr[1] = env-banked_spsr[0];
}
i = is_a64(env) ?
aarch64_banked_spsr_index(el) : bank_number(env-unached_cpsr  CPSR_M);
env-banked_spsr[i] = env-spsr;


 +}
 +
  for (i = 0; i  KVM_NR_SPSR; i++) {
  reg.id = AARCH64_CORE_REG(spsr[i]);
 -reg.addr = (uintptr_t) env-banked_spsr[i - 1];
 +reg.addr = (uintptr_t) env-banked_spsr[i+1];
  ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, reg);
  if (ret) {
  return ret;
 @@ -253,6 +270,7 @@ int kvm_arch_get_registers(CPUState *cs)
  struct kvm_one_reg reg;
  uint64_t val;
  uint32_t fpr;
 +unsigned int el;
  int i;
  int ret;
  
 @@ -325,15 +343,32 @@ int kvm_arch_get_registers(CPUState *cs)
  return ret;
  }
  
 +/* Fetch the SPSR registers
 + *
 + * KVM has an array of state indexed for all the possible aarch32
 + * privilage levels. Although not all are valid at all points
 + * there are some transitions possible which can access old state
 + * so it is worth keeping them all.
 + */
  for (i = 0; i  KVM_NR_SPSR; i++) {
  reg.id = AARCH64_CORE_REG(spsr[i]);
 -reg.addr = (uintptr_t) env-banked_spsr[i - 1];
 +reg.addr = (uintptr_t) env-banked_spsr[i+1];
  ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, reg);
  if (ret) {
  return ret;
  }
  }
  
 +el = arm_current_el(env);
 +if (is_a64(env)  el  0) {
 +g_assert(el == 1);
 +/* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
 +env-banked_spsr[0] = env-banked_spsr[1];
 +env-spsr = env-banked_spsr[aarch64_banked_spsr_index(el)];
 +} else {
 +env-spsr = env-banked_spsr[el];

 same concern with bank_number as above.

 +}
 +
  /* Advanced SIMD and FP registers */
  for (i = 0; i  32; i++) {
  reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
 -- 
 2.3.0
 

 Thanks,
 -Christoffer

-- 
Alex Bennée
--
To unsubscribe from this list:

Re: [PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

2015-03-02 Thread Christoffer Dall 
Hi Alex,

Seems like you accidentally sent out two copies of this patch, hopefully
I'm reviewing the right one...

On Wed, Feb 25, 2015 at 04:02:38PM +, Alex Bennée wrote:
 From: Christoffer Dall christoffer.d...@linaro.org
 
 The current code was negatively indexing the cpu state array and not
 synchronizing banked spsr register state with the current mode's spsr
 state, causing occasional failures with migration.
 
 Some munging is done to take care of the aarch64 mapping and also to
 ensure the most current value of the spsr is updated to the banked
 registers (relevant for KVM-TCG migration).
 
 Signed-off-by: Christoffer Dall christoffer.d...@linaro.org
 Signed-off-by: Alex Bennée alex.ben...@linaro.org
 
 ---
 v2 (ajb)
   - minor tweaks and clarifications
 
 diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
 index c60e989..1e36b0a 100644
 --- a/target-arm/kvm64.c
 +++ b/target-arm/kvm64.c
 @@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
  uint64_t val;
  int i;
  int ret;
 +unsigned int el;
  
  ARMCPU *cpu = ARM_CPU(cs);
  CPUARMState *env = cpu-env;
 @@ -206,9 +207,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
  return ret;
  }
  
 +/* Saved Program State Registers
 + *
 + * Before we restore from the banked_spsr[] array we need to
 + * ensure that any modifications to env-spsr are correctly
 + * reflected and map aarch64 exception levels if required.
 + */
 +el = arm_current_el(env);
 +if (is_a64(env)  el  0) {
 +g_assert(el == 1);
 +/* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
 +env-banked_spsr[1] = env-banked_spsr[0];
 +env-banked_spsr[aarch64_banked_spsr_index(el)] = env-spsr;
 +} else {
 +env-banked_spsr[el] = env-spsr;

is this valid if (is_a64(env)  el == 0)) ?  I thought that if you're
in el == 0, then env-banked_spsr[x] is the most up-to-date one, not
env-spsr ?

for !is_a64(env) this looks wrong, because of the same as above if el ==
0, but also because I think you need
bank_number(env-uncached_cpsr  CPSR_M) to index into the array.

 +}
 +
  for (i = 0; i  KVM_NR_SPSR; i++) {
  reg.id = AARCH64_CORE_REG(spsr[i]);
 -reg.addr = (uintptr_t) env-banked_spsr[i - 1];
 +reg.addr = (uintptr_t) env-banked_spsr[i+1];
  ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, reg);
  if (ret) {
  return ret;
 @@ -253,6 +270,7 @@ int kvm_arch_get_registers(CPUState *cs)
  struct kvm_one_reg reg;
  uint64_t val;
  uint32_t fpr;
 +unsigned int el;
  int i;
  int ret;
  
 @@ -325,15 +343,32 @@ int kvm_arch_get_registers(CPUState *cs)
  return ret;
  }
  
 +/* Fetch the SPSR registers
 + *
 + * KVM has an array of state indexed for all the possible aarch32
 + * privilage levels. Although not all are valid at all points
 + * there are some transitions possible which can access old state
 + * so it is worth keeping them all.
 + */
  for (i = 0; i  KVM_NR_SPSR; i++) {
  reg.id = AARCH64_CORE_REG(spsr[i]);
 -reg.addr = (uintptr_t) env-banked_spsr[i - 1];
 +reg.addr = (uintptr_t) env-banked_spsr[i+1];
  ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, reg);
  if (ret) {
  return ret;
  }
  }
  
 +el = arm_current_el(env);
 +if (is_a64(env)  el  0) {
 +g_assert(el == 1);
 +/* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
 +env-banked_spsr[0] = env-banked_spsr[1];
 +env-spsr = env-banked_spsr[aarch64_banked_spsr_index(el)];
 +} else {
 +env-spsr = env-banked_spsr[el];

same concern with bank_number as above.

 +}
 +
  /* Advanced SIMD and FP registers */
  for (i = 0; i  32; i++) {
  reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
 -- 
 2.3.0
 

Thanks,
-Christoffer
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] target-arm/kvm64: fix save/restore of SPSR regs

2015-02-25 Thread Alex Bennée 
From: Christoffer Dall christoffer.d...@linaro.org

The current code was negatively indexing the cpu state array and not
synchronizing banked spsr register state with the current mode's spsr
state, causing occasional failures with migration.

Some munging is done to take care of the aarch64 mapping and also to
ensure the most current value of the spsr is updated to the banked
registers (relevant for KVM-TCG migration).

Signed-off-by: Christoffer Dall christoffer.d...@linaro.org
Signed-off-by: Alex Bennée alex.ben...@linaro.org

---
v2 (ajb)
  - minor tweaks and clarifications

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index c60e989..1e36b0a 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
 uint64_t val;
 int i;
 int ret;
+unsigned int el;
 
 ARMCPU *cpu = ARM_CPU(cs);
 CPUARMState *env = cpu-env;
@@ -206,9 +207,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
 return ret;
 }
 
+/* Saved Program State Registers
+ *
+ * Before we restore from the banked_spsr[] array we need to
+ * ensure that any modifications to env-spsr are correctly
+ * reflected and map aarch64 exception levels if required.
+ */
+el = arm_current_el(env);
+if (is_a64(env)  el  0) {
+g_assert(el == 1);
+/* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
+env-banked_spsr[1] = env-banked_spsr[0];
+env-banked_spsr[aarch64_banked_spsr_index(el)] = env-spsr;
+} else {
+env-banked_spsr[el] = env-spsr;
+}
+
 for (i = 0; i  KVM_NR_SPSR; i++) {
 reg.id = AARCH64_CORE_REG(spsr[i]);
-reg.addr = (uintptr_t) env-banked_spsr[i - 1];
+reg.addr = (uintptr_t) env-banked_spsr[i+1];
 ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, reg);
 if (ret) {
 return ret;
@@ -253,6 +270,7 @@ int kvm_arch_get_registers(CPUState *cs)
 struct kvm_one_reg reg;
 uint64_t val;
 uint32_t fpr;
+unsigned int el;
 int i;
 int ret;
 
@@ -325,15 +343,32 @@ int kvm_arch_get_registers(CPUState *cs)
 return ret;
 }
 
+/* Fetch the SPSR registers
+ *
+ * KVM has an array of state indexed for all the possible aarch32
+ * privilage levels. Although not all are valid at all points
+ * there are some transitions possible which can access old state
+ * so it is worth keeping them all.
+ */
 for (i = 0; i  KVM_NR_SPSR; i++) {
 reg.id = AARCH64_CORE_REG(spsr[i]);
-reg.addr = (uintptr_t) env-banked_spsr[i - 1];
+reg.addr = (uintptr_t) env-banked_spsr[i+1];
 ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, reg);
 if (ret) {
 return ret;
 }
 }
 
+el = arm_current_el(env);
+if (is_a64(env)  el  0) {
+g_assert(el == 1);
+/* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
+env-banked_spsr[0] = env-banked_spsr[1];
+env-spsr = env-banked_spsr[aarch64_banked_spsr_index(el)];
+} else {
+env-spsr = env-banked_spsr[el];
+}
+
 /* Advanced SIMD and FP registers */
 for (i = 0; i  32; i++) {
 reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
-- 
2.3.0

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html