Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1332309 > unrolled thread

[PATCH v4 00/23] arm64: Virtualization Host Extension support

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 00/23] arm64: Virtualization Host Extension support Marc Zyngier <marc.zyngier@arm.com> - 2016-02-11 19:50 +0100
    [PATCH v4 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP Marc Zyngier <marc.zyngier@arm.com> - 2016-02-11 19:50 +0100
      Re: [PATCH v4 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if  running in HYP Catalin Marinas <catalin.marinas@arm.com> - 2016-02-15 11:30 +0100
      Re: [PATCH v4 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if  running in HYP Will Deacon <will.deacon@arm.com> - 2016-02-15 18:50 +0100
        Re: [PATCH v4 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if  running in HYP Will Deacon <will.deacon@arm.com> - 2016-02-15 20:10 +0100
          Re: [PATCH v4 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if  running in HYP Marc Zyngier <marc.zyngier@arm.com> - 2016-02-16 11:00 +0100
      [PATCH v4.1] arm64: perf: Count EL2 events if the kernel is running in HYP Marc Zyngier <marc.zyngier@arm.com> - 2016-02-17 19:00 +0100
        Re: [PATCH v4.1] arm64: perf: Count EL2 events if the kernel is  running in HYP Will Deacon <will.deacon@arm.com> - 2016-02-19 15:40 +0100
    [PATCH v4 19/23] arm64: KVM: Move most of the fault decoding to C Marc Zyngier <marc.zyngier@arm.com> - 2016-02-11 19:50 +0100
    [PATCH v4 20/23] arm64: perf: Count EL2 events if the kernel is running in HYP Marc Zyngier <marc.zyngier@arm.com> - 2016-02-11 19:50 +0100
      Re: [PATCH v4 20/23] arm64: perf: Count EL2 events if the kernel is  running in HYP Will Deacon <will.deacon@arm.com> - 2016-02-15 18:30 +0100
        Re: [PATCH v4 20/23] arm64: perf: Count EL2 events if the kernel is  running in HYP Will Deacon <will.deacon@arm.com> - 2016-02-15 19:30 +0100
        Re: [PATCH v4 20/23] arm64: perf: Count EL2 events if the kernel is  running in HYP Marc Zyngier <marc.zyngier@arm.com> - 2016-02-15 19:30 +0100
    [PATCH v4 14/23] arm64: KVM: VHE: Make __fpsimd_enabled VHE aware Marc Zyngier <marc.zyngier@arm.com> - 2016-02-11 19:50 +0100
    [PATCH v4 13/23] arm64: KVM: VHE: Enable minimal sysreg save/restore Marc Zyngier <marc.zyngier@arm.com> - 2016-02-11 19:50 +0100
    Re: [PATCH v4 00/23] arm64: Virtualization Host Extension support Christoffer Dall <christoffer.dall@linaro.org> - 2016-02-11 20:10 +0100
      Re: [PATCH v4 00/23] arm64: Virtualization Host Extension support Marc Zyngier <marc.zyngier@arm.com> - 2016-02-11 20:20 +0100
        Re: [PATCH v4 00/23] arm64: Virtualization Host Extension support Catalin Marinas <catalin.marinas@arm.com> - 2016-02-15 11:30 +0100
          Re: [PATCH v4 00/23] arm64: Virtualization Host Extension support Marc Zyngier <marc.zyngier@arm.com> - 2016-02-15 11:40 +0100

#1332309 — [PATCH v4 00/23] arm64: Virtualization Host Extension support

ARMv8.1 comes with the "Virtualization Host Extension" (VHE for
short), which enables simpler support of Type-2 hypervisors.

This extension allows the kernel to directly run at EL2, and
significantly reduces the number of system registers shared between
host and guest, reducing the overhead of virtualization.

In order to have the same kernel binary running on all versions of the
architecture, this series makes heavy use of runtime code patching.

The first 22 patches massage the KVM code to deal with VHE and enable
Linux to run at EL2. The last patch catches an ugly case when VHE
capable CPUs are paired with some of their less capable siblings. This
should never happen, but hey...

I have deliberately left out some of the more "advanced"
optimizations, as they are likely to distract the reviewer from the
core infrastructure, which is what I care about at the moment.

Note: GDB is currently busted on VHE systems, as it checks for version
      6 on the debug architecture, while VHE is version 7. The
      binutils people are on the case.

This has been tested on the FVP_Base_SLV-V8-A model, and based on
v4.5-rc3 + kvmarm/master. I've put a branch out on:

git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git kvm-arm64/vhe

* From v3:
  - Cleaned debug handling after comments from Catalin
  - Added missing barriers in the panic patch
  - Collected Rb and Acks from Christoffer and Catalin

* From v2:
  - Added support for perf to count kernel events in EL2
  - Added support for EL2 breakpoints
  - Moved the VTCR_EL2 setup from assembly to C
  - Made the fault handling easier to understand (hopefuly)
  - Plenty of smaller fixups

* From v1:
  - Full rewrite now that the World Switch is written in C code.
  - Dropped the "early IRQ handling" for the moment.

Marc Zyngier (23):
  arm/arm64: KVM: Add hook for C-based stage2 init
  arm64: KVM: Switch to C-based stage2 init
  arm/arm64: Add new is_kernel_in_hyp_mode predicate
  arm64: Allow the arch timer to use the HYP timer
  arm64: Add ARM64_HAS_VIRT_HOST_EXTN feature
  arm64: KVM: Skip HYP setup when already running in HYP
  arm64: KVM: VHE: Patch out use of HVC
  arm64: KVM: VHE: Patch out kern_hyp_va
  arm64: KVM: VHE: Introduce unified system register accessors
  arm64: KVM: VHE: Differenciate host/guest sysreg save/restore
  arm64: KVM: VHE: Split save/restore of registers shared between guest
    and host
  arm64: KVM: VHE: Use unified system register accessors
  arm64: KVM: VHE: Enable minimal sysreg save/restore
  arm64: KVM: VHE: Make __fpsimd_enabled VHE aware
  arm64: KVM: VHE: Implement VHE activate/deactivate_traps
  arm64: KVM: VHE: Use unified sysreg accessors for timer
  arm64: KVM: VHE: Add fpsimd enabling on guest access
  arm64: KVM: VHE: Add alternative panic handling
  arm64: KVM: Move most of the fault decoding to C
  arm64: perf: Count EL2 events if the kernel is running in HYP
  arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP
  arm64: VHE: Add support for running Linux in EL2 mode
  arm64: Panic when VHE and non VHE CPUs coexist

 arch/arm/include/asm/kvm_host.h        |   4 +
 arch/arm/include/asm/virt.h            |   5 +
 arch/arm/kvm/arm.c                     | 174 ++++++++++++++++++----------
 arch/arm/kvm/mmu.c                     |   7 ++
 arch/arm64/Kconfig                     |  13 +++
 arch/arm64/include/asm/cpufeature.h    |   3 +-
 arch/arm64/include/asm/hw_breakpoint.h |  49 +++++---
 arch/arm64/include/asm/kvm_arm.h       |   6 +-
 arch/arm64/include/asm/kvm_asm.h       |   2 +
 arch/arm64/include/asm/kvm_emulate.h   |   3 +
 arch/arm64/include/asm/kvm_host.h      |   6 +
 arch/arm64/include/asm/kvm_mmu.h       |  12 +-
 arch/arm64/include/asm/virt.h          |  27 +++++
 arch/arm64/kernel/asm-offsets.c        |   3 -
 arch/arm64/kernel/cpufeature.c         |  11 ++
 arch/arm64/kernel/head.S               |  50 +++++++-
 arch/arm64/kernel/perf_event.c         |  14 ++-
 arch/arm64/kernel/smp.c                |   3 +
 arch/arm64/kvm/hyp-init.S              |  18 ---
 arch/arm64/kvm/hyp.S                   |   7 ++
 arch/arm64/kvm/hyp/Makefile            |   1 +
 arch/arm64/kvm/hyp/entry.S             |   6 +
 arch/arm64/kvm/hyp/hyp-entry.S         | 109 ++++++------------
 arch/arm64/kvm/hyp/hyp.h               | 108 ++++++++++++++++--
 arch/arm64/kvm/hyp/s2-setup.c          |  44 +++++++
 arch/arm64/kvm/hyp/switch.c            | 202 +++++++++++++++++++++++++++++----
 arch/arm64/kvm/hyp/sysreg-sr.c         | 147 ++++++++++++++++--------
 arch/arm64/kvm/hyp/timer-sr.c          |  10 +-
 drivers/clocksource/arm_arch_timer.c   |  96 ++++++++++------
 29 files changed, 842 insertions(+), 298 deletions(-)
 create mode 100644 arch/arm64/kvm/hyp/s2-setup.c

-- 
2.1.4

[toc] | [next] | [standalone]

#1332311 — [PATCH v4 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP

With VHE, we place kernel {watch,break}-points at EL2 to get things
like kgdb and "perf -e mem:..." working.

This requires a bit of repainting in the low-level encore/decode,
but is otherwise pretty simple.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/hw_breakpoint.h | 49 +++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
index 9732908..4d8d5a8 100644
--- a/arch/arm64/include/asm/hw_breakpoint.h
+++ b/arch/arm64/include/asm/hw_breakpoint.h
@@ -18,6 +18,7 @@
 
 #include <asm/cputype.h>
 #include <asm/cpufeature.h>
+#include <asm/virt.h>
 
 #ifdef __KERNEL__
 
@@ -35,24 +36,6 @@ struct arch_hw_breakpoint {
 	struct arch_hw_breakpoint_ctrl ctrl;
 };
 
-static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
-{
-	return (ctrl.len << 5) | (ctrl.type << 3) | (ctrl.privilege << 1) |
-		ctrl.enabled;
-}
-
-static inline void decode_ctrl_reg(u32 reg,
-				   struct arch_hw_breakpoint_ctrl *ctrl)
-{
-	ctrl->enabled	= reg & 0x1;
-	reg >>= 1;
-	ctrl->privilege	= reg & 0x3;
-	reg >>= 2;
-	ctrl->type	= reg & 0x3;
-	reg >>= 2;
-	ctrl->len	= reg & 0xff;
-}
-
 /* Breakpoint */
 #define ARM_BREAKPOINT_EXECUTE	0
 
@@ -62,6 +45,7 @@ static inline void decode_ctrl_reg(u32 reg,
 #define AARCH64_ESR_ACCESS_MASK	(1 << 6)
 
 /* Privilege Levels */
+#define AARCH64_BREAKPOINT_EL2	0
 #define AARCH64_BREAKPOINT_EL1	1
 #define AARCH64_BREAKPOINT_EL0	2
 
@@ -76,6 +60,35 @@ static inline void decode_ctrl_reg(u32 reg,
 #define ARM_KERNEL_STEP_ACTIVE	1
 #define ARM_KERNEL_STEP_SUSPEND	2
 
+#define DBG_HMC_HYP		(1 << 13)
+#define DBG_SSC_HYP		(3 << 14)
+
+static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
+{
+	u32 val = (ctrl.len << 5) | (ctrl.type << 3) | ctrl.enabled;
+
+	if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1)
+		val |= DBG_HMC_HYP | DBG_SSC_HYP | (AARCH64_BREAKPOINT_EL2 << 1);
+	else
+		val |= ctrl.privilege << 1;
+
+	return val;
+}
+
+static inline void decode_ctrl_reg(u32 reg,
+				   struct arch_hw_breakpoint_ctrl *ctrl)
+{
+	ctrl->enabled	= reg & 0x1;
+	reg >>= 1;
+	ctrl->privilege	= reg & 0x3;
+	if (ctrl->privilege == AARCH64_BREAKPOINT_EL2)
+		ctrl->privilege	= AARCH64_BREAKPOINT_EL1;
+	reg >>= 2;
+	ctrl->type	= reg & 0x3;
+	reg >>= 2;
+	ctrl->len	= reg & 0xff;
+}
+
 /*
  * Limits.
  * Changing these will require modifications to the register accessors.
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1334351 — Re: [PATCH v4 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP

On Thu, Feb 11, 2016 at 06:40:02PM +0000, Marc Zyngier wrote:
> With VHE, we place kernel {watch,break}-points at EL2 to get things
> like kgdb and "perf -e mem:..." working.
> 
> This requires a bit of repainting in the low-level encore/decode,
> but is otherwise pretty simple.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

To the best of my knowledge, this patch is fine ;)

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

[toc] | [prev] | [next] | [standalone]

#1334667 — Re: [PATCH v4 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP

On Thu, Feb 11, 2016 at 06:40:02PM +0000, Marc Zyngier wrote:
> With VHE, we place kernel {watch,break}-points at EL2 to get things
> like kgdb and "perf -e mem:..." working.
> 
> This requires a bit of repainting in the low-level encore/decode,
> but is otherwise pretty simple.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/include/asm/hw_breakpoint.h | 49 +++++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
> index 9732908..4d8d5a8 100644
> --- a/arch/arm64/include/asm/hw_breakpoint.h
> +++ b/arch/arm64/include/asm/hw_breakpoint.h
> @@ -18,6 +18,7 @@
>  
>  #include <asm/cputype.h>
>  #include <asm/cpufeature.h>
> +#include <asm/virt.h>
>  
>  #ifdef __KERNEL__
>  
> @@ -35,24 +36,6 @@ struct arch_hw_breakpoint {
>  	struct arch_hw_breakpoint_ctrl ctrl;
>  };
>  
> -static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
> -{
> -	return (ctrl.len << 5) | (ctrl.type << 3) | (ctrl.privilege << 1) |
> -		ctrl.enabled;
> -}
> -
> -static inline void decode_ctrl_reg(u32 reg,
> -				   struct arch_hw_breakpoint_ctrl *ctrl)
> -{
> -	ctrl->enabled	= reg & 0x1;
> -	reg >>= 1;
> -	ctrl->privilege	= reg & 0x3;
> -	reg >>= 2;
> -	ctrl->type	= reg & 0x3;
> -	reg >>= 2;
> -	ctrl->len	= reg & 0xff;
> -}
> -
>  /* Breakpoint */
>  #define ARM_BREAKPOINT_EXECUTE	0
>  
> @@ -62,6 +45,7 @@ static inline void decode_ctrl_reg(u32 reg,
>  #define AARCH64_ESR_ACCESS_MASK	(1 << 6)
>  
>  /* Privilege Levels */
> +#define AARCH64_BREAKPOINT_EL2	0
>  #define AARCH64_BREAKPOINT_EL1	1
>  #define AARCH64_BREAKPOINT_EL0	2
>  
> @@ -76,6 +60,35 @@ static inline void decode_ctrl_reg(u32 reg,
>  #define ARM_KERNEL_STEP_ACTIVE	1
>  #define ARM_KERNEL_STEP_SUSPEND	2
>  
> +#define DBG_HMC_HYP		(1 << 13)
> +#define DBG_SSC_HYP		(3 << 14)

Why do we need to touch the SSC field at all?

> +
> +static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
> +{
> +	u32 val = (ctrl.len << 5) | (ctrl.type << 3) | ctrl.enabled;
> +
> +	if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1)
> +		val |= DBG_HMC_HYP | DBG_SSC_HYP | (AARCH64_BREAKPOINT_EL2 << 1);

I don't think this is correct. We want to allow, for example, a userspace
watchpoint to fire thanks to something like put_user, so the encoding
really needs to build up the PMC field (like we do already), then orr in
the HMC field.

The "gotcha", which is similar to the PMU stuff, is that you can't have
HMC==1 (EL2) and PMC==2 (i.e. EL2 and EL0, but not EL1).

I *think* the conclusion is that you need AARCH64_BREAKPOINT_EL2 to look
like DBG_HMC_HYP | AARCH64_BREAKPOINT_EL1.

Will

[toc] | [prev] | [next] | [standalone]

#1334750 — Re: [PATCH v4 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP

On Mon, Feb 15, 2016 at 05:46:56PM +0000, Will Deacon wrote:
> On Thu, Feb 11, 2016 at 06:40:02PM +0000, Marc Zyngier wrote:
> > With VHE, we place kernel {watch,break}-points at EL2 to get things
> > like kgdb and "perf -e mem:..." working.
> > 
> > This requires a bit of repainting in the low-level encore/decode,
> > but is otherwise pretty simple.
> > 
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  arch/arm64/include/asm/hw_breakpoint.h | 49 +++++++++++++++++++++-------------
> >  1 file changed, 31 insertions(+), 18 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
> > index 9732908..4d8d5a8 100644
> > --- a/arch/arm64/include/asm/hw_breakpoint.h
> > +++ b/arch/arm64/include/asm/hw_breakpoint.h
> > @@ -18,6 +18,7 @@
> >  
> >  #include <asm/cputype.h>
> >  #include <asm/cpufeature.h>
> > +#include <asm/virt.h>
> >  
> >  #ifdef __KERNEL__
> >  
> > @@ -35,24 +36,6 @@ struct arch_hw_breakpoint {
> >  	struct arch_hw_breakpoint_ctrl ctrl;
> >  };
> >  
> > -static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
> > -{
> > -	return (ctrl.len << 5) | (ctrl.type << 3) | (ctrl.privilege << 1) |
> > -		ctrl.enabled;
> > -}
> > -
> > -static inline void decode_ctrl_reg(u32 reg,
> > -				   struct arch_hw_breakpoint_ctrl *ctrl)
> > -{
> > -	ctrl->enabled	= reg & 0x1;
> > -	reg >>= 1;
> > -	ctrl->privilege	= reg & 0x3;
> > -	reg >>= 2;
> > -	ctrl->type	= reg & 0x3;
> > -	reg >>= 2;
> > -	ctrl->len	= reg & 0xff;
> > -}
> > -
> >  /* Breakpoint */
> >  #define ARM_BREAKPOINT_EXECUTE	0
> >  
> > @@ -62,6 +45,7 @@ static inline void decode_ctrl_reg(u32 reg,
> >  #define AARCH64_ESR_ACCESS_MASK	(1 << 6)
> >  
> >  /* Privilege Levels */
> > +#define AARCH64_BREAKPOINT_EL2	0
> >  #define AARCH64_BREAKPOINT_EL1	1
> >  #define AARCH64_BREAKPOINT_EL0	2
> >  
> > @@ -76,6 +60,35 @@ static inline void decode_ctrl_reg(u32 reg,
> >  #define ARM_KERNEL_STEP_ACTIVE	1
> >  #define ARM_KERNEL_STEP_SUSPEND	2
> >  
> > +#define DBG_HMC_HYP		(1 << 13)
> > +#define DBG_SSC_HYP		(3 << 14)
> 
> Why do we need to touch the SSC field at all?
> 
> > +
> > +static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
> > +{
> > +	u32 val = (ctrl.len << 5) | (ctrl.type << 3) | ctrl.enabled;
> > +
> > +	if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1)
> > +		val |= DBG_HMC_HYP | DBG_SSC_HYP | (AARCH64_BREAKPOINT_EL2 << 1);
> 
> I don't think this is correct. We want to allow, for example, a userspace
> watchpoint to fire thanks to something like put_user, so the encoding
> really needs to build up the PMC field (like we do already), then orr in
> the HMC field.

Hmm, I got my arm and my arm64 mixed up here. For the latter, we don't
actually support EL0+EL1 watchpoints, but I still think that the
{HMC,SSC,PMC} encoding of {1,00,xx} is cleaner.

Will

[toc] | [prev] | [next] | [standalone]

#1335225 — Re: [PATCH v4 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP

On 15/02/16 19:07, Will Deacon wrote:
> On Mon, Feb 15, 2016 at 05:46:56PM +0000, Will Deacon wrote:
>> On Thu, Feb 11, 2016 at 06:40:02PM +0000, Marc Zyngier wrote:
>>> With VHE, we place kernel {watch,break}-points at EL2 to get things
>>> like kgdb and "perf -e mem:..." working.
>>>
>>> This requires a bit of repainting in the low-level encore/decode,
>>> but is otherwise pretty simple.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>>  arch/arm64/include/asm/hw_breakpoint.h | 49 +++++++++++++++++++++-------------
>>>  1 file changed, 31 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
>>> index 9732908..4d8d5a8 100644
>>> --- a/arch/arm64/include/asm/hw_breakpoint.h
>>> +++ b/arch/arm64/include/asm/hw_breakpoint.h
>>> @@ -18,6 +18,7 @@
>>>  
>>>  #include <asm/cputype.h>
>>>  #include <asm/cpufeature.h>
>>> +#include <asm/virt.h>
>>>  
>>>  #ifdef __KERNEL__
>>>  
>>> @@ -35,24 +36,6 @@ struct arch_hw_breakpoint {
>>>  	struct arch_hw_breakpoint_ctrl ctrl;
>>>  };
>>>  
>>> -static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
>>> -{
>>> -	return (ctrl.len << 5) | (ctrl.type << 3) | (ctrl.privilege << 1) |
>>> -		ctrl.enabled;
>>> -}
>>> -
>>> -static inline void decode_ctrl_reg(u32 reg,
>>> -				   struct arch_hw_breakpoint_ctrl *ctrl)
>>> -{
>>> -	ctrl->enabled	= reg & 0x1;
>>> -	reg >>= 1;
>>> -	ctrl->privilege	= reg & 0x3;
>>> -	reg >>= 2;
>>> -	ctrl->type	= reg & 0x3;
>>> -	reg >>= 2;
>>> -	ctrl->len	= reg & 0xff;
>>> -}
>>> -
>>>  /* Breakpoint */
>>>  #define ARM_BREAKPOINT_EXECUTE	0
>>>  
>>> @@ -62,6 +45,7 @@ static inline void decode_ctrl_reg(u32 reg,
>>>  #define AARCH64_ESR_ACCESS_MASK	(1 << 6)
>>>  
>>>  /* Privilege Levels */
>>> +#define AARCH64_BREAKPOINT_EL2	0
>>>  #define AARCH64_BREAKPOINT_EL1	1
>>>  #define AARCH64_BREAKPOINT_EL0	2
>>>  
>>> @@ -76,6 +60,35 @@ static inline void decode_ctrl_reg(u32 reg,
>>>  #define ARM_KERNEL_STEP_ACTIVE	1
>>>  #define ARM_KERNEL_STEP_SUSPEND	2
>>>  
>>> +#define DBG_HMC_HYP		(1 << 13)
>>> +#define DBG_SSC_HYP		(3 << 14)
>>
>> Why do we need to touch the SSC field at all?
>>
>>> +
>>> +static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
>>> +{
>>> +	u32 val = (ctrl.len << 5) | (ctrl.type << 3) | ctrl.enabled;
>>> +
>>> +	if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1)
>>> +		val |= DBG_HMC_HYP | DBG_SSC_HYP | (AARCH64_BREAKPOINT_EL2 << 1);
>>
>> I don't think this is correct. We want to allow, for example, a userspace
>> watchpoint to fire thanks to something like put_user, so the encoding
>> really needs to build up the PMC field (like we do already), then orr in
>> the HMC field.
> 
> Hmm, I got my arm and my arm64 mixed up here. For the latter, we don't
> actually support EL0+EL1 watchpoints, but I still think that the
> {HMC,SSC,PMC} encoding of {1,00,xx} is cleaner.

Indeed. So with that in mind, I ended up with the following patch, which is much simpler than the previous one:

diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
index 9732908..115ea2a 100644
--- a/arch/arm64/include/asm/hw_breakpoint.h
+++ b/arch/arm64/include/asm/hw_breakpoint.h
@@ -18,6 +18,7 @@
 
 #include <asm/cputype.h>
 #include <asm/cpufeature.h>
+#include <asm/virt.h>
 
 #ifdef __KERNEL__
 
@@ -35,10 +36,21 @@ struct arch_hw_breakpoint {
 	struct arch_hw_breakpoint_ctrl ctrl;
 };
 
+/* Privilege Levels */
+#define AARCH64_BREAKPOINT_EL1	1
+#define AARCH64_BREAKPOINT_EL0	2
+
+#define DBG_HMC_HYP		(1 << 13)
+
 static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl)
 {
-	return (ctrl.len << 5) | (ctrl.type << 3) | (ctrl.privilege << 1) |
+	u32 val = (ctrl.len << 5) | (ctrl.type << 3) | (ctrl.privilege << 1) |
 		ctrl.enabled;
+
+	if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1)
+		val |= DBG_HMC_HYP;
+
+	return val;
 }
 
 static inline void decode_ctrl_reg(u32 reg,
@@ -61,10 +73,6 @@ static inline void decode_ctrl_reg(u32 reg,
 #define ARM_BREAKPOINT_STORE	2
 #define AARCH64_ESR_ACCESS_MASK	(1 << 6)
 
-/* Privilege Levels */
-#define AARCH64_BREAKPOINT_EL1	1
-#define AARCH64_BREAKPOINT_EL0	2
-
 /* Lengths */
 #define ARM_BREAKPOINT_LEN_1	0x1
 #define ARM_BREAKPOINT_LEN_2	0x3


Thanks,
	
	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1336613 — [PATCH v4.1] arm64: perf: Count EL2 events if the kernel is running in HYP

When the kernel is running in HYP (with VHE), it is necessary to
include EL2 events if the user requests counting kernel or
hypervisor events.

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kernel/perf_event.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index f7ab14c..1b52269 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -20,6 +20,7 @@
  */
 
 #include <asm/irq_regs.h>
+#include <asm/virt.h>
 
 #include <linux/of.h>
 #include <linux/perf/arm_pmu.h>
@@ -691,9 +692,12 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
 
 	if (attr->exclude_idle)
 		return -EPERM;
+	if (is_kernel_in_hyp_mode() &&
+	    attr->exclude_kernel != attr->exclude_hv)
+		return -EINVAL;
 	if (attr->exclude_user)
 		config_base |= ARMV8_EXCLUDE_EL0;
-	if (attr->exclude_kernel)
+	if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
 		config_base |= ARMV8_EXCLUDE_EL1;
 	if (!attr->exclude_hv)
 		config_base |= ARMV8_INCLUDE_EL2;
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1338238 — Re: [PATCH v4.1] arm64: perf: Count EL2 events if the kernel is running in HYP

On Wed, Feb 17, 2016 at 05:57:39PM +0000, Marc Zyngier wrote:
> When the kernel is running in HYP (with VHE), it is necessary to
> include EL2 events if the user requests counting kernel or
> hypervisor events.
> 
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kernel/perf_event.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Acked-by: Will Deacon <will.deacon@arm.com>

Will

[toc] | [prev] | [next] | [standalone]

#1332312 — [PATCH v4 19/23] arm64: KVM: Move most of the fault decoding to C

The fault decoding process (including computing the IPA in the case
of a permission fault) would be much better done in C code, as we
have a reasonable infrastructure to deal with the VHE/non-VHE
differences.

Let's move the whole thing to C, including the workaround for
erratum 834220, and just patch the odd ESR_EL2 access remaining
in hyp-entry.S.

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kernel/asm-offsets.c |  3 --
 arch/arm64/kvm/hyp/hyp-entry.S  | 69 +++------------------------------
 arch/arm64/kvm/hyp/switch.c     | 85 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 90 insertions(+), 67 deletions(-)

diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index fffa4ac6..b0ab4e9 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -110,9 +110,6 @@ int main(void)
   DEFINE(CPU_USER_PT_REGS,	offsetof(struct kvm_regs, regs));
   DEFINE(CPU_FP_REGS,		offsetof(struct kvm_regs, fp_regs));
   DEFINE(VCPU_FPEXC32_EL2,	offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2]));
-  DEFINE(VCPU_ESR_EL2,		offsetof(struct kvm_vcpu, arch.fault.esr_el2));
-  DEFINE(VCPU_FAR_EL2,		offsetof(struct kvm_vcpu, arch.fault.far_el2));
-  DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
   DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
 #endif
 #ifdef CONFIG_CPU_PM
diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index 1bdeee7..3488894 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -19,7 +19,6 @@
 
 #include <asm/alternative.h>
 #include <asm/assembler.h>
-#include <asm/asm-offsets.h>
 #include <asm/cpufeature.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_asm.h>
@@ -69,7 +68,11 @@ ENDPROC(__vhe_hyp_call)
 el1_sync:				// Guest trapped into EL2
 	save_x0_to_x3
 
+alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
 	mrs	x1, esr_el2
+alternative_else
+	mrs	x1, esr_el1
+alternative_endif
 	lsr	x2, x1, #ESR_ELx_EC_SHIFT
 
 	cmp	x2, #ESR_ELx_EC_HVC64
@@ -105,72 +108,10 @@ el1_trap:
 	cmp	x2, #ESR_ELx_EC_FP_ASIMD
 	b.eq	__fpsimd_guest_restore
 
-	cmp	x2, #ESR_ELx_EC_DABT_LOW
-	mov	x0, #ESR_ELx_EC_IABT_LOW
-	ccmp	x2, x0, #4, ne
-	b.ne	1f		// 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.ne	1f		// 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
-
-	/*
-	 * Check for Stage-1 page table walk, which is guaranteed
-	 * to give a valid HPFAR_EL2.
-	 */
-	tbnz	x1, #7, 1f	// S1PTW is set
-
-	/* Preserve PAR_EL1 */
-	mrs	x3, par_el1
-	stp	x3, xzr, [sp, #-16]!
-
-	/*
-	 * Permission fault, HPFAR_EL2 is invalid.
-	 * Resolve the IPA the hard way using the guest VA.
-	 * Stage-1 translation already validated the memory access rights.
-	 * As such, we can use the EL1 translation regime, and don't have
-	 * to distinguish between EL0 and EL1 access.
-	 */
-	mrs	x2, far_el2
-	at	s1e1r, x2
-	isb
-
-	/* Read result */
-	mrs	x3, par_el1
-	ldp	x0, xzr, [sp], #16	// Restore PAR_EL1 from the stack
-	msr	par_el1, x0
-	tbnz	x3, #0, 3f		// Bail out if we failed the translation
-	ubfx	x3, x3, #12, #36	// Extract IPA
-	lsl	x3, x3, #4		// and present it like HPFAR
-	b	2f
-
-1:	mrs	x3, hpfar_el2
-	mrs	x2, far_el2
-
-2:	mrs	x0, tpidr_el2
-	str	w1, [x0, #VCPU_ESR_EL2]
-	str	x2, [x0, #VCPU_FAR_EL2]
-	str	x3, [x0, #VCPU_HPFAR_EL2]
-
+	mrs	x0, tpidr_el2
 	mov	x1, #ARM_EXCEPTION_TRAP
 	b	__guest_exit
 
-	/*
-	 * Translation failed. Just return to the guest and
-	 * let it fault again. Another CPU is probably playing
-	 * behind our back.
-	 */
-3:	restore_x0_to_x3
-
-	eret
-
 el1_irq:
 	save_x0_to_x3
 	mrs	x0, tpidr_el2
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 731f0a2..ecf5b05 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -15,6 +15,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/types.h>
 #include <asm/kvm_asm.h>
 
 #include "hyp.h"
@@ -149,6 +150,86 @@ static void __hyp_text __vgic_restore_state(struct kvm_vcpu *vcpu)
 	__vgic_call_restore_state()(vcpu);
 }
 
+static bool __hyp_text __true_value(void)
+{
+	return true;
+}
+
+static bool __hyp_text __false_value(void)
+{
+	return false;
+}
+
+static hyp_alternate_select(__check_arm_834220,
+			    __false_value, __true_value,
+			    ARM64_WORKAROUND_834220);
+
+static bool __hyp_text __translate_far_to_hpfar(u64 far, u64 *hpfar)
+{
+	u64 par, tmp;
+
+	/*
+	 * Resolve the IPA the hard way using the guest VA.
+	 *
+	 * Stage-1 translation already validated the memory access
+	 * rights. As such, we can use the EL1 translation regime, and
+	 * don't have to distinguish between EL0 and EL1 access.
+	 *
+	 * We do need to save/restore PAR_EL1 though, as we haven't
+	 * saved the guest context yet, and we may return early...
+	 */
+	par = read_sysreg(par_el1);
+	asm volatile("at s1e1r, %0" : : "r" (far));
+	isb();
+
+	tmp = read_sysreg(par_el1);
+	write_sysreg(par, par_el1);
+
+	if (unlikely(tmp & 1))
+		return false; /* Translation failed, back to guest */
+
+	/* Convert PAR to HPFAR format */
+	*hpfar = ((tmp >> 12) & ((1UL << 36) - 1)) << 4;
+	return true;
+}
+
+static bool __hyp_text __populate_fault_info(struct kvm_vcpu *vcpu)
+{
+	u64 esr = read_sysreg_el2(esr);
+	u8 ec = esr >> ESR_ELx_EC_SHIFT;
+	u64 hpfar, far;
+
+	vcpu->arch.fault.esr_el2 = esr;
+
+	if (ec != ESR_ELx_EC_DABT_LOW && ec != ESR_ELx_EC_IABT_LOW)
+		return true;
+
+	far = read_sysreg_el2(far);
+
+	/*
+	 * The HPFAR can be invalid if the stage 2 fault did not
+	 * happen during a stage 1 page table walk (the ESR_EL2.S1PTW
+	 * bit is clear) and one of the two following cases are true:
+	 *   1. The fault was due to a permission fault
+	 *   2. The processor carries errata 834220
+	 *
+	 * Therefore, for all non S1PTW faults where we either have a
+	 * permission fault or the errata workaround is enabled, we
+	 * resolve the IPA using the AT instruction.
+	 */
+	if (!(esr & ESR_ELx_S1PTW) &&
+	    (__check_arm_834220()() || (esr & ESR_ELx_FSC_TYPE) == FSC_PERM)) {
+		if (!__translate_far_to_hpfar(far, &hpfar))
+			return false;
+	} else {
+		hpfar = read_sysreg(hpfar_el2);
+	}
+
+	vcpu->arch.fault.far_el2 = far;
+	vcpu->arch.fault.hpfar_el2 = hpfar;
+	return true;
+}
+
 static int __hyp_text __guest_run(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *host_ctxt;
@@ -180,9 +261,13 @@ static int __hyp_text __guest_run(struct kvm_vcpu *vcpu)
 	__debug_restore_state(vcpu, kern_hyp_va(vcpu->arch.debug_ptr), guest_ctxt);
 
 	/* Jump in the fire! */
+again:
 	exit_code = __guest_enter(vcpu, host_ctxt);
 	/* And we're baaack! */
 
+	if (exit_code == ARM_EXCEPTION_TRAP && !__populate_fault_info(vcpu))
+		goto again;
+
 	fp_enabled = __fpsimd_enabled();
 
 	__sysreg_save_guest_state(guest_ctxt);
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1332313 — [PATCH v4 20/23] arm64: perf: Count EL2 events if the kernel is running in HYP

When the kernel is running in HYP (with VHE), it is necessary to
include EL2 events if the user requests counting kernel or
hypervisor events.

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kernel/perf_event.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index f7ab14c..6013a38 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -20,6 +20,7 @@
  */
 
 #include <asm/irq_regs.h>
+#include <asm/virt.h>
 
 #include <linux/of.h>
 #include <linux/perf/arm_pmu.h>
@@ -693,10 +694,15 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
 		return -EPERM;
 	if (attr->exclude_user)
 		config_base |= ARMV8_EXCLUDE_EL0;
-	if (attr->exclude_kernel)
-		config_base |= ARMV8_EXCLUDE_EL1;
-	if (!attr->exclude_hv)
-		config_base |= ARMV8_INCLUDE_EL2;
+	if (is_kernel_in_hyp_mode()) {
+		if (!attr->exclude_kernel || !attr->exclude_hv)
+			config_base |= ARMV8_INCLUDE_EL2;
+	} else {
+		if (attr->exclude_kernel)
+			config_base |= ARMV8_EXCLUDE_EL1;
+		if (!attr->exclude_hv)
+			config_base |= ARMV8_INCLUDE_EL2;
+	}
 
 	/*
 	 * Install the filter into config_base as this is used to
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1334646 — Re: [PATCH v4 20/23] arm64: perf: Count EL2 events if the kernel is running in HYP

On Thu, Feb 11, 2016 at 06:40:01PM +0000, Marc Zyngier wrote:
> When the kernel is running in HYP (with VHE), it is necessary to
> include EL2 events if the user requests counting kernel or
> hypervisor events.
> 
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kernel/perf_event.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index f7ab14c..6013a38 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -20,6 +20,7 @@
>   */
>  
>  #include <asm/irq_regs.h>
> +#include <asm/virt.h>
>  
>  #include <linux/of.h>
>  #include <linux/perf/arm_pmu.h>
> @@ -693,10 +694,15 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>  		return -EPERM;
>  	if (attr->exclude_user)
>  		config_base |= ARMV8_EXCLUDE_EL0;
> -	if (attr->exclude_kernel)
> -		config_base |= ARMV8_EXCLUDE_EL1;
> -	if (!attr->exclude_hv)
> -		config_base |= ARMV8_INCLUDE_EL2;
> +	if (is_kernel_in_hyp_mode()) {
> +		if (!attr->exclude_kernel || !attr->exclude_hv)
> +			config_base |= ARMV8_INCLUDE_EL2;

Hmm, so if userspace sets exclude_kernel but not exclude_user and
exclude_hv, what should we do? I'm slightly tempted to reject the
filter with -EINVAL...

Will

[toc] | [prev] | [next] | [standalone]

#1334701 — Re: [PATCH v4 20/23] arm64: perf: Count EL2 events if the kernel is running in HYP

On Mon, Feb 15, 2016 at 06:23:35PM +0000, Marc Zyngier wrote:
> On 15/02/16 17:22, Will Deacon wrote:
> > On Thu, Feb 11, 2016 at 06:40:01PM +0000, Marc Zyngier wrote:
> >> When the kernel is running in HYP (with VHE), it is necessary to
> >> include EL2 events if the user requests counting kernel or
> >> hypervisor events.
> >>
> >> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
> >> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  arch/arm64/kernel/perf_event.c | 14 ++++++++++----
> >>  1 file changed, 10 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> >> index f7ab14c..6013a38 100644
> >> --- a/arch/arm64/kernel/perf_event.c
> >> +++ b/arch/arm64/kernel/perf_event.c
> >> @@ -20,6 +20,7 @@
> >>   */
> >>  
> >>  #include <asm/irq_regs.h>
> >> +#include <asm/virt.h>
> >>  
> >>  #include <linux/of.h>
> >>  #include <linux/perf/arm_pmu.h>
> >> @@ -693,10 +694,15 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
> >>  		return -EPERM;
> >>  	if (attr->exclude_user)
> >>  		config_base |= ARMV8_EXCLUDE_EL0;
> >> -	if (attr->exclude_kernel)
> >> -		config_base |= ARMV8_EXCLUDE_EL1;
> >> -	if (!attr->exclude_hv)
> >> -		config_base |= ARMV8_INCLUDE_EL2;
> >> +	if (is_kernel_in_hyp_mode()) {
> >> +		if (!attr->exclude_kernel || !attr->exclude_hv)
> >> +			config_base |= ARMV8_INCLUDE_EL2;
> > 
> > Hmm, so if userspace sets exclude_kernel but not exclude_user and
> > exclude_hv, what should we do? I'm slightly tempted to reject the
> > filter with -EINVAL...
> 
> I was angling for the minimum level of surprise for the user, but
> I guess that the results are going to be troubling anyway.
> 
> How about something like this on top:
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index 6013a38..8c00ed4 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -695,7 +695,10 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>  	if (attr->exclude_user)
>  		config_base |= ARMV8_EXCLUDE_EL0;
>  	if (is_kernel_in_hyp_mode()) {
> -		if (!attr->exclude_kernel || !attr->exclude_hv)
> +		/* Demand that kernel and hv are consistent */
> +		if (attr->exclude_kernel != attr->exclude_hv)
> +			return -EINVAL;
> +		if (!attr->exclude_hv)
>  			config_base |= ARMV8_INCLUDE_EL2;
>  	} else {
>  		if (attr->exclude_kernel)

Looks like the right idea, and you can probably refactor things slightly
to avoid having two codepaths dealing with ARMV8_INCLUDE_EL2.

Note that I've got a couple of outstanding questions with the architects
that I should have answers to later in the week.

Will

[toc] | [prev] | [next] | [standalone]

#1334707 — Re: [PATCH v4 20/23] arm64: perf: Count EL2 events if the kernel is running in HYP

On 15/02/16 17:22, Will Deacon wrote:
> On Thu, Feb 11, 2016 at 06:40:01PM +0000, Marc Zyngier wrote:
>> When the kernel is running in HYP (with VHE), it is necessary to
>> include EL2 events if the user requests counting kernel or
>> hypervisor events.
>>
>> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
>> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/kernel/perf_event.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index f7ab14c..6013a38 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -20,6 +20,7 @@
>>   */
>>  
>>  #include <asm/irq_regs.h>
>> +#include <asm/virt.h>
>>  
>>  #include <linux/of.h>
>>  #include <linux/perf/arm_pmu.h>
>> @@ -693,10 +694,15 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>>  		return -EPERM;
>>  	if (attr->exclude_user)
>>  		config_base |= ARMV8_EXCLUDE_EL0;
>> -	if (attr->exclude_kernel)
>> -		config_base |= ARMV8_EXCLUDE_EL1;
>> -	if (!attr->exclude_hv)
>> -		config_base |= ARMV8_INCLUDE_EL2;
>> +	if (is_kernel_in_hyp_mode()) {
>> +		if (!attr->exclude_kernel || !attr->exclude_hv)
>> +			config_base |= ARMV8_INCLUDE_EL2;
> 
> Hmm, so if userspace sets exclude_kernel but not exclude_user and
> exclude_hv, what should we do? I'm slightly tempted to reject the
> filter with -EINVAL...

I was angling for the minimum level of surprise for the user, but
I guess that the results are going to be troubling anyway.

How about something like this on top:

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 6013a38..8c00ed4 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -695,7 +695,10 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
 	if (attr->exclude_user)
 		config_base |= ARMV8_EXCLUDE_EL0;
 	if (is_kernel_in_hyp_mode()) {
-		if (!attr->exclude_kernel || !attr->exclude_hv)
+		/* Demand that kernel and hv are consistent */
+		if (attr->exclude_kernel != attr->exclude_hv)
+			return -EINVAL;
+		if (!attr->exclude_hv)
 			config_base |= ARMV8_INCLUDE_EL2;
 	} else {
 		if (attr->exclude_kernel)

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1332314 — [PATCH v4 14/23] arm64: KVM: VHE: Make __fpsimd_enabled VHE aware

As non-VHE and VHE have different ways to express the trapping of
FPSIMD registers to EL2, make __fpsimd_enabled a patchable predicate
and provide a VHE implementation.

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/kvm_arm.h |  3 +++
 arch/arm64/kvm/hyp/hyp.h         |  5 +----
 arch/arm64/kvm/hyp/switch.c      | 19 +++++++++++++++++++
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index bef6e92..f416714 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -218,4 +218,7 @@
 	ECN(SOFTSTP_CUR), ECN(WATCHPT_LOW), ECN(WATCHPT_CUR), \
 	ECN(BKPT32), ECN(VECTOR32), ECN(BRK64)
 
+#define CPACR_EL1_FPEN		(3 << 20)
+#define CPACR_EL1_TTA		(1 << 28)
+
 #endif /* __ARM64_KVM_ARM_H__ */
diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h
index 5dfa883..44eaff7 100644
--- a/arch/arm64/kvm/hyp/hyp.h
+++ b/arch/arm64/kvm/hyp/hyp.h
@@ -171,10 +171,7 @@ void __debug_cond_restore_host_state(struct kvm_vcpu *vcpu);
 
 void __fpsimd_save_state(struct user_fpsimd_state *fp_regs);
 void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs);
-static inline bool __fpsimd_enabled(void)
-{
-	return !(read_sysreg(cptr_el2) & CPTR_EL2_TFP);
-}
+bool __fpsimd_enabled(void);
 
 u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host_ctxt);
 void __noreturn __hyp_do_panic(unsigned long, ...);
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 68f3cba..0d82ae9 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -17,6 +17,25 @@
 
 #include "hyp.h"
 
+static bool __hyp_text __fpsimd_enabled_nvhe(void)
+{
+	return !(read_sysreg(cptr_el2) & CPTR_EL2_TFP);
+}
+
+static bool __hyp_text __fpsimd_enabled_vhe(void)
+{
+	return !!(read_sysreg(cpacr_el1) & CPACR_EL1_FPEN);
+}
+
+static hyp_alternate_select(__fpsimd_is_enabled,
+			    __fpsimd_enabled_nvhe, __fpsimd_enabled_vhe,
+			    ARM64_HAS_VIRT_HOST_EXTN);
+
+bool __hyp_text __fpsimd_enabled(void)
+{
+	return __fpsimd_is_enabled()();
+}
+
 static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
 {
 	u64 val;
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1332315 — [PATCH v4 13/23] arm64: KVM: VHE: Enable minimal sysreg save/restore

We're now in a position where we can introduce VHE's minimal
save/restore, which is limited to the handful of shared sysregs.

Add the required alternative function calls that result in a
"do nothing" call on VHE, and the normal save/restore for non-VHE.

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/hyp/sysreg-sr.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 7d7d757..74b5f81 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -23,6 +23,9 @@
 
 #include "hyp.h"
 
+/* Yes, this does nothing, on purpose */
+static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { }
+
 /*
  * Non-VHE: Both host and guest must save everything.
  *
@@ -67,9 +70,13 @@ static void __hyp_text __sysreg_save_state(struct kvm_cpu_context *ctxt)
 	ctxt->gp_regs.spsr[KVM_SPSR_EL1]= read_sysreg_el1(spsr);
 }
 
+static hyp_alternate_select(__sysreg_call_save_host_state,
+			    __sysreg_save_state, __sysreg_do_nothing,
+			    ARM64_HAS_VIRT_HOST_EXTN);
+
 void __hyp_text __sysreg_save_host_state(struct kvm_cpu_context *ctxt)
 {
-	__sysreg_save_state(ctxt);
+	__sysreg_call_save_host_state()(ctxt);
 	__sysreg_save_common_state(ctxt);
 }
 
@@ -116,9 +123,13 @@ static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt)
 	write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr);
 }
 
+static hyp_alternate_select(__sysreg_call_restore_host_state,
+			    __sysreg_restore_state, __sysreg_do_nothing,
+			    ARM64_HAS_VIRT_HOST_EXTN);
+
 void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt)
 {
-	__sysreg_restore_state(ctxt);
+	__sysreg_call_restore_host_state()(ctxt);
 	__sysreg_restore_common_state(ctxt);
 }
 
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1332322

On Thu, Feb 11, 2016 at 06:39:41PM +0000, Marc Zyngier wrote:
> ARMv8.1 comes with the "Virtualization Host Extension" (VHE for
> short), which enables simpler support of Type-2 hypervisors.
> 
> This extension allows the kernel to directly run at EL2, and
> significantly reduces the number of system registers shared between
> host and guest, reducing the overhead of virtualization.
> 
> In order to have the same kernel binary running on all versions of the
> architecture, this series makes heavy use of runtime code patching.
> 
> The first 22 patches massage the KVM code to deal with VHE and enable
> Linux to run at EL2. The last patch catches an ugly case when VHE
> capable CPUs are paired with some of their less capable siblings. This
> should never happen, but hey...
> 
> I have deliberately left out some of the more "advanced"
> optimizations, as they are likely to distract the reviewer from the
> core infrastructure, which is what I care about at the moment.
> 
> Note: GDB is currently busted on VHE systems, as it checks for version
>       6 on the debug architecture, while VHE is version 7. The
>       binutils people are on the case.
> 
> This has been tested on the FVP_Base_SLV-V8-A model, and based on
> v4.5-rc3 + kvmarm/master. I've put a branch out on:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git kvm-arm64/vhe

I'm happy with this series as it stands, we just need someone to take a
look at that debug patch and be happy with it.

Thanks,
-Christoffer

[toc] | [prev] | [next] | [standalone]

#1332325

On 11/02/16 19:07, Christoffer Dall wrote:
> On Thu, Feb 11, 2016 at 06:39:41PM +0000, Marc Zyngier wrote:
>> ARMv8.1 comes with the "Virtualization Host Extension" (VHE for
>> short), which enables simpler support of Type-2 hypervisors.
>>
>> This extension allows the kernel to directly run at EL2, and
>> significantly reduces the number of system registers shared between
>> host and guest, reducing the overhead of virtualization.
>>
>> In order to have the same kernel binary running on all versions of the
>> architecture, this series makes heavy use of runtime code patching.
>>
>> The first 22 patches massage the KVM code to deal with VHE and enable
>> Linux to run at EL2. The last patch catches an ugly case when VHE
>> capable CPUs are paired with some of their less capable siblings. This
>> should never happen, but hey...
>>
>> I have deliberately left out some of the more "advanced"
>> optimizations, as they are likely to distract the reviewer from the
>> core infrastructure, which is what I care about at the moment.
>>
>> Note: GDB is currently busted on VHE systems, as it checks for version
>>       6 on the debug architecture, while VHE is version 7. The
>>       binutils people are on the case.
>>
>> This has been tested on the FVP_Base_SLV-V8-A model, and based on
>> v4.5-rc3 + kvmarm/master. I've put a branch out on:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git kvm-arm64/vhe
> 
> I'm happy with this series as it stands, we just need someone to take a
> look at that debug patch and be happy with it.

I think Will is planning to have a go at it shortly. Once he and Catalin
are happy with the thing, I'll throw it into -next for a good shake.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1334354

On Thu, Feb 11, 2016 at 07:10:50PM +0000, Marc Zyngier wrote:
> On 11/02/16 19:07, Christoffer Dall wrote:
> > On Thu, Feb 11, 2016 at 06:39:41PM +0000, Marc Zyngier wrote:
> >> ARMv8.1 comes with the "Virtualization Host Extension" (VHE for
> >> short), which enables simpler support of Type-2 hypervisors.
> >>
> >> This extension allows the kernel to directly run at EL2, and
> >> significantly reduces the number of system registers shared between
> >> host and guest, reducing the overhead of virtualization.
> >>
> >> In order to have the same kernel binary running on all versions of the
> >> architecture, this series makes heavy use of runtime code patching.
> >>
> >> The first 22 patches massage the KVM code to deal with VHE and enable
> >> Linux to run at EL2. The last patch catches an ugly case when VHE
> >> capable CPUs are paired with some of their less capable siblings. This
> >> should never happen, but hey...
> >>
> >> I have deliberately left out some of the more "advanced"
> >> optimizations, as they are likely to distract the reviewer from the
> >> core infrastructure, which is what I care about at the moment.
> >>
> >> Note: GDB is currently busted on VHE systems, as it checks for version
> >>       6 on the debug architecture, while VHE is version 7. The
> >>       binutils people are on the case.
> >>
> >> This has been tested on the FVP_Base_SLV-V8-A model, and based on
> >> v4.5-rc3 + kvmarm/master. I've put a branch out on:
> >>
> >> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git kvm-arm64/vhe
> > 
> > I'm happy with this series as it stands, we just need someone to take a
> > look at that debug patch and be happy with it.
> 
> I think Will is planning to have a go at it shortly. Once he and Catalin
> are happy with the thing, I'll throw it into -next for a good shake.

Fine by me but it would be good for Will to have a look, especially on
the debug and perf stuff.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1334363

On 15/02/16 10:23, Catalin Marinas wrote:
> On Thu, Feb 11, 2016 at 07:10:50PM +0000, Marc Zyngier wrote:
>> On 11/02/16 19:07, Christoffer Dall wrote:
>>> On Thu, Feb 11, 2016 at 06:39:41PM +0000, Marc Zyngier wrote:
>>>> ARMv8.1 comes with the "Virtualization Host Extension" (VHE for
>>>> short), which enables simpler support of Type-2 hypervisors.
>>>>
>>>> This extension allows the kernel to directly run at EL2, and
>>>> significantly reduces the number of system registers shared between
>>>> host and guest, reducing the overhead of virtualization.
>>>>
>>>> In order to have the same kernel binary running on all versions of the
>>>> architecture, this series makes heavy use of runtime code patching.
>>>>
>>>> The first 22 patches massage the KVM code to deal with VHE and enable
>>>> Linux to run at EL2. The last patch catches an ugly case when VHE
>>>> capable CPUs are paired with some of their less capable siblings. This
>>>> should never happen, but hey...
>>>>
>>>> I have deliberately left out some of the more "advanced"
>>>> optimizations, as they are likely to distract the reviewer from the
>>>> core infrastructure, which is what I care about at the moment.
>>>>
>>>> Note: GDB is currently busted on VHE systems, as it checks for version
>>>>       6 on the debug architecture, while VHE is version 7. The
>>>>       binutils people are on the case.
>>>>
>>>> This has been tested on the FVP_Base_SLV-V8-A model, and based on
>>>> v4.5-rc3 + kvmarm/master. I've put a branch out on:
>>>>
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git kvm-arm64/vhe
>>>
>>> I'm happy with this series as it stands, we just need someone to take a
>>> look at that debug patch and be happy with it.
>>
>> I think Will is planning to have a go at it shortly. Once he and Catalin
>> are happy with the thing, I'll throw it into -next for a good shake.
> 
> Fine by me but it would be good for Will to have a look, especially on
> the debug and perf stuff.

Definitely. To make it clear, I do not intend to put anything in -next
until all the relevant people has reviewed it, and at least given their Ack.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web