On Tue, Nov 29, 2016 at 09:37:07AM +0000, Marc Zyngier wrote: > On 28/11/16 19:42, Christoffer Dall wrote: > > On Mon, Nov 28, 2016 at 06:39:04PM +0000, Marc Zyngier wrote: > >> On 28/11/16 17:43, Marc Zyngier wrote: > >>> Hi Jintack, > >>> > >>> On 28/11/16 16:46, Jintack Lim wrote: > >>>> 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 <jintack@xxxxxxxxxxxxxxx> > >>>> --- > >>>> 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 > >>>> > >> > >> [...] > >> > >>> 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? > Yes, that sounds much better. I have some patches to get rid of a lot of things, like cntvoff, during the world-switch for VHE, so if Jintack just wants to focus on the cnthctl I will catch cntvoff later. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
