Re: [PATCH v4 29/40] KVM: arm64: Prepare to handle deferred save/restore of 32-bit registers

2018-02-23 Thread Julien Grall 

Hi Christoffer,

On 15/02/18 21:03, Christoffer Dall wrote:

32-bit registers are not used by a 64-bit host kernel and can be
deferred, but we need to rework the accesses to this register to access
the latest value depending on whether or not guest system registers are
loaded on the CPU or only reside in memory.


I realize that you explain in the new patch why FPEXC32_EL2 is not 
directly accessed below. But I think it might be worth to explain it 
here as well because the commit message implies all 32-bit registers 
could be deferred.




Signed-off-by: Christoffer Dall 
---

Notes:
 Changes since v3:
  - Don't also try to write hardware spsr when sysregs are not loaded
  - Adapted patch to use switch-based sysreg save/restore approach
  - (Kept additional BUG_ON() in vcpu_read_spsr32() to keep the compiler 
happy)
 
 Changes since v2:

  - New patch (deferred register handling has been reworked)

  arch/arm64/include/asm/kvm_emulate.h | 32 +
  arch/arm64/kvm/regmap.c  | 67 +++-
  arch/arm64/kvm/sys_regs.c|  6 
  3 files changed, 65 insertions(+), 40 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h 
b/arch/arm64/include/asm/kvm_emulate.h
index 9cb13b23c7a1..23b33e8ea03a 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -33,7 +33,8 @@
  #include 
  
  unsigned long *vcpu_reg32(const struct kvm_vcpu *vcpu, u8 reg_num);

-unsigned long *vcpu_spsr32(const struct kvm_vcpu *vcpu);
+unsigned long vcpu_read_spsr32(const struct kvm_vcpu *vcpu);
+void vcpu_write_spsr32(struct kvm_vcpu *vcpu, unsigned long v);
  
  bool kvm_condition_valid32(const struct kvm_vcpu *vcpu);

  void kvm_skip_instr32(struct kvm_vcpu *vcpu, bool is_wide_instr);
@@ -162,41 +163,26 @@ static inline void vcpu_set_reg(struct kvm_vcpu *vcpu, u8 
reg_num,
  
  static inline unsigned long vcpu_read_spsr(const struct kvm_vcpu *vcpu)

  {
-   unsigned long *p = (unsigned long 
*)&vcpu_gp_regs(vcpu)->spsr[KVM_SPSR_EL1];
-
-   if (vcpu_mode_is_32bit(vcpu)) {
-   unsigned long *p_32bit = vcpu_spsr32(vcpu);
-
-   /* KVM_SPSR_SVC aliases KVM_SPSR_EL1 */
-   if (p_32bit != (unsigned long *)p)
-   return *p_32bit;
-   }
+   if (vcpu_mode_is_32bit(vcpu))
+   return vcpu_read_spsr32(vcpu);
  
  	if (vcpu->arch.sysregs_loaded_on_cpu)

return read_sysreg_el1(spsr);
else
-   return *p;
+   return vcpu_gp_regs(vcpu)->spsr[KVM_SPSR_EL1];
  }
  
-static inline void vcpu_write_spsr(const struct kvm_vcpu *vcpu, unsigned long v)

+static inline void vcpu_write_spsr(struct kvm_vcpu *vcpu, unsigned long v)
  {
-   unsigned long *p = (unsigned long 
*)&vcpu_gp_regs(vcpu)->spsr[KVM_SPSR_EL1];
-
-   /* KVM_SPSR_SVC aliases KVM_SPSR_EL1 */
if (vcpu_mode_is_32bit(vcpu)) {
-   unsigned long *p_32bit = vcpu_spsr32(vcpu);
-
-   /* KVM_SPSR_SVC aliases KVM_SPSR_EL1 */
-   if (p_32bit != (unsigned long *)p) {
-   *p_32bit = v;
-   return;
-   }
+   vcpu_write_spsr32(vcpu, v);
+   return;
}
  
  	if (vcpu->arch.sysregs_loaded_on_cpu)

write_sysreg_el1(v, spsr);
else
-   *p = v;
+   vcpu_gp_regs(vcpu)->spsr[KVM_SPSR_EL1] = v;
  }
  
  static inline bool vcpu_mode_priv(const struct kvm_vcpu *vcpu)

diff --git a/arch/arm64/kvm/regmap.c b/arch/arm64/kvm/regmap.c
index bbc6ae32e4af..eefe403a2e63 100644
--- a/arch/arm64/kvm/regmap.c
+++ b/arch/arm64/kvm/regmap.c
@@ -141,28 +141,61 @@ unsigned long *vcpu_reg32(const struct kvm_vcpu *vcpu, u8 
reg_num)
  /*
   * Return the SPSR for the current mode of the virtual CPU.
   */
-unsigned long *vcpu_spsr32(const struct kvm_vcpu *vcpu)
+static int vcpu_spsr32_mode(const struct kvm_vcpu *vcpu)
  {
unsigned long mode = *vcpu_cpsr(vcpu) & COMPAT_PSR_MODE_MASK;
switch (mode) {
-   case COMPAT_PSR_MODE_SVC:
-   mode = KVM_SPSR_SVC;
-   break;
-   case COMPAT_PSR_MODE_ABT:
-   mode = KVM_SPSR_ABT;
-   break;
-   case COMPAT_PSR_MODE_UND:
-   mode = KVM_SPSR_UND;
-   break;
-   case COMPAT_PSR_MODE_IRQ:
-   mode = KVM_SPSR_IRQ;
-   break;
-   case COMPAT_PSR_MODE_FIQ:
-   mode = KVM_SPSR_FIQ;
-   break;
+   case COMPAT_PSR_MODE_SVC: return KVM_SPSR_SVC;
+   case COMPAT_PSR_MODE_ABT: return KVM_SPSR_ABT;
+   case COMPAT_PSR_MODE_UND: return KVM_SPSR_UND;
+   case COMPAT_PSR_MODE_IRQ: return KVM_SPSR_IRQ;
+   case COMPAT_PSR_MODE_FIQ: return KVM_SPSR_FIQ;
+   default: BUG();
+   }
+}
+
+unsigned long vcpu_read_spsr32(const struct kvm_vcpu *vcpu)
+{
+   int spsr_idx = vcpu_spsr32_mode(vcpu);
+

Re: [PATCH v4 29/40] KVM: arm64: Prepare to handle deferred save/restore of 32-bit registers

2018-02-22 Thread Andrew Jones 
On Thu, Feb 15, 2018 at 10:03:21PM +0100, Christoffer Dall wrote:
> 32-bit registers are not used by a 64-bit host kernel and can be
> deferred, but we need to rework the accesses to this register to access

these registers

> the latest value depending on whether or not guest system registers are

values

> loaded on the CPU or only reside in memory.
> 
> Signed-off-by: Christoffer Dall 
> ---
> 
> Notes:
> Changes since v3:
>  - Don't also try to write hardware spsr when sysregs are not loaded
>  - Adapted patch to use switch-based sysreg save/restore approach
>  - (Kept additional BUG_ON() in vcpu_read_spsr32() to keep the compiler 
> happy)
> 
> Changes since v2:
>  - New patch (deferred register handling has been reworked)
> 
>  arch/arm64/include/asm/kvm_emulate.h | 32 +
>  arch/arm64/kvm/regmap.c  | 67 
> +++-
>  arch/arm64/kvm/sys_regs.c|  6 
>  3 files changed, 65 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_emulate.h 
> b/arch/arm64/include/asm/kvm_emulate.h
> index 9cb13b23c7a1..23b33e8ea03a 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -33,7 +33,8 @@
>  #include 
>  
>  unsigned long *vcpu_reg32(const struct kvm_vcpu *vcpu, u8 reg_num);
> -unsigned long *vcpu_spsr32(const struct kvm_vcpu *vcpu);
> +unsigned long vcpu_read_spsr32(const struct kvm_vcpu *vcpu);
> +void vcpu_write_spsr32(struct kvm_vcpu *vcpu, unsigned long v);
>  
>  bool kvm_condition_valid32(const struct kvm_vcpu *vcpu);
>  void kvm_skip_instr32(struct kvm_vcpu *vcpu, bool is_wide_instr);
> @@ -162,41 +163,26 @@ static inline void vcpu_set_reg(struct kvm_vcpu *vcpu, 
> u8 reg_num,
>  
>  static inline unsigned long vcpu_read_spsr(const struct kvm_vcpu *vcpu)
>  {
> - unsigned long *p = (unsigned long 
> *)&vcpu_gp_regs(vcpu)->spsr[KVM_SPSR_EL1];
> -
> - if (vcpu_mode_is_32bit(vcpu)) {
> - unsigned long *p_32bit = vcpu_spsr32(vcpu);
> -
> - /* KVM_SPSR_SVC aliases KVM_SPSR_EL1 */
> - if (p_32bit != (unsigned long *)p)
> - return *p_32bit;
> - }
> + if (vcpu_mode_is_32bit(vcpu))
> + return vcpu_read_spsr32(vcpu);
>  
>   if (vcpu->arch.sysregs_loaded_on_cpu)
>   return read_sysreg_el1(spsr);
>   else
> - return *p;
> + return vcpu_gp_regs(vcpu)->spsr[KVM_SPSR_EL1];
>  }
>  
> -static inline void vcpu_write_spsr(const struct kvm_vcpu *vcpu, unsigned 
> long v)
> +static inline void vcpu_write_spsr(struct kvm_vcpu *vcpu, unsigned long v)
>  {
> - unsigned long *p = (unsigned long 
> *)&vcpu_gp_regs(vcpu)->spsr[KVM_SPSR_EL1];
> -
> - /* KVM_SPSR_SVC aliases KVM_SPSR_EL1 */
>   if (vcpu_mode_is_32bit(vcpu)) {
> - unsigned long *p_32bit = vcpu_spsr32(vcpu);
> -
> - /* KVM_SPSR_SVC aliases KVM_SPSR_EL1 */
> - if (p_32bit != (unsigned long *)p) {
> - *p_32bit = v;
> - return;
> - }
> + vcpu_write_spsr32(vcpu, v);
> + return;
>   }
>  
>   if (vcpu->arch.sysregs_loaded_on_cpu)
>   write_sysreg_el1(v, spsr);
>   else
> - *p = v;
> + vcpu_gp_regs(vcpu)->spsr[KVM_SPSR_EL1] = v;
>  }
>  
>  static inline bool vcpu_mode_priv(const struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/kvm/regmap.c b/arch/arm64/kvm/regmap.c
> index bbc6ae32e4af..eefe403a2e63 100644
> --- a/arch/arm64/kvm/regmap.c
> +++ b/arch/arm64/kvm/regmap.c
> @@ -141,28 +141,61 @@ unsigned long *vcpu_reg32(const struct kvm_vcpu *vcpu, 
> u8 reg_num)
>  /*
>   * Return the SPSR for the current mode of the virtual CPU.
>   */
> -unsigned long *vcpu_spsr32(const struct kvm_vcpu *vcpu)
> +static int vcpu_spsr32_mode(const struct kvm_vcpu *vcpu)
>  {
>   unsigned long mode = *vcpu_cpsr(vcpu) & COMPAT_PSR_MODE_MASK;
>   switch (mode) {
> - case COMPAT_PSR_MODE_SVC:
> - mode = KVM_SPSR_SVC;
> - break;
> - case COMPAT_PSR_MODE_ABT:
> - mode = KVM_SPSR_ABT;
> - break;
> - case COMPAT_PSR_MODE_UND:
> - mode = KVM_SPSR_UND;
> - break;
> - case COMPAT_PSR_MODE_IRQ:
> - mode = KVM_SPSR_IRQ;
> - break;
> - case COMPAT_PSR_MODE_FIQ:
> - mode = KVM_SPSR_FIQ;
> - break;
> + case COMPAT_PSR_MODE_SVC: return KVM_SPSR_SVC;
> + case COMPAT_PSR_MODE_ABT: return KVM_SPSR_ABT;
> + case COMPAT_PSR_MODE_UND: return KVM_SPSR_UND;
> + case COMPAT_PSR_MODE_IRQ: return KVM_SPSR_IRQ;
> + case COMPAT_PSR_MODE_FIQ: return KVM_SPSR_FIQ;
> + default: BUG();
> + }
> +}
> +
> +unsigned long vcpu_read_spsr32(const struct kvm_vcpu *vcpu)
> +{
> + int spsr_idx = vcpu_spsr32_mode(vcpu);
> +
> + if (!vcpu->arch.sysregs_loaded_on_cpu)
> + return vc

Re: [PATCH v4 29/40] KVM: arm64: Prepare to handle deferred save/restore of 32-bit registers

2018-02-21 Thread Marc Zyngier 
On Thu, 15 Feb 2018 21:03:21 +,
Christoffer Dall wrote:
> 
> 32-bit registers are not used by a 64-bit host kernel and can be
> deferred, but we need to rework the accesses to this register to access
> the latest value depending on whether or not guest system registers are
> loaded on the CPU or only reside in memory.
> 
> Signed-off-by: Christoffer Dall 

Ah, much nicer than the "clunky" version! :-)

Reviewed-by: Marc Zyngier 

M.

-- 
Jazz is not dead, it just smell funny.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm