Re: [PATCH 2/2] arm64: KVM: Add workaround for Cortex-A57 erratum 834220

2015-11-17 Thread Will Deacon
Hi Marc,

On Mon, Nov 16, 2015 at 10:28:18AM +, Marc Zyngier wrote:
> Cortex-A57 parts up to r1p2 can misreport Stage 2 translation faults
> when a Stage 1 permission fault or device alignment fault should
> have been reported.
> 
> This patch implements the workaround (which is to validate that the
> Stage-1 translation actually succeeds) by using code patching.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  arch/arm64/Kconfig  | 21 +
>  arch/arm64/include/asm/cpufeature.h |  3 ++-
>  arch/arm64/kernel/cpu_errata.c  |  9 +
>  arch/arm64/kvm/hyp.S|  6 ++
>  4 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 9ac16a4..746d985 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -316,6 +316,27 @@ config ARM64_ERRATUM_832075
>  
> If unsure, say Y.
>  
> +config ARM64_ERRATUM_834220
> + bool "Cortex-A57: 834220: Stage 2 translation fault might be 
> incorrectly reported in presence of a Stage 1 fault"
> + depends on KVM
> + default y
> + help
> +   This option adds an alternative code sequence to work around ARM
> +   erratum 834220 on Cortex-A57 parts up to r1p2.
> +
> +   Affected Cortex-A57 parts might report a Stage 2 translation
> +   fault as a the result of a Stage 1 fault for load crossing a

s/as a the/as the/
s/for load/for a load/

> +   page boundary when there is a permission or device memory
> +   alignment fault at Stage 1 and a translation fault at Stage 2.
> +
> +   The workaround is to verify that the Stage-1 translation

Consistency between "Stage 1" and "Stage-1".

> +   doesn't generate a fault before handling the Stage-2 fault.

Same here.

> +   Please note that this does not necessarily enable the workaround,
> +   as it depends on the alternative framework, which will only patch
> +   the kernel if an affected CPU is detected.
> +
> +   If unsure, say Y.
> +
>  config ARM64_ERRATUM_845719
>   bool "Cortex-A53: 845719: a load might read incorrect data"
>   depends on COMPAT
> diff --git a/arch/arm64/include/asm/cpufeature.h 
> b/arch/arm64/include/asm/cpufeature.h
> index 11d5bb0f..52722ee 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -29,8 +29,9 @@
>  #define ARM64_HAS_PAN4
>  #define ARM64_HAS_LSE_ATOMICS5
>  #define ARM64_WORKAROUND_CAVIUM_231546
> +#define ARM64_WORKAROUND_834220  7
>  
> -#define ARM64_NCAPS  7
> +#define ARM64_NCAPS  8
>  
>  #ifndef __ASSEMBLY__
>  
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 24926f2..feb6b4e 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -75,6 +75,15 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
>  (1 << MIDR_VARIANT_SHIFT) | 2),
>   },
>  #endif
> +#ifdef CONFIG_ARM64_ERRATUM_834220
> + {
> + /* Cortex-A57 r0p0 - r1p2 */
> + .desc = "ARM erratum 834220",
> + .capability = ARM64_WORKAROUND_834220,
> + MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
> +(1 << MIDR_VARIANT_SHIFT) | 2),
> + },
> +#endif
>  #ifdef CONFIG_ARM64_ERRATUM_845719
>   {
>   /* Cortex-A53 r0p[01234] */
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 1599701..ff2e038 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -1015,9 +1015,15 @@ el1_trap:
>   b.ne1f  // Not an abort we care about
>  
>   /* This is an abort. Check for permission fault */
> +alternative_if_not ARM64_WORKAROUND_834220
>   and x2, x1, #ESR_ELx_FSC_TYPE
>   cmp x2, #FSC_PERM
>   b.ne1f  // Not a permission fault
> +alternative_else
> + nop // Use the permission fault path to
> + nop // check for a valid S1 translation,
> + nop // regardless of the ESR value.
> +alternative_endif

With the cosmetic changes:

  Reviewed-by: Will Deacon 

Can you cc stable as well, please?

Will
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH 12/21] arm64: KVM: Implement fpsimd save/restore

2015-11-17 Thread Marc Zyngier
On 17/11/15 11:49, Steve Capper wrote:
> On 17 November 2015 at 11:25, Marc Zyngier  wrote:
>> On 17/11/15 11:13, Steve Capper wrote:
>>> On 16 November 2015 at 13:11, Marc Zyngier  wrote:
 Implement the fpsimd save restore, keeping the lazy part in
 assembler (as returning to C would be overkill).

 Signed-off-by: Marc Zyngier 
 ---
  arch/arm64/kvm/hyp/Makefile |  1 +
  arch/arm64/kvm/hyp/entry.S  | 32 +++-
  arch/arm64/kvm/hyp/fpsimd.S | 33 +
  arch/arm64/kvm/hyp/hyp.h|  3 +++
  arch/arm64/kvm/hyp/switch.c |  8 
  5 files changed, 76 insertions(+), 1 deletion(-)
  create mode 100644 arch/arm64/kvm/hyp/fpsimd.S

 diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
 index 9c11b0f..56238d0 100644
 --- a/arch/arm64/kvm/hyp/Makefile
 +++ b/arch/arm64/kvm/hyp/Makefile
 @@ -9,3 +9,4 @@ obj-$(CONFIG_KVM_ARM_HOST) += sysreg-sr.o
  obj-$(CONFIG_KVM_ARM_HOST) += debug-sr.o
  obj-$(CONFIG_KVM_ARM_HOST) += entry.o
  obj-$(CONFIG_KVM_ARM_HOST) += switch.o
 +obj-$(CONFIG_KVM_ARM_HOST) += fpsimd.o
 diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
 index 2c4449a..7552922 100644
 --- a/arch/arm64/kvm/hyp/entry.S
 +++ b/arch/arm64/kvm/hyp/entry.S
 @@ -27,6 +27,7 @@

  #define CPU_GP_REG_OFFSET(x)   (CPU_GP_REGS + x)
  #define CPU_XREG_OFFSET(x) CPU_GP_REG_OFFSET(CPU_USER_PT_REGS + 8*x)
 +#define CPU_SYSREG_OFFSET(x)   (CPU_SYSREGS + 8*x)

 .text
 .pushsection.hyp.text, "ax"
 @@ -152,4 +153,33 @@ ENTRY(__guest_exit)
 ret
  ENDPROC(__guest_exit)

 -   /* Insert fault handling here */
 +ENTRY(__fpsimd_guest_restore)
 +   pushx4, lr
 +
 +   mrs x2, cptr_el2
 +   bic x2, x2, #CPTR_EL2_TFP
 +   msr cptr_el2, x2
 +   isb
 +
 +   mrs x3, tpidr_el2
 +
 +   ldr x0, [x3, #VCPU_HOST_CONTEXT]
 +   kern_hyp_va x0
 +   add x0, x0, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
 +   bl  __fpsimd_save_state
 +
 +   add x2, x3, #VCPU_CONTEXT
 +   add x0, x2, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
 +   bl  __fpsimd_restore_state
 +
 +   mrs x1, hcr_el2
 +   tbnzx1, #HCR_RW_SHIFT, 1f
 +   ldr x4, [x2, #CPU_SYSREG_OFFSET(FPEXC32_EL2)]
 +   msr fpexc32_el2, x4
 +1:
 +   pop x4, lr
 +   pop x2, x3
 +   pop x0, x1
 +
 +   eret
 +ENDPROC(__fpsimd_guest_restore)
 diff --git a/arch/arm64/kvm/hyp/fpsimd.S b/arch/arm64/kvm/hyp/fpsimd.S
 new file mode 100644
 index 000..da3f22c
 --- /dev/null
 +++ b/arch/arm64/kvm/hyp/fpsimd.S
 @@ -0,0 +1,33 @@
 +/*
 + * Copyright (C) 2015 - ARM Ltd
 + * Author: Marc Zyngier 
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program.  If not, see .
 + */
 +
 +#include 
 +
 +#include 
 +
 +   .text
 +   .pushsection.hyp.text, "ax"
 +
 +ENTRY(__fpsimd_save_state)
 +   fpsimd_save x0, 1
 +   ret
 +ENDPROC(__fpsimd_save_state)
 +
 +ENTRY(__fpsimd_restore_state)
 +   fpsimd_restore  x0, 1
 +   ret
 +ENDPROC(__fpsimd_restore_state)
 diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h
 index bf13238..240fb79 100644
 --- a/arch/arm64/kvm/hyp/hyp.h
 +++ b/arch/arm64/kvm/hyp/hyp.h
 @@ -70,6 +70,9 @@ void __debug_clear_restore_state(struct kvm_vcpu *vcpu,
  struct kvm_guest_debug_arch *dbg,
  struct kvm_cpu_context *ctxt);

 +void __fpsimd_save_state(struct user_fpsimd_state *fp_regs);
 +void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs);
 +
  u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context 
 *host_ctxt);

  #endif /* __ARM64_KVM_HYP_H__ */
 diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
 index a3af81a..06d3e20 100644
 --- a/arch/arm64/kvm/hyp/switch.c
 +++ b/arch/arm64/kvm/hyp/switch.c
 @@ -88,6 +88,7 @@ int __hyp_text __guest_run(struct kvm_vcpu *vcpu)
  {
>>>

Re: [PATCH 12/21] arm64: KVM: Implement fpsimd save/restore

2015-11-17 Thread Steve Capper
On 17 November 2015 at 11:25, Marc Zyngier  wrote:
> On 17/11/15 11:13, Steve Capper wrote:
>> On 16 November 2015 at 13:11, Marc Zyngier  wrote:
>>> Implement the fpsimd save restore, keeping the lazy part in
>>> assembler (as returning to C would be overkill).
>>>
>>> Signed-off-by: Marc Zyngier 
>>> ---
>>>  arch/arm64/kvm/hyp/Makefile |  1 +
>>>  arch/arm64/kvm/hyp/entry.S  | 32 +++-
>>>  arch/arm64/kvm/hyp/fpsimd.S | 33 +
>>>  arch/arm64/kvm/hyp/hyp.h|  3 +++
>>>  arch/arm64/kvm/hyp/switch.c |  8 
>>>  5 files changed, 76 insertions(+), 1 deletion(-)
>>>  create mode 100644 arch/arm64/kvm/hyp/fpsimd.S
>>>
>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>> index 9c11b0f..56238d0 100644
>>> --- a/arch/arm64/kvm/hyp/Makefile
>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>> @@ -9,3 +9,4 @@ obj-$(CONFIG_KVM_ARM_HOST) += sysreg-sr.o
>>>  obj-$(CONFIG_KVM_ARM_HOST) += debug-sr.o
>>>  obj-$(CONFIG_KVM_ARM_HOST) += entry.o
>>>  obj-$(CONFIG_KVM_ARM_HOST) += switch.o
>>> +obj-$(CONFIG_KVM_ARM_HOST) += fpsimd.o
>>> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
>>> index 2c4449a..7552922 100644
>>> --- a/arch/arm64/kvm/hyp/entry.S
>>> +++ b/arch/arm64/kvm/hyp/entry.S
>>> @@ -27,6 +27,7 @@
>>>
>>>  #define CPU_GP_REG_OFFSET(x)   (CPU_GP_REGS + x)
>>>  #define CPU_XREG_OFFSET(x) CPU_GP_REG_OFFSET(CPU_USER_PT_REGS + 8*x)
>>> +#define CPU_SYSREG_OFFSET(x)   (CPU_SYSREGS + 8*x)
>>>
>>> .text
>>> .pushsection.hyp.text, "ax"
>>> @@ -152,4 +153,33 @@ ENTRY(__guest_exit)
>>> ret
>>>  ENDPROC(__guest_exit)
>>>
>>> -   /* Insert fault handling here */
>>> +ENTRY(__fpsimd_guest_restore)
>>> +   pushx4, lr
>>> +
>>> +   mrs x2, cptr_el2
>>> +   bic x2, x2, #CPTR_EL2_TFP
>>> +   msr cptr_el2, x2
>>> +   isb
>>> +
>>> +   mrs x3, tpidr_el2
>>> +
>>> +   ldr x0, [x3, #VCPU_HOST_CONTEXT]
>>> +   kern_hyp_va x0
>>> +   add x0, x0, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
>>> +   bl  __fpsimd_save_state
>>> +
>>> +   add x2, x3, #VCPU_CONTEXT
>>> +   add x0, x2, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
>>> +   bl  __fpsimd_restore_state
>>> +
>>> +   mrs x1, hcr_el2
>>> +   tbnzx1, #HCR_RW_SHIFT, 1f
>>> +   ldr x4, [x2, #CPU_SYSREG_OFFSET(FPEXC32_EL2)]
>>> +   msr fpexc32_el2, x4
>>> +1:
>>> +   pop x4, lr
>>> +   pop x2, x3
>>> +   pop x0, x1
>>> +
>>> +   eret
>>> +ENDPROC(__fpsimd_guest_restore)
>>> diff --git a/arch/arm64/kvm/hyp/fpsimd.S b/arch/arm64/kvm/hyp/fpsimd.S
>>> new file mode 100644
>>> index 000..da3f22c
>>> --- /dev/null
>>> +++ b/arch/arm64/kvm/hyp/fpsimd.S
>>> @@ -0,0 +1,33 @@
>>> +/*
>>> + * Copyright (C) 2015 - ARM Ltd
>>> + * Author: Marc Zyngier 
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program.  If not, see .
>>> + */
>>> +
>>> +#include 
>>> +
>>> +#include 
>>> +
>>> +   .text
>>> +   .pushsection.hyp.text, "ax"
>>> +
>>> +ENTRY(__fpsimd_save_state)
>>> +   fpsimd_save x0, 1
>>> +   ret
>>> +ENDPROC(__fpsimd_save_state)
>>> +
>>> +ENTRY(__fpsimd_restore_state)
>>> +   fpsimd_restore  x0, 1
>>> +   ret
>>> +ENDPROC(__fpsimd_restore_state)
>>> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h
>>> index bf13238..240fb79 100644
>>> --- a/arch/arm64/kvm/hyp/hyp.h
>>> +++ b/arch/arm64/kvm/hyp/hyp.h
>>> @@ -70,6 +70,9 @@ void __debug_clear_restore_state(struct kvm_vcpu *vcpu,
>>>  struct kvm_guest_debug_arch *dbg,
>>>  struct kvm_cpu_context *ctxt);
>>>
>>> +void __fpsimd_save_state(struct user_fpsimd_state *fp_regs);
>>> +void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs);
>>> +
>>>  u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context 
>>> *host_ctxt);
>>>
>>>  #endif /* __ARM64_KVM_HYP_H__ */
>>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>>> index a3af81a..06d3e20 100644
>>> --- a/arch/arm64/kvm/hyp/switch.c
>>> +++ b/arch/arm64/kvm/hyp/switch.c
>>> @@ -88,6 +88,7 @@ int __hyp_text __guest_run(struct kvm_vcpu *vcpu)
>>>  {
>>> struct kvm_cpu_context *host_ctxt;
>>> struct kvm_cpu_context *guest_ctxt;
>>> +   bool fp_enabled;
>>> u64 exit_code;
>>>
>>> vcpu = k

Re: [PATCH 1/2] arm64: KVM: Fix AArch32 to AArch64 register mapping

2015-11-17 Thread Robin Murphy

Hi Marc,

On 16/11/15 10:28, Marc Zyngier wrote:

When running a 32bit guest under a 64bit hypervisor, the ARMv8
architecture defines a mapping of the 32bit registers in the 64bit
space. This includes banked registers that are being demultiplexed
over the 64bit ones.

On exception caused by an operation involving a 32bit register, the
HW exposes the register number in the ESR_EL2 register. It was so
far understood that SW had to compute which register was AArch64
register was used (based on the current AArch32 mode and register
number).

It turns out that I misinterpreted the ARM ARM, and the clue is in
D1.20.1: "For some exceptions, the exception syndrome given in the
ESR_ELx identifies one or more register numbers from the issued
instruction that generated the exception. Where the exception is
taken from an Exception level using AArch32 these register numbers
give the AArch64 view of the register."

Which means that the HW is already giving us the translated version,
and that we shouldn't try to interpret it at all (for example, doing
an MMIO operation from the IRQ mode using the LR register leads to
very unexpected behaviours).

The fix is thus not to perform a call to vcpu_reg32() at all from
vcpu_reg(), and use whatever register number is supplied directly.
The only case we need to find out about the mapping is when we
actively generate a register access, which only occurs when injecting
a fault in a guest.

Signed-off-by: Marc Zyngier 
---
  arch/arm64/include/asm/kvm_emulate.h | 8 +---
  arch/arm64/kvm/inject_fault.c| 2 +-
  2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h 
b/arch/arm64/include/asm/kvm_emulate.h
index 17e92f0..3ca894e 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -99,11 +99,13 @@ static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
*vcpu_cpsr(vcpu) |= COMPAT_PSR_T_BIT;
  }

+/*
+ * vcpu_reg should always be passed a register number coming from a
+ * read of ESR_EL2. Otherwise, it may give the wrong result on AArch32
+ * with banked registers.
+ */
  static inline unsigned long *vcpu_reg(const struct kvm_vcpu *vcpu, u8 reg_num)
  {
-   if (vcpu_mode_is_32bit(vcpu))
-   return vcpu_reg32(vcpu, reg_num);
-
return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.regs[reg_num];
  }

diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
index 85c5715..648112e 100644
--- a/arch/arm64/kvm/inject_fault.c
+++ b/arch/arm64/kvm/inject_fault.c
@@ -48,7 +48,7 @@ static void prepare_fault32(struct kvm_vcpu *vcpu, u32 mode, 
u32 vect_offset)

/* Note: These now point to the banked copies */
*vcpu_spsr(vcpu) = new_spsr_value;
-   *vcpu_reg(vcpu, 14) = *vcpu_pc(vcpu) + return_offset;
+   *vcpu_reg32(vcpu, 14) = *vcpu_pc(vcpu) + return_offset;


To the best of my knowledge after picking through all the uses of 
vcpu_reg, particularly in the shared 32-bit code, this does seem to be 
the only one which involves a potentially-banked register number that 
didn't originally come from an ESR read, and thus needs translation.


Reviewed-by: Robin Murphy 

(unfortunately I don't have an actual test-case as it was already a 
third-hand report when I started trying to look into it).


Thanks for picking this up,
Robin.



/* Branch to exception vector */
if (sctlr & (1 << 13))



___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH 12/21] arm64: KVM: Implement fpsimd save/restore

2015-11-17 Thread Marc Zyngier
On 17/11/15 11:13, Steve Capper wrote:
> On 16 November 2015 at 13:11, Marc Zyngier  wrote:
>> Implement the fpsimd save restore, keeping the lazy part in
>> assembler (as returning to C would be overkill).
>>
>> Signed-off-by: Marc Zyngier 
>> ---
>>  arch/arm64/kvm/hyp/Makefile |  1 +
>>  arch/arm64/kvm/hyp/entry.S  | 32 +++-
>>  arch/arm64/kvm/hyp/fpsimd.S | 33 +
>>  arch/arm64/kvm/hyp/hyp.h|  3 +++
>>  arch/arm64/kvm/hyp/switch.c |  8 
>>  5 files changed, 76 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/arm64/kvm/hyp/fpsimd.S
>>
>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>> index 9c11b0f..56238d0 100644
>> --- a/arch/arm64/kvm/hyp/Makefile
>> +++ b/arch/arm64/kvm/hyp/Makefile
>> @@ -9,3 +9,4 @@ obj-$(CONFIG_KVM_ARM_HOST) += sysreg-sr.o
>>  obj-$(CONFIG_KVM_ARM_HOST) += debug-sr.o
>>  obj-$(CONFIG_KVM_ARM_HOST) += entry.o
>>  obj-$(CONFIG_KVM_ARM_HOST) += switch.o
>> +obj-$(CONFIG_KVM_ARM_HOST) += fpsimd.o
>> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
>> index 2c4449a..7552922 100644
>> --- a/arch/arm64/kvm/hyp/entry.S
>> +++ b/arch/arm64/kvm/hyp/entry.S
>> @@ -27,6 +27,7 @@
>>
>>  #define CPU_GP_REG_OFFSET(x)   (CPU_GP_REGS + x)
>>  #define CPU_XREG_OFFSET(x) CPU_GP_REG_OFFSET(CPU_USER_PT_REGS + 8*x)
>> +#define CPU_SYSREG_OFFSET(x)   (CPU_SYSREGS + 8*x)
>>
>> .text
>> .pushsection.hyp.text, "ax"
>> @@ -152,4 +153,33 @@ ENTRY(__guest_exit)
>> ret
>>  ENDPROC(__guest_exit)
>>
>> -   /* Insert fault handling here */
>> +ENTRY(__fpsimd_guest_restore)
>> +   pushx4, lr
>> +
>> +   mrs x2, cptr_el2
>> +   bic x2, x2, #CPTR_EL2_TFP
>> +   msr cptr_el2, x2
>> +   isb
>> +
>> +   mrs x3, tpidr_el2
>> +
>> +   ldr x0, [x3, #VCPU_HOST_CONTEXT]
>> +   kern_hyp_va x0
>> +   add x0, x0, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
>> +   bl  __fpsimd_save_state
>> +
>> +   add x2, x3, #VCPU_CONTEXT
>> +   add x0, x2, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
>> +   bl  __fpsimd_restore_state
>> +
>> +   mrs x1, hcr_el2
>> +   tbnzx1, #HCR_RW_SHIFT, 1f
>> +   ldr x4, [x2, #CPU_SYSREG_OFFSET(FPEXC32_EL2)]
>> +   msr fpexc32_el2, x4
>> +1:
>> +   pop x4, lr
>> +   pop x2, x3
>> +   pop x0, x1
>> +
>> +   eret
>> +ENDPROC(__fpsimd_guest_restore)
>> diff --git a/arch/arm64/kvm/hyp/fpsimd.S b/arch/arm64/kvm/hyp/fpsimd.S
>> new file mode 100644
>> index 000..da3f22c
>> --- /dev/null
>> +++ b/arch/arm64/kvm/hyp/fpsimd.S
>> @@ -0,0 +1,33 @@
>> +/*
>> + * Copyright (C) 2015 - ARM Ltd
>> + * Author: Marc Zyngier 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see .
>> + */
>> +
>> +#include 
>> +
>> +#include 
>> +
>> +   .text
>> +   .pushsection.hyp.text, "ax"
>> +
>> +ENTRY(__fpsimd_save_state)
>> +   fpsimd_save x0, 1
>> +   ret
>> +ENDPROC(__fpsimd_save_state)
>> +
>> +ENTRY(__fpsimd_restore_state)
>> +   fpsimd_restore  x0, 1
>> +   ret
>> +ENDPROC(__fpsimd_restore_state)
>> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h
>> index bf13238..240fb79 100644
>> --- a/arch/arm64/kvm/hyp/hyp.h
>> +++ b/arch/arm64/kvm/hyp/hyp.h
>> @@ -70,6 +70,9 @@ void __debug_clear_restore_state(struct kvm_vcpu *vcpu,
>>  struct kvm_guest_debug_arch *dbg,
>>  struct kvm_cpu_context *ctxt);
>>
>> +void __fpsimd_save_state(struct user_fpsimd_state *fp_regs);
>> +void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs);
>> +
>>  u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host_ctxt);
>>
>>  #endif /* __ARM64_KVM_HYP_H__ */
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index a3af81a..06d3e20 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -88,6 +88,7 @@ int __hyp_text __guest_run(struct kvm_vcpu *vcpu)
>>  {
>> struct kvm_cpu_context *host_ctxt;
>> struct kvm_cpu_context *guest_ctxt;
>> +   bool fp_enabled;
>> u64 exit_code;
>>
>> vcpu = kern_hyp_va(vcpu);
>> @@ -117,6 +118,8 @@ int __hyp_text __guest_run(struct kvm_vcpu *vcpu)
>> exit_code = __guest_enter(vcpu, host_ctxt);
>> /* And we're baaack! */
>>
>> + 

Re: [PATCH 12/21] arm64: KVM: Implement fpsimd save/restore

2015-11-17 Thread Steve Capper
On 16 November 2015 at 13:11, Marc Zyngier  wrote:
> Implement the fpsimd save restore, keeping the lazy part in
> assembler (as returning to C would be overkill).
>
> Signed-off-by: Marc Zyngier 
> ---
>  arch/arm64/kvm/hyp/Makefile |  1 +
>  arch/arm64/kvm/hyp/entry.S  | 32 +++-
>  arch/arm64/kvm/hyp/fpsimd.S | 33 +
>  arch/arm64/kvm/hyp/hyp.h|  3 +++
>  arch/arm64/kvm/hyp/switch.c |  8 
>  5 files changed, 76 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/kvm/hyp/fpsimd.S
>
> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index 9c11b0f..56238d0 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_KVM_ARM_HOST) += sysreg-sr.o
>  obj-$(CONFIG_KVM_ARM_HOST) += debug-sr.o
>  obj-$(CONFIG_KVM_ARM_HOST) += entry.o
>  obj-$(CONFIG_KVM_ARM_HOST) += switch.o
> +obj-$(CONFIG_KVM_ARM_HOST) += fpsimd.o
> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index 2c4449a..7552922 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -27,6 +27,7 @@
>
>  #define CPU_GP_REG_OFFSET(x)   (CPU_GP_REGS + x)
>  #define CPU_XREG_OFFSET(x) CPU_GP_REG_OFFSET(CPU_USER_PT_REGS + 8*x)
> +#define CPU_SYSREG_OFFSET(x)   (CPU_SYSREGS + 8*x)
>
> .text
> .pushsection.hyp.text, "ax"
> @@ -152,4 +153,33 @@ ENTRY(__guest_exit)
> ret
>  ENDPROC(__guest_exit)
>
> -   /* Insert fault handling here */
> +ENTRY(__fpsimd_guest_restore)
> +   pushx4, lr
> +
> +   mrs x2, cptr_el2
> +   bic x2, x2, #CPTR_EL2_TFP
> +   msr cptr_el2, x2
> +   isb
> +
> +   mrs x3, tpidr_el2
> +
> +   ldr x0, [x3, #VCPU_HOST_CONTEXT]
> +   kern_hyp_va x0
> +   add x0, x0, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
> +   bl  __fpsimd_save_state
> +
> +   add x2, x3, #VCPU_CONTEXT
> +   add x0, x2, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
> +   bl  __fpsimd_restore_state
> +
> +   mrs x1, hcr_el2
> +   tbnzx1, #HCR_RW_SHIFT, 1f
> +   ldr x4, [x2, #CPU_SYSREG_OFFSET(FPEXC32_EL2)]
> +   msr fpexc32_el2, x4
> +1:
> +   pop x4, lr
> +   pop x2, x3
> +   pop x0, x1
> +
> +   eret
> +ENDPROC(__fpsimd_guest_restore)
> diff --git a/arch/arm64/kvm/hyp/fpsimd.S b/arch/arm64/kvm/hyp/fpsimd.S
> new file mode 100644
> index 000..da3f22c
> --- /dev/null
> +++ b/arch/arm64/kvm/hyp/fpsimd.S
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2015 - ARM Ltd
> + * Author: Marc Zyngier 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include 
> +
> +#include 
> +
> +   .text
> +   .pushsection.hyp.text, "ax"
> +
> +ENTRY(__fpsimd_save_state)
> +   fpsimd_save x0, 1
> +   ret
> +ENDPROC(__fpsimd_save_state)
> +
> +ENTRY(__fpsimd_restore_state)
> +   fpsimd_restore  x0, 1
> +   ret
> +ENDPROC(__fpsimd_restore_state)
> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h
> index bf13238..240fb79 100644
> --- a/arch/arm64/kvm/hyp/hyp.h
> +++ b/arch/arm64/kvm/hyp/hyp.h
> @@ -70,6 +70,9 @@ void __debug_clear_restore_state(struct kvm_vcpu *vcpu,
>  struct kvm_guest_debug_arch *dbg,
>  struct kvm_cpu_context *ctxt);
>
> +void __fpsimd_save_state(struct user_fpsimd_state *fp_regs);
> +void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs);
> +
>  u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host_ctxt);
>
>  #endif /* __ARM64_KVM_HYP_H__ */
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index a3af81a..06d3e20 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -88,6 +88,7 @@ int __hyp_text __guest_run(struct kvm_vcpu *vcpu)
>  {
> struct kvm_cpu_context *host_ctxt;
> struct kvm_cpu_context *guest_ctxt;
> +   bool fp_enabled;
> u64 exit_code;
>
> vcpu = kern_hyp_va(vcpu);
> @@ -117,6 +118,8 @@ int __hyp_text __guest_run(struct kvm_vcpu *vcpu)
> exit_code = __guest_enter(vcpu, host_ctxt);
> /* And we're baaack! */
>
> +   fp_enabled = !!(read_sysreg(cptr_el2) & CPTR_EL2_TFP);

Should this not be a single logical not?
If CPTR_EL2_TFP is set then the floating point will trap in the guest,
thus float