On 28/11/16 19:42, Christoffer Dall wrote:

On Mon, Nov 28, 2016 at 06:39:04PM +0000, Marc Zyngier wrote:

quoted

On 28/11/16 17:43, Marc Zyngier wrote:

quoted

Hi Jintack,

On 28/11/16 16:46, Jintack Lim wrote:

quoted

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.  Current code is
unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
may allow guest OS to access physical timer. So, fix it.

Signed-off-by: Jintack Lim <redacted>
---
 arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
 arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
 include/clocksource/arm_arch_timer.h |  6 ++--
 virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
 4 files changed, 103 insertions(+), 6 deletions(-)
 create mode 100644 arch/arm/include/asm/kvm_timer.h
 create mode 100644 arch/arm64/include/asm/kvm_timer.h

[...]

quoted

We could make it nicer (read "faster") by introducing a
hyp_alternate_select construct that only returns a value instead
of calling a function. I remember writing something like that
at some point, and dropping it...

So here's what this could look like (warning, wacky code ahead,
though I fixed a stupid bug that was present in the previous patch).
The generated code is quite nice (no branch, only an extra mov
instruction on the default path)... Of course, completely untested!

Isn't this all about determining which bitmask to use, statically, once,
after the system has booted?

How about a good old fashioned static variable, or global struct like
the global one we use for the VGIC, which sets the proper mit mask
during kvm init, and the world-switch code just uses a variable?

We could indeed do that (I've been carried away with my tendency for
weird and wonderful hacks).

But as Jintack mentioned, there is a much better approach, which is to
do nothing at all on the VHE path (we can set the permission bits once
and for all). cntvoff_el2 also falls into the same category of things we
should be able to only restore and not bother resetting (as it doesn't
affect the EL2 virtual counter).

Thoughts?

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