Re: [PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-16 Thread Suzuki K Poulose 

On 16/01/17 14:11, Marc Zyngier wrote:

On 16/01/17 13:30, Marc Zyngier wrote:

On 13/01/17 14:56, Suzuki K Poulose wrote:

On 13/01/17 13:30, Marc Zyngier wrote:

[+ Suzuki, who wrote the whole cpus_have_const_cap thing]



[...]


But maybe we should have have some stronger guarantees that we'll
always get things inlined, and that the "const" side is enforced:


Agreed.



diff --git a/arch/arm64/include/asm/cpufeature.h 
b/arch/arm64/include/asm/cpufeature.h
index b4989df..4710469 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -105,10 +105,11 @@ static inline bool cpu_have_feature(unsigned int num)
 }

 /* System capability check for constant caps */
-static inline bool cpus_have_const_cap(int num)
+static __always_inline bool cpus_have_const_cap(int num)


I think we should have the above change and make it inline always.


 {
-   if (num >= ARM64_NCAPS)
-   return false;
+   BUILD_BUG_ON(!__builtin_constant_p(num));


This is not needed, as the compilation would fail if num is not a constant with
static key code.


I also just checked this, and it doesn't fail if the compiler doesn't
directly supports jump labels (we then fallback to the static key being
a standard memory access).


Ah, I missed that part of the story. Sorry about that. Please go ahead with the
changes. I had a similar check in my first version and was dropped later with a
similar review comment. We hadn't considered older tool chain.


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

Re: [PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-16 Thread Marc Zyngier 
On 16/01/17 13:30, Marc Zyngier wrote:
> On 13/01/17 14:56, Suzuki K Poulose wrote:
>> On 13/01/17 13:30, Marc Zyngier wrote:
>>> [+ Suzuki, who wrote the whole cpus_have_const_cap thing]
>>>
> 
> [...]
> 
>>> But maybe we should have have some stronger guarantees that we'll
>>> always get things inlined, and that the "const" side is enforced:
>>
>> Agreed.
>>
>>>
>>> diff --git a/arch/arm64/include/asm/cpufeature.h 
>>> b/arch/arm64/include/asm/cpufeature.h
>>> index b4989df..4710469 100644
>>> --- a/arch/arm64/include/asm/cpufeature.h
>>> +++ b/arch/arm64/include/asm/cpufeature.h
>>> @@ -105,10 +105,11 @@ static inline bool cpu_have_feature(unsigned int num)
>>>  }
>>>
>>>  /* System capability check for constant caps */
>>> -static inline bool cpus_have_const_cap(int num)
>>> +static __always_inline bool cpus_have_const_cap(int num)
>>
>> I think we should have the above change and make it inline always.
>>
>>>  {
>>> -   if (num >= ARM64_NCAPS)
>>> -   return false;
>>> +   BUILD_BUG_ON(!__builtin_constant_p(num));
>>
>> This is not needed, as the compilation would fail if num is not a constant 
>> with
>> static key code.

I also just checked this, and it doesn't fail if the compiler doesn't
directly supports jump labels (we then fallback to the static key being
a standard memory access).

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-16 Thread Marc Zyngier 
On 13/01/17 14:56, Suzuki K Poulose wrote:
> On 13/01/17 13:30, Marc Zyngier wrote:
>> [+ Suzuki, who wrote the whole cpus_have_const_cap thing]
>>

[...]

>> But maybe we should have have some stronger guarantees that we'll
>> always get things inlined, and that the "const" side is enforced:
> 
> Agreed.
> 
>>
>> diff --git a/arch/arm64/include/asm/cpufeature.h 
>> b/arch/arm64/include/asm/cpufeature.h
>> index b4989df..4710469 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -105,10 +105,11 @@ static inline bool cpu_have_feature(unsigned int num)
>>  }
>>
>>  /* System capability check for constant caps */
>> -static inline bool cpus_have_const_cap(int num)
>> +static __always_inline bool cpus_have_const_cap(int num)
> 
> I think we should have the above change and make it inline always.
> 
>>  {
>> -if (num >= ARM64_NCAPS)
>> -return false;
>> +BUILD_BUG_ON(!__builtin_constant_p(num));
> 
> This is not needed, as the compilation would fail if num is not a constant 
> with
> static key code.
> 
>> +BUILD_BUG_ON(num >= ARM64_NCAPS);
>> +
> 
> Also, I think it would be good to return false for caps > the ARM64_NCAPS, in 
> sync
> with the non-const version.

But what's the semantic? It means we're accessing a capability that
doesn't exist, which looks like a major bug in my book. Is there any
valid use case for this?

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-13 Thread Jintack Lim 
On Fri, Jan 13, 2017 at 9:56 AM, Suzuki K Poulose
 wrote:
> On 13/01/17 13:30, Marc Zyngier wrote:
>>
>> [+ Suzuki, who wrote the whole cpus_have_const_cap thing]
>>
>> On 13/01/17 12:36, Christoffer Dall wrote:
>>>
>>> On Fri, Jan 13, 2017 at 11:31:32AM +, Marc Zyngier wrote:

 From: Jintack Lim 

> ...
>
>
  /*
   * __boot_cpu_mode records what mode CPUs were booted in.
 @@ -80,6 +81,14 @@ static inline bool is_kernel_in_hyp_mode(void)
 return read_sysreg(CurrentEL) == CurrentEL_EL2;
  }

 +static inline bool has_vhe(void)
 +{
 +   if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
 +   return true;
 +
 +   return false;
 +}
 +
>>>
>>>
>>> I was experimenting with using has_vhe for some of the optimization code
>>> I was writing, and I saw a hyp crash as a result.  That made me wonder
>>> if this is really safe in Hyp mode?
>>>
>>> Specifically, there is no guarantee that this will actually be inlined
>>> in the caller, right?  At least that's what I can gather from trying to
>>> understand the semantics of the inline keyword in the GCC manual.
>>
>>
>> Indeed, there is no strict guarantee that this is enforced. We should
>> probably have __always_inline instead. But having checked the generated
>> code for __timer_restore_state, the function is definitely inlined
>> (gcc 6.2). Happy to queue an extra patch changing that.
>>
>>> Further, are we guaranteed that the static branch gets compiled into
>>> something that doesn't actually look at cpu_hwcap_keys, which is not
>>> mapped in hyp mode?
>>
>>
>> Here's the disassembly:
>>
>> 08ad01d0 <__timer_restore_state>:
>> 08ad01d0:   f941ldr x1, [x0]
>> 08ad01d4:   9240bc21and x1, x1, #0x
>> 08ad01d8:   d503201fnop
>> 08ad01dc:   d503201fnop
>> 08ad01e0:   d53ce102mrs x2, cnthctl_el2
>> 08ad01e4:   927ef842and x2, x2,
>> #0xfffd
>> 08ad01e8:   b2400042orr x2, x2, #0x1
>> 08ad01ec:   d51ce102msr cnthctl_el2, x2
>> 08ad01f0:   d2834002mov x2, #0x1a00
>> // #6656
>> 08ad01f4:   8b02add x0, x0, x2
>> 08ad01f8:   91038002add x2, x0, #0xe0
>> 08ad01fc:   39425443ldrbw3, [x2,#149]
>> 08ad0200:   34000103cbz w3, 08ad0220
>> <__timer_restore_state+0x50>
>> 08ad0204:   f945a821ldr x1, [x1,#2896]
>> 08ad0208:   d51ce061msr cntvoff_el2, x1
>> 08ad020c:   f9400441ldr x1, [x2,#8]
>> 08ad0210:   d51be341msr cntv_cval_el0, x1
>> 08ad0214:   d5033fdfisb
>> 08ad0218:   b940e000ldr w0, [x0,#224]
>> 08ad021c:   d51be320msr cntv_ctl_el0, x0
>> 08ad0220:   d65f03c0ret
>>
>> The static branch resolves as such when VHE is enabled (taken from
>> a running model):
>>
>> 08ad01d0 <__timer_restore_state>:
>> 08ad01d0:   f941ldr x1, [x0]
>> 08ad01d4:   9240bc21nop
>> 08ad01d8:   d503201fnop
>> 08ad01dc:   d503201fb   08ad01f0
>> 08ad01e0:   d53ce102mrs x2, cnthctl_el2
>> [...]
>>
>> That's using a toolchain that supports the "asm goto" feature that is used
>> to implement static branches (and that's guaranteed not to generate any
>> memory access other than the code patching itself).
>>
>> Now, with a toolchain that doesn't support this, such as gcc 4.8:
>>
>> 08aa5168 <__timer_restore_state>:
>> 08aa5168:   f941ldr x1, [x0]
>> 08aa516c:   9240bc21and x1, x1, #0x
>> 08aa5170:   d503201fnop
>> 08aa5174:   f00038a2adrpx2, 091bc000
>> 
>> 08aa5178:   9113e042add x2, x2, #0x4f8
>> 08aa517c:   b9402c42ldr w2, [x2,#44]
>> 08aa5180:   6b1f005fcmp w2, wzr
>> 08aa5184:   54acb.gt08aa5198
>> <__timer_restore_state+0x30>
>> 08aa5188:   d53ce102mrs x2, cnthctl_el2
>> 08aa518c:   927ef842and x2, x2,
>> #0xfffd
>> 08aa5190:   b2400042orr x2, x2, #0x1
>> 08aa5194:   d51ce102msr cnthctl_el2, x2
>> 08aa5198:   91400402add x2, x0, #0x1, lsl #12
>> 08aa519c:   396dd443ldrbw3, [x2,#2933]
>> 08aa51a0:   34000103cbz w3, 08aa51c0
>> <__timer_restore_state+0x58>
>> 08aa51a4:   f945a821

Re: [PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-13 Thread Mark Rutland 
On Fri, Jan 13, 2017 at 02:42:04PM +, Mark Rutland wrote:
> On Fri, Jan 13, 2017 at 01:30:29PM +, Marc Zyngier wrote:
> > On 13/01/17 12:36, Christoffer Dall wrote:
> > > Further, are we guaranteed that the static branch gets compiled into
> > > something that doesn't actually look at cpu_hwcap_keys, which is not
> > > mapped in hyp mode?
> 
> If I disable CONFIG_JUMP_LABEL (which lives under "General setup", with
> teh title "Optimize very unlikely/likely branches"), I see adrp; add;
> ldr sequences accessing cpu_hwcap_keys when using cpus_have_const_cap()
> in hyp code, even with the patch below.

Looking again, that's the same sequence Marc mentioned, as it falls in
the BSS. I just happened to be looking at the unlinked .o file rather
than the vmlinux.

Sorry for the noise.

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

Re: [PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-13 Thread Suzuki K Poulose 

On 13/01/17 13:30, Marc Zyngier wrote:

[+ Suzuki, who wrote the whole cpus_have_const_cap thing]

On 13/01/17 12:36, Christoffer Dall wrote:

On Fri, Jan 13, 2017 at 11:31:32AM +, Marc Zyngier wrote:

From: Jintack Lim 


...


 /*
  * __boot_cpu_mode records what mode CPUs were booted in.
@@ -80,6 +81,14 @@ static inline bool is_kernel_in_hyp_mode(void)
return read_sysreg(CurrentEL) == CurrentEL_EL2;
 }

+static inline bool has_vhe(void)
+{
+   if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
+   return true;
+
+   return false;
+}
+


I was experimenting with using has_vhe for some of the optimization code
I was writing, and I saw a hyp crash as a result.  That made me wonder
if this is really safe in Hyp mode?

Specifically, there is no guarantee that this will actually be inlined
in the caller, right?  At least that's what I can gather from trying to
understand the semantics of the inline keyword in the GCC manual.


Indeed, there is no strict guarantee that this is enforced. We should
probably have __always_inline instead. But having checked the generated
code for __timer_restore_state, the function is definitely inlined
(gcc 6.2). Happy to queue an extra patch changing that.


Further, are we guaranteed that the static branch gets compiled into
something that doesn't actually look at cpu_hwcap_keys, which is not
mapped in hyp mode?


Here's the disassembly:

08ad01d0 <__timer_restore_state>:
08ad01d0:   f941ldr x1, [x0]
08ad01d4:   9240bc21and x1, x1, #0x
08ad01d8:   d503201fnop
08ad01dc:   d503201fnop
08ad01e0:   d53ce102mrs x2, cnthctl_el2
08ad01e4:   927ef842and x2, x2, #0xfffd
08ad01e8:   b2400042orr x2, x2, #0x1
08ad01ec:   d51ce102msr cnthctl_el2, x2
08ad01f0:   d2834002mov x2, #0x1a00 
// #6656
08ad01f4:   8b02add x0, x0, x2
08ad01f8:   91038002add x2, x0, #0xe0
08ad01fc:   39425443ldrbw3, [x2,#149]
08ad0200:   34000103cbz w3, 08ad0220 
<__timer_restore_state+0x50>
08ad0204:   f945a821ldr x1, [x1,#2896]
08ad0208:   d51ce061msr cntvoff_el2, x1
08ad020c:   f9400441ldr x1, [x2,#8]
08ad0210:   d51be341msr cntv_cval_el0, x1
08ad0214:   d5033fdfisb
08ad0218:   b940e000ldr w0, [x0,#224]
08ad021c:   d51be320msr cntv_ctl_el0, x0
08ad0220:   d65f03c0ret

The static branch resolves as such when VHE is enabled (taken from
a running model):

08ad01d0 <__timer_restore_state>:
08ad01d0:   f941ldr x1, [x0]
08ad01d4:   9240bc21nop
08ad01d8:   d503201fnop
08ad01dc:   d503201fb   08ad01f0
08ad01e0:   d53ce102mrs x2, cnthctl_el2
[...]

That's using a toolchain that supports the "asm goto" feature that is used
to implement static branches (and that's guaranteed not to generate any
memory access other than the code patching itself).

Now, with a toolchain that doesn't support this, such as gcc 4.8:

08aa5168 <__timer_restore_state>:
08aa5168:   f941ldr x1, [x0]
08aa516c:   9240bc21and x1, x1, #0x
08aa5170:   d503201fnop
08aa5174:   f00038a2adrpx2, 091bc000 

08aa5178:   9113e042add x2, x2, #0x4f8
08aa517c:   b9402c42ldr w2, [x2,#44]
08aa5180:   6b1f005fcmp w2, wzr
08aa5184:   54acb.gt08aa5198 
<__timer_restore_state+0x30>
08aa5188:   d53ce102mrs x2, cnthctl_el2
08aa518c:   927ef842and x2, x2, #0xfffd
08aa5190:   b2400042orr x2, x2, #0x1
08aa5194:   d51ce102msr cnthctl_el2, x2
08aa5198:   91400402add x2, x0, #0x1, lsl #12
08aa519c:   396dd443ldrbw3, [x2,#2933]
08aa51a0:   34000103cbz w3, 08aa51c0 
<__timer_restore_state+0x58>
08aa51a4:   f945a821ldr x1, [x1,#2896]
08aa51a8:   d51ce061msr cntvoff_el2, x1
08aa51ac:   f9457441ldr x1, [x2,#2792]
08aa51b0:   d51be341msr cntv_cval_el0, x1
08aa51b4:   d5033fdfisb
08aa51b8:   b95ae000ldr w0, [x0,#6880]
08aa51bc:   d51be320msr cntv_ctl_el0, x0
08aa51c0:

Re: [PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-13 Thread Mark Rutland 
On Fri, Jan 13, 2017 at 01:36:12PM +0100, Christoffer Dall wrote:
> On Fri, Jan 13, 2017 at 11:31:32AM +, Marc Zyngier wrote:

> Further, are we guaranteed that the static branch gets compiled into
> something that doesn't actually look at cpu_hwcap_keys, which is not
> mapped in hyp mode?

The fact that this might happen silently seems to be a larger problem.

Can we do something like the EFI stub, and ensure that (unintentional)
references to symbols outside of the hyp-stub will fail to link? That's
ensrue by some symbol mangling in drivers/firmware/efi/libstub/Makefile.

I think this may have come up before; I can't recall if there was some
reason that was problematic.

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

Re: [PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-13 Thread Mark Rutland 
Hi,

On Fri, Jan 13, 2017 at 01:30:29PM +, Marc Zyngier wrote:
> [+ Suzuki, who wrote the whole cpus_have_const_cap thing]
> 
> On 13/01/17 12:36, Christoffer Dall wrote:
> > On Fri, Jan 13, 2017 at 11:31:32AM +, Marc Zyngier wrote:

> >> +static inline bool has_vhe(void)
> >> +{
> >> +  if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
> >> +  return true;
> >> +
> >> +  return false;
> >> +}
> >> +
> > 
> > I was experimenting with using has_vhe for some of the optimization code
> > I was writing, and I saw a hyp crash as a result.  That made me wonder
> > if this is really safe in Hyp mode?
> > 
> > Specifically, there is no guarantee that this will actually be inlined
> > in the caller, right?  At least that's what I can gather from trying to
> > understand the semantics of the inline keyword in the GCC manual.
> 
> Indeed, there is no strict guarantee that this is enforced. We should
> probably have __always_inline instead. But having checked the generated
> code for __timer_restore_state, the function is definitely inlined
> (gcc 6.2). Happy to queue an extra patch changing that.

> > Further, are we guaranteed that the static branch gets compiled into
> > something that doesn't actually look at cpu_hwcap_keys, which is not
> > mapped in hyp mode?

If I disable CONFIG_JUMP_LABEL (which lives under "General setup", with
teh title "Optimize very unlikely/likely branches"), I see adrp; add;
ldr sequences accessing cpu_hwcap_keys when using cpus_have_const_cap()
in hyp code, even with the patch below.

Do we have the whole kernel image mapped around hyp, so that this would
work by relative offset? Do we have a guarantee that adrp+add is used?

Thanks,
Mark.

> diff --git a/arch/arm64/include/asm/cpufeature.h 
> b/arch/arm64/include/asm/cpufeature.h
> index b4989df..4710469 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -105,10 +105,11 @@ static inline bool cpu_have_feature(unsigned int num)
>  }
>  
>  /* System capability check for constant caps */
> -static inline bool cpus_have_const_cap(int num)
> +static __always_inline bool cpus_have_const_cap(int num)
>  {
> - if (num >= ARM64_NCAPS)
> - return false;
> + BUILD_BUG_ON(!__builtin_constant_p(num));
> + BUILD_BUG_ON(num >= ARM64_NCAPS);
> +
>   return static_branch_unlikely(_hwcap_keys[num]);
>  }
>  
> diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
> index 439f6b5..1257701 100644
> --- a/arch/arm64/include/asm/virt.h
> +++ b/arch/arm64/include/asm/virt.h
> @@ -81,7 +81,7 @@ static inline bool is_kernel_in_hyp_mode(void)
>   return read_sysreg(CurrentEL) == CurrentEL_EL2;
>  }
>  
> -static inline bool has_vhe(void)
> +static __always_inline bool has_vhe(void)
>  {
>   if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
>   return true;
> 
> 
> But that's probably another patch or two. Thoughts?
> 
>   M.
> -- 
> Jazz is not dead. It just smells funny...
> ___
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-13 Thread Christoffer Dall 
On Fri, Jan 13, 2017 at 01:57:23PM +, Marc Zyngier wrote:
> On 13/01/17 13:46, Christoffer Dall wrote:
> > On Fri, Jan 13, 2017 at 01:30:29PM +, Marc Zyngier wrote:
> >> [+ Suzuki, who wrote the whole cpus_have_const_cap thing]
> >>
> >> On 13/01/17 12:36, Christoffer Dall wrote:
> >>> On Fri, Jan 13, 2017 at 11:31:32AM +, Marc Zyngier wrote:
>  From: Jintack Lim 
> 
>  Current KVM world switch code is unintentionally setting wrong bits to
>  CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
>  timer.  Bit positions of CNTHCTL_EL2 are changing depending on
>  HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
>  not set, but they are 11th and 10th bits respectively when E2H is set.
> 
>  In fact, on VHE we only need to set those bits once, not for every world
>  switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
>  1, which makes those bits have no effect for the host kernel execution.
>  So we just set those bits once for guests, and that's it.
> 
>  Signed-off-by: Jintack Lim 
>  Reviewed-by: Marc Zyngier 
>  Signed-off-by: Marc Zyngier 
>  ---
>   arch/arm/include/asm/virt.h   |  5 +
>   arch/arm/kvm/arm.c|  3 +++
>   arch/arm64/include/asm/virt.h |  9 +
>   include/kvm/arm_arch_timer.h  |  1 +
>   virt/kvm/arm/arch_timer.c | 23 +++
>   virt/kvm/arm/hyp/timer-sr.c   | 33 +
>   6 files changed, 62 insertions(+), 12 deletions(-)
> 
>  diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
>  index a2e75b8..6dae195 100644
>  --- a/arch/arm/include/asm/virt.h
>  +++ b/arch/arm/include/asm/virt.h
>  @@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
>   return false;
>   }
>   
>  +static inline bool has_vhe(void)
>  +{
>  +return false;
>  +}
>  +
>   /* The section containing the hypervisor idmap text */
>   extern char __hyp_idmap_text_start[];
>   extern char __hyp_idmap_text_end[];
>  diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>  index 1167678..9d74464 100644
>  --- a/arch/arm/kvm/arm.c
>  +++ b/arch/arm/kvm/arm.c
>  @@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
>   __cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
>   __cpu_init_stage2();
>   
>  +if (is_kernel_in_hyp_mode())
>  +kvm_timer_init_vhe();
>  +
>   kvm_arm_init_debug();
>   }
>   
>  diff --git a/arch/arm64/include/asm/virt.h 
>  b/arch/arm64/include/asm/virt.h
>  index fea1073..439f6b5 100644
>  --- a/arch/arm64/include/asm/virt.h
>  +++ b/arch/arm64/include/asm/virt.h
>  @@ -47,6 +47,7 @@
>   #include 
>   #include 
>   #include 
>  +#include 
>   
>   /*
>    * __boot_cpu_mode records what mode CPUs were booted in.
>  @@ -80,6 +81,14 @@ static inline bool is_kernel_in_hyp_mode(void)
>   return read_sysreg(CurrentEL) == CurrentEL_EL2;
>   }
>   
>  +static inline bool has_vhe(void)
>  +{
>  +if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
>  +return true;
>  +
>  +return false;
>  +}
>  +
> >>>
> >>> I was experimenting with using has_vhe for some of the optimization code
> >>> I was writing, and I saw a hyp crash as a result.  That made me wonder
> >>> if this is really safe in Hyp mode?
> >>>
> >>> Specifically, there is no guarantee that this will actually be inlined
> >>> in the caller, right?  At least that's what I can gather from trying to
> >>> understand the semantics of the inline keyword in the GCC manual.
> >>
> >> Indeed, there is no strict guarantee that this is enforced. We should
> >> probably have __always_inline instead. But having checked the generated
> >> code for __timer_restore_state, the function is definitely inlined
> >> (gcc 6.2). Happy to queue an extra patch changing that.
> >>
> >>> Further, are we guaranteed that the static branch gets compiled into
> >>> something that doesn't actually look at cpu_hwcap_keys, which is not
> >>> mapped in hyp mode?
> >>
> >> Here's the disassembly:
> >>
> >> 08ad01d0 <__timer_restore_state>:
> >> 08ad01d0:   f941ldr x1, [x0]
> >> 08ad01d4:   9240bc21and x1, x1, #0x
> >> 08ad01d8:   d503201fnop
> >> 08ad01dc:   d503201fnop
> >> 08ad01e0:   d53ce102mrs x2, cnthctl_el2
> >> 08ad01e4:   927ef842and x2, x2, #0xfffd
> >> 08ad01e8:   b2400042orr x2, x2, #0x1
>

Re: [PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-13 Thread Marc Zyngier 
On 13/01/17 13:46, Christoffer Dall wrote:
> On Fri, Jan 13, 2017 at 01:30:29PM +, Marc Zyngier wrote:
>> [+ Suzuki, who wrote the whole cpus_have_const_cap thing]
>>
>> On 13/01/17 12:36, Christoffer Dall wrote:
>>> On Fri, Jan 13, 2017 at 11:31:32AM +, Marc Zyngier wrote:
 From: Jintack Lim 

 Current KVM world switch code is unintentionally setting wrong bits to
 CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
 timer.  Bit positions of CNTHCTL_EL2 are changing depending on
 HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
 not set, but they are 11th and 10th bits respectively when E2H is set.

 In fact, on VHE we only need to set those bits once, not for every world
 switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
 1, which makes those bits have no effect for the host kernel execution.
 So we just set those bits once for guests, and that's it.

 Signed-off-by: Jintack Lim 
 Reviewed-by: Marc Zyngier 
 Signed-off-by: Marc Zyngier 
 ---
  arch/arm/include/asm/virt.h   |  5 +
  arch/arm/kvm/arm.c|  3 +++
  arch/arm64/include/asm/virt.h |  9 +
  include/kvm/arm_arch_timer.h  |  1 +
  virt/kvm/arm/arch_timer.c | 23 +++
  virt/kvm/arm/hyp/timer-sr.c   | 33 +
  6 files changed, 62 insertions(+), 12 deletions(-)

 diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
 index a2e75b8..6dae195 100644
 --- a/arch/arm/include/asm/virt.h
 +++ b/arch/arm/include/asm/virt.h
 @@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
return false;
  }
  
 +static inline bool has_vhe(void)
 +{
 +  return false;
 +}
 +
  /* The section containing the hypervisor idmap text */
  extern char __hyp_idmap_text_start[];
  extern char __hyp_idmap_text_end[];
 diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
 index 1167678..9d74464 100644
 --- a/arch/arm/kvm/arm.c
 +++ b/arch/arm/kvm/arm.c
 @@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
__cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
__cpu_init_stage2();
  
 +  if (is_kernel_in_hyp_mode())
 +  kvm_timer_init_vhe();
 +
kvm_arm_init_debug();
  }
  
 diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
 index fea1073..439f6b5 100644
 --- a/arch/arm64/include/asm/virt.h
 +++ b/arch/arm64/include/asm/virt.h
 @@ -47,6 +47,7 @@
  #include 
  #include 
  #include 
 +#include 
  
  /*
   * __boot_cpu_mode records what mode CPUs were booted in.
 @@ -80,6 +81,14 @@ static inline bool is_kernel_in_hyp_mode(void)
return read_sysreg(CurrentEL) == CurrentEL_EL2;
  }
  
 +static inline bool has_vhe(void)
 +{
 +  if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
 +  return true;
 +
 +  return false;
 +}
 +
>>>
>>> I was experimenting with using has_vhe for some of the optimization code
>>> I was writing, and I saw a hyp crash as a result.  That made me wonder
>>> if this is really safe in Hyp mode?
>>>
>>> Specifically, there is no guarantee that this will actually be inlined
>>> in the caller, right?  At least that's what I can gather from trying to
>>> understand the semantics of the inline keyword in the GCC manual.
>>
>> Indeed, there is no strict guarantee that this is enforced. We should
>> probably have __always_inline instead. But having checked the generated
>> code for __timer_restore_state, the function is definitely inlined
>> (gcc 6.2). Happy to queue an extra patch changing that.
>>
>>> Further, are we guaranteed that the static branch gets compiled into
>>> something that doesn't actually look at cpu_hwcap_keys, which is not
>>> mapped in hyp mode?
>>
>> Here's the disassembly:
>>
>> 08ad01d0 <__timer_restore_state>:
>> 08ad01d0:   f941ldr x1, [x0]
>> 08ad01d4:   9240bc21and x1, x1, #0x
>> 08ad01d8:   d503201fnop
>> 08ad01dc:   d503201fnop
>> 08ad01e0:   d53ce102mrs x2, cnthctl_el2
>> 08ad01e4:   927ef842and x2, x2, #0xfffd
>> 08ad01e8:   b2400042orr x2, x2, #0x1
>> 08ad01ec:   d51ce102msr cnthctl_el2, x2
>> 08ad01f0:   d2834002mov x2, #0x1a00  
>>// #6656
>> 08ad01f4:   8b02add x0, x0, x2
>> 08ad01f8:   91038002add x2, x0, #0xe0
>> 08ad01fc:   39425443ldrbw3, [x2,#149]
>>

Re: [PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-13 Thread Christoffer Dall 
On Fri, Jan 13, 2017 at 01:30:29PM +, Marc Zyngier wrote:
> [+ Suzuki, who wrote the whole cpus_have_const_cap thing]
> 
> On 13/01/17 12:36, Christoffer Dall wrote:
> > On Fri, Jan 13, 2017 at 11:31:32AM +, Marc Zyngier wrote:
> >> From: Jintack Lim 
> >>
> >> Current KVM world switch code is unintentionally setting wrong bits to
> >> CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
> >> timer.  Bit positions of CNTHCTL_EL2 are changing depending on
> >> HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
> >> not set, but they are 11th and 10th bits respectively when E2H is set.
> >>
> >> In fact, on VHE we only need to set those bits once, not for every world
> >> switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
> >> 1, which makes those bits have no effect for the host kernel execution.
> >> So we just set those bits once for guests, and that's it.
> >>
> >> Signed-off-by: Jintack Lim 
> >> Reviewed-by: Marc Zyngier 
> >> Signed-off-by: Marc Zyngier 
> >> ---
> >>  arch/arm/include/asm/virt.h   |  5 +
> >>  arch/arm/kvm/arm.c|  3 +++
> >>  arch/arm64/include/asm/virt.h |  9 +
> >>  include/kvm/arm_arch_timer.h  |  1 +
> >>  virt/kvm/arm/arch_timer.c | 23 +++
> >>  virt/kvm/arm/hyp/timer-sr.c   | 33 +
> >>  6 files changed, 62 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
> >> index a2e75b8..6dae195 100644
> >> --- a/arch/arm/include/asm/virt.h
> >> +++ b/arch/arm/include/asm/virt.h
> >> @@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
> >>return false;
> >>  }
> >>  
> >> +static inline bool has_vhe(void)
> >> +{
> >> +  return false;
> >> +}
> >> +
> >>  /* The section containing the hypervisor idmap text */
> >>  extern char __hyp_idmap_text_start[];
> >>  extern char __hyp_idmap_text_end[];
> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> >> index 1167678..9d74464 100644
> >> --- a/arch/arm/kvm/arm.c
> >> +++ b/arch/arm/kvm/arm.c
> >> @@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
> >>__cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
> >>__cpu_init_stage2();
> >>  
> >> +  if (is_kernel_in_hyp_mode())
> >> +  kvm_timer_init_vhe();
> >> +
> >>kvm_arm_init_debug();
> >>  }
> >>  
> >> diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
> >> index fea1073..439f6b5 100644
> >> --- a/arch/arm64/include/asm/virt.h
> >> +++ b/arch/arm64/include/asm/virt.h
> >> @@ -47,6 +47,7 @@
> >>  #include 
> >>  #include 
> >>  #include 
> >> +#include 
> >>  
> >>  /*
> >>   * __boot_cpu_mode records what mode CPUs were booted in.
> >> @@ -80,6 +81,14 @@ static inline bool is_kernel_in_hyp_mode(void)
> >>return read_sysreg(CurrentEL) == CurrentEL_EL2;
> >>  }
> >>  
> >> +static inline bool has_vhe(void)
> >> +{
> >> +  if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
> >> +  return true;
> >> +
> >> +  return false;
> >> +}
> >> +
> > 
> > I was experimenting with using has_vhe for some of the optimization code
> > I was writing, and I saw a hyp crash as a result.  That made me wonder
> > if this is really safe in Hyp mode?
> > 
> > Specifically, there is no guarantee that this will actually be inlined
> > in the caller, right?  At least that's what I can gather from trying to
> > understand the semantics of the inline keyword in the GCC manual.
> 
> Indeed, there is no strict guarantee that this is enforced. We should
> probably have __always_inline instead. But having checked the generated
> code for __timer_restore_state, the function is definitely inlined
> (gcc 6.2). Happy to queue an extra patch changing that.
> 
> > Further, are we guaranteed that the static branch gets compiled into
> > something that doesn't actually look at cpu_hwcap_keys, which is not
> > mapped in hyp mode?
> 
> Here's the disassembly:
> 
> 08ad01d0 <__timer_restore_state>:
> 08ad01d0:   f941ldr x1, [x0]
> 08ad01d4:   9240bc21and x1, x1, #0x
> 08ad01d8:   d503201fnop
> 08ad01dc:   d503201fnop
> 08ad01e0:   d53ce102mrs x2, cnthctl_el2
> 08ad01e4:   927ef842and x2, x2, #0xfffd
> 08ad01e8:   b2400042orr x2, x2, #0x1
> 08ad01ec:   d51ce102msr cnthctl_el2, x2
> 08ad01f0:   d2834002mov x2, #0x1a00   
>   // #6656
> 08ad01f4:   8b02add x0, x0, x2
> 08ad01f8:   91038002add x2, x0, #0xe0
> 08ad01fc:   39425443ldrbw3, [x2,#149]
> 08ad0200:   34000103cbz w3, 08ad0220

Re: [PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-13 Thread Marc Zyngier 
[+ Suzuki, who wrote the whole cpus_have_const_cap thing]

On 13/01/17 12:36, Christoffer Dall wrote:
> On Fri, Jan 13, 2017 at 11:31:32AM +, Marc Zyngier wrote:
>> From: Jintack Lim 
>>
>> Current KVM world switch code is unintentionally setting wrong bits to
>> CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
>> timer.  Bit positions of CNTHCTL_EL2 are changing depending on
>> HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
>> not set, but they are 11th and 10th bits respectively when E2H is set.
>>
>> In fact, on VHE we only need to set those bits once, not for every world
>> switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
>> 1, which makes those bits have no effect for the host kernel execution.
>> So we just set those bits once for guests, and that's it.
>>
>> Signed-off-by: Jintack Lim 
>> Reviewed-by: Marc Zyngier 
>> Signed-off-by: Marc Zyngier 
>> ---
>>  arch/arm/include/asm/virt.h   |  5 +
>>  arch/arm/kvm/arm.c|  3 +++
>>  arch/arm64/include/asm/virt.h |  9 +
>>  include/kvm/arm_arch_timer.h  |  1 +
>>  virt/kvm/arm/arch_timer.c | 23 +++
>>  virt/kvm/arm/hyp/timer-sr.c   | 33 +
>>  6 files changed, 62 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
>> index a2e75b8..6dae195 100644
>> --- a/arch/arm/include/asm/virt.h
>> +++ b/arch/arm/include/asm/virt.h
>> @@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
>>  return false;
>>  }
>>  
>> +static inline bool has_vhe(void)
>> +{
>> +return false;
>> +}
>> +
>>  /* The section containing the hypervisor idmap text */
>>  extern char __hyp_idmap_text_start[];
>>  extern char __hyp_idmap_text_end[];
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 1167678..9d74464 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
>>  __cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
>>  __cpu_init_stage2();
>>  
>> +if (is_kernel_in_hyp_mode())
>> +kvm_timer_init_vhe();
>> +
>>  kvm_arm_init_debug();
>>  }
>>  
>> diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
>> index fea1073..439f6b5 100644
>> --- a/arch/arm64/include/asm/virt.h
>> +++ b/arch/arm64/include/asm/virt.h
>> @@ -47,6 +47,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  /*
>>   * __boot_cpu_mode records what mode CPUs were booted in.
>> @@ -80,6 +81,14 @@ static inline bool is_kernel_in_hyp_mode(void)
>>  return read_sysreg(CurrentEL) == CurrentEL_EL2;
>>  }
>>  
>> +static inline bool has_vhe(void)
>> +{
>> +if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
>> +return true;
>> +
>> +return false;
>> +}
>> +
> 
> I was experimenting with using has_vhe for some of the optimization code
> I was writing, and I saw a hyp crash as a result.  That made me wonder
> if this is really safe in Hyp mode?
> 
> Specifically, there is no guarantee that this will actually be inlined
> in the caller, right?  At least that's what I can gather from trying to
> understand the semantics of the inline keyword in the GCC manual.

Indeed, there is no strict guarantee that this is enforced. We should
probably have __always_inline instead. But having checked the generated
code for __timer_restore_state, the function is definitely inlined
(gcc 6.2). Happy to queue an extra patch changing that.

> Further, are we guaranteed that the static branch gets compiled into
> something that doesn't actually look at cpu_hwcap_keys, which is not
> mapped in hyp mode?

Here's the disassembly:

08ad01d0 <__timer_restore_state>:
08ad01d0:   f941ldr x1, [x0]
08ad01d4:   9240bc21and x1, x1, #0x
08ad01d8:   d503201fnop
08ad01dc:   d503201fnop
08ad01e0:   d53ce102mrs x2, cnthctl_el2
08ad01e4:   927ef842and x2, x2, #0xfffd
08ad01e8:   b2400042orr x2, x2, #0x1
08ad01ec:   d51ce102msr cnthctl_el2, x2
08ad01f0:   d2834002mov x2, #0x1a00 
// #6656
08ad01f4:   8b02add x0, x0, x2
08ad01f8:   91038002add x2, x0, #0xe0
08ad01fc:   39425443ldrbw3, [x2,#149]
08ad0200:   34000103cbz w3, 08ad0220 
<__timer_restore_state+0x50>
08ad0204:   f945a821ldr x1, [x1,#2896]
08ad0208:   d51ce061msr cntvoff_el2, x1
08ad020c:   f9400441ldr x1, [x2,#8]
08ad0210:   d51be341msr

Re: [PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-13 Thread Christoffer Dall 
On Fri, Jan 13, 2017 at 11:31:32AM +, Marc Zyngier wrote:
> From: Jintack Lim 
> 
> Current KVM world switch code is unintentionally setting wrong bits to
> CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
> timer.  Bit positions of CNTHCTL_EL2 are changing depending on
> HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
> not set, but they are 11th and 10th bits respectively when E2H is set.
> 
> In fact, on VHE we only need to set those bits once, not for every world
> switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
> 1, which makes those bits have no effect for the host kernel execution.
> So we just set those bits once for guests, and that's it.
> 
> Signed-off-by: Jintack Lim 
> Reviewed-by: Marc Zyngier 
> Signed-off-by: Marc Zyngier 
> ---
>  arch/arm/include/asm/virt.h   |  5 +
>  arch/arm/kvm/arm.c|  3 +++
>  arch/arm64/include/asm/virt.h |  9 +
>  include/kvm/arm_arch_timer.h  |  1 +
>  virt/kvm/arm/arch_timer.c | 23 +++
>  virt/kvm/arm/hyp/timer-sr.c   | 33 +
>  6 files changed, 62 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
> index a2e75b8..6dae195 100644
> --- a/arch/arm/include/asm/virt.h
> +++ b/arch/arm/include/asm/virt.h
> @@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
>   return false;
>  }
>  
> +static inline bool has_vhe(void)
> +{
> + return false;
> +}
> +
>  /* The section containing the hypervisor idmap text */
>  extern char __hyp_idmap_text_start[];
>  extern char __hyp_idmap_text_end[];
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 1167678..9d74464 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
>   __cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
>   __cpu_init_stage2();
>  
> + if (is_kernel_in_hyp_mode())
> + kvm_timer_init_vhe();
> +
>   kvm_arm_init_debug();
>  }
>  
> diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
> index fea1073..439f6b5 100644
> --- a/arch/arm64/include/asm/virt.h
> +++ b/arch/arm64/include/asm/virt.h
> @@ -47,6 +47,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /*
>   * __boot_cpu_mode records what mode CPUs were booted in.
> @@ -80,6 +81,14 @@ static inline bool is_kernel_in_hyp_mode(void)
>   return read_sysreg(CurrentEL) == CurrentEL_EL2;
>  }
>  
> +static inline bool has_vhe(void)
> +{
> + if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
> + return true;
> +
> + return false;
> +}
> +

I was experimenting with using has_vhe for some of the optimization code
I was writing, and I saw a hyp crash as a result.  That made me wonder
if this is really safe in Hyp mode?

Specifically, there is no guarantee that this will actually be inlined
in the caller, right?  At least that's what I can gather from trying to
understand the semantics of the inline keyword in the GCC manual.

Further, are we guaranteed that the static branch gets compiled into
something that doesn't actually look at cpu_hwcap_keys, which is not
mapped in hyp mode?

Thanks,
-Christoffer

>  #ifdef CONFIG_ARM64_VHE
>  extern void verify_cpu_run_el(void);
>  #else
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index b717ed9..5c970ce 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -76,4 +76,5 @@ void kvm_timer_unschedule(struct kvm_vcpu *vcpu);
>  
>  void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
>  
> +void kvm_timer_init_vhe(void);
>  #endif
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index a7fe606..6a084cd 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -24,6 +24,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -509,3 +510,25 @@ void kvm_timer_init(struct kvm *kvm)
>  {
>   kvm->arch.timer.cntvoff = kvm_phys_timer_read();
>  }
> +
> +/*
> + * On VHE system, we only need to configure trap on physical timer and 
> counter
> + * accesses in EL0 and EL1 once, not for every world switch.
> + * The host kernel runs at EL2 with HCR_EL2.TGE == 1,
> + * and this makes those bits have no effect for the host kernel execution.
> + */
> +void kvm_timer_init_vhe(void)
> +{
> + /* When HCR_EL2.E2H ==1, EL1PCEN and EL1PCTEN are shifted by 10 */
> + u32 cnthctl_shift = 10;
> + u64 val;
> +
> + /*
> +  * Disallow physical timer access for the guest.
> +  * Physical counter access is allowed.
> +  */
> + val = read_sysreg(cnthctl_el2);
> + val &= ~(CNTHCTL_EL1PCEN << cnthctl_shift);
> + val |= (CNTHCTL_EL1PCTEN << cnthctl_shift);
> + write_sysreg(val, cnthctl_el2);
> +}

[PATCH 2/3] KVM: arm64: Access CNTHCTL_EL2 bit fields correctly on VHE systems

2017-01-13 Thread Marc Zyngier 
From: Jintack Lim 

Current KVM world switch code is unintentionally setting wrong bits to
CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
timer.  Bit positions of CNTHCTL_EL2 are changing depending on
HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
not set, but they are 11th and 10th bits respectively when E2H is set.

In fact, on VHE we only need to set those bits once, not for every world
switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
1, which makes those bits have no effect for the host kernel execution.
So we just set those bits once for guests, and that's it.

Signed-off-by: Jintack Lim 
Reviewed-by: Marc Zyngier 
Signed-off-by: Marc Zyngier 
---
 arch/arm/include/asm/virt.h   |  5 +
 arch/arm/kvm/arm.c|  3 +++
 arch/arm64/include/asm/virt.h |  9 +
 include/kvm/arm_arch_timer.h  |  1 +
 virt/kvm/arm/arch_timer.c | 23 +++
 virt/kvm/arm/hyp/timer-sr.c   | 33 +
 6 files changed, 62 insertions(+), 12 deletions(-)

diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
index a2e75b8..6dae195 100644
--- a/arch/arm/include/asm/virt.h
+++ b/arch/arm/include/asm/virt.h
@@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
return false;
 }
 
+static inline bool has_vhe(void)
+{
+   return false;
+}
+
 /* The section containing the hypervisor idmap text */
 extern char __hyp_idmap_text_start[];
 extern char __hyp_idmap_text_end[];
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 1167678..9d74464 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
__cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
__cpu_init_stage2();
 
+   if (is_kernel_in_hyp_mode())
+   kvm_timer_init_vhe();
+
kvm_arm_init_debug();
 }
 
diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
index fea1073..439f6b5 100644
--- a/arch/arm64/include/asm/virt.h
+++ b/arch/arm64/include/asm/virt.h
@@ -47,6 +47,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /*
  * __boot_cpu_mode records what mode CPUs were booted in.
@@ -80,6 +81,14 @@ static inline bool is_kernel_in_hyp_mode(void)
return read_sysreg(CurrentEL) == CurrentEL_EL2;
 }
 
+static inline bool has_vhe(void)
+{
+   if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
+   return true;
+
+   return false;
+}
+
 #ifdef CONFIG_ARM64_VHE
 extern void verify_cpu_run_el(void);
 #else
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index b717ed9..5c970ce 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -76,4 +76,5 @@ void kvm_timer_unschedule(struct kvm_vcpu *vcpu);
 
 void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
 
+void kvm_timer_init_vhe(void);
 #endif
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index a7fe606..6a084cd 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -24,6 +24,7 @@
 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -509,3 +510,25 @@ void kvm_timer_init(struct kvm *kvm)
 {
kvm->arch.timer.cntvoff = kvm_phys_timer_read();
 }
+
+/*
+ * On VHE system, we only need to configure trap on physical timer and counter
+ * accesses in EL0 and EL1 once, not for every world switch.
+ * The host kernel runs at EL2 with HCR_EL2.TGE == 1,
+ * and this makes those bits have no effect for the host kernel execution.
+ */
+void kvm_timer_init_vhe(void)
+{
+   /* When HCR_EL2.E2H ==1, EL1PCEN and EL1PCTEN are shifted by 10 */
+   u32 cnthctl_shift = 10;
+   u64 val;
+
+   /*
+* Disallow physical timer access for the guest.
+* Physical counter access is allowed.
+*/
+   val = read_sysreg(cnthctl_el2);
+   val &= ~(CNTHCTL_EL1PCEN << cnthctl_shift);
+   val |= (CNTHCTL_EL1PCTEN << cnthctl_shift);
+   write_sysreg(val, cnthctl_el2);
+}
diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
index 798866a..63e28dd 100644
--- a/virt/kvm/arm/hyp/timer-sr.c
+++ b/virt/kvm/arm/hyp/timer-sr.c
@@ -35,10 +35,16 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
/* Disable the virtual timer */
write_sysreg_el0(0, cntv_ctl);
 
-   /* Allow physical timer/counter access for the host */
-   val = read_sysreg(cnthctl_el2);
-   val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
-   write_sysreg(val, cnthctl_el2);
+   /*
+* We don't need to do this for VHE since the host kernel runs in EL2
+* with HCR_EL2.TGE ==1, which makes those bits have no impact.
+*/
+   if (!has_vhe()) {
+   /* Allow physical timer/counter access for the host */
+   val = read_sysreg(cnthctl_el2);
+