From: Christoffer Dall <christoffer.dall@linaro.org>
To: Marc Zyngier <marc.zyngier@arm.com>
Cc: Jintack Lim <jintack@cs.columbia.edu>,
kvmarm@lists.cs.columbia.edu,
linux-arm-kernel@lists.infradead.org, will.deacon@arm.com,
catalin.marinas@arm.com, pbonzini@redhat.com, rkrcmar@redhat.com,
linux@armlinux.org.uk, julien.grall@arm.com,
andre.przywara@arm.com, kvm@vger.kernel.org
Subject: Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly
Date: Mon, 28 Nov 2016 20:42:21 +0100 [thread overview]
Message-ID: <20161128194221.GG18170@cbox> (raw)
In-Reply-To: <799d03f5-a929-9547-1ae7-94026b76f116@arm.com>
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@cs.columbia.edu>
> >> ---
> >> 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?
Thanks,
-Christoffer
WARNING: multiple messages have this Message-ID (diff)
From: christoffer.dall@linaro.org (Christoffer Dall)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly
Date: Mon, 28 Nov 2016 20:42:21 +0100 [thread overview]
Message-ID: <20161128194221.GG18170@cbox> (raw)
In-Reply-To: <799d03f5-a929-9547-1ae7-94026b76f116@arm.com>
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@cs.columbia.edu>
> >> ---
> >> 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?
Thanks,
-Christoffer
next prev parent reply other threads:[~2016-11-28 19:42 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-28 16:46 [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly Jintack Lim
2016-11-28 16:46 ` Jintack Lim
2016-11-28 17:43 ` Marc Zyngier
2016-11-28 17:43 ` Marc Zyngier
2016-11-28 18:39 ` Marc Zyngier
2016-11-28 18:39 ` Marc Zyngier
2016-11-28 19:42 ` Christoffer Dall [this message]
2016-11-28 19:42 ` Christoffer Dall
2016-11-29 9:37 ` Marc Zyngier
2016-11-29 9:37 ` Marc Zyngier
2016-11-29 10:47 ` Christoffer Dall
2016-11-29 10:47 ` Christoffer Dall
2016-11-29 10:53 ` Marc Zyngier
2016-11-29 10:53 ` Marc Zyngier
2016-11-29 3:28 ` Jintack Lim
2016-11-29 3:28 ` Jintack Lim
2016-11-29 9:36 ` Marc Zyngier
2016-11-29 9:36 ` Marc Zyngier
2016-11-29 11:29 ` Jintack Lim
2016-11-29 11:29 ` Jintack Lim
2016-11-29 16:53 ` Suzuki K Poulose
2016-11-29 16:53 ` Suzuki K Poulose
2016-11-29 21:05 ` Jintack Lim
2016-11-29 21:05 ` Jintack Lim
2016-11-30 13:31 ` Marc Zyngier
2016-11-30 13:31 ` Marc Zyngier
2016-11-30 13:41 ` Jintack Lim
2016-11-30 13:41 ` Jintack Lim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161128194221.GG18170@cbox \
--to=christoffer.dall@linaro.org \
--cc=andre.przywara@arm.com \
--cc=catalin.marinas@arm.com \
--cc=jintack@cs.columbia.edu \
--cc=julien.grall@arm.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux@armlinux.org.uk \
--cc=marc.zyngier@arm.com \
--cc=pbonzini@redhat.com \
--cc=rkrcmar@redhat.com \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.