linux-kernel - Re: [RFC 8/8] KVM: arm/arm64: Emulate the EL1 phys timer register access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170110194040.GP4348@cbox>
Date:   Tue, 10 Jan 2017 20:40:40 +0100
From:   Christoffer Dall <christoffer.dall@...aro.org>
To:     Jintack Lim <jintack@...columbia.edu>
Cc:     kvmarm@...ts.cs.columbia.edu, Marc Zyngier <marc.zyngier@....com>,
        Paolo Bonzini <pbonzini@...hat.com>, rkrcmar@...hat.com,
        linux@...linux.org.uk, Catalin Marinas <catalin.marinas@....com>,
        will.deacon@....com, andre.przywara@....com,
        linux-arm-kernel@...ts.infradead.org,
        KVM General <kvm@...r.kernel.org>, linux-kernel@...r.kernel.org
Subject: Re: [RFC 8/8] KVM: arm/arm64: Emulate the EL1 phys timer register
 access

On Tue, Jan 10, 2017 at 12:36:36PM -0500, Jintack Lim wrote:
> On Mon, Jan 9, 2017 at 7:16 AM, Christoffer Dall
> <christoffer.dall@...aro.org> wrote:
> > On Mon, Dec 26, 2016 at 12:12:06PM -0500, Jintack Lim wrote:
> >> Emulate read and write operations to CNTP_TVAL, CNTP_CVAL and CNTP_CTL.
> >> Now the VM is able to use the EL1 physical timer.
> >>
> >> Signed-off-by: Jintack Lim <jintack@...columbia.edu>
> >> ---
> >>  arch/arm64/kvm/sys_regs.c    | 35 ++++++++++++++++++++++++++++++++---
> >>  include/kvm/arm_arch_timer.h |  3 +++
> >>  virt/kvm/arm/arch_timer.c    |  4 ++--
> >>  3 files changed, 37 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >> index fd9e747..7cef94f 100644
> >> --- a/arch/arm64/kvm/sys_regs.c
> >> +++ b/arch/arm64/kvm/sys_regs.c
> >> @@ -824,7 +824,15 @@ static bool access_cntp_tval(struct kvm_vcpu *vcpu,
> >>               struct sys_reg_params *p,
> >>               const struct sys_reg_desc *r)
> >>  {
> >> -     kvm_inject_undefined(vcpu);
> >> +     struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
> >> +     cycle_t now = kvm_phys_timer_read();
> >> +
> >> +     if (p->is_write) {
> >> +             ptimer->cnt_cval = p->regval + now;
> >> +             kvm_timer_emulate(vcpu, ptimer);
> >
> > Hmm, do we really need those calls here?
> >
> > I guess I'm a little confused about exactly what the kvm_timer_emulate()
> > function is supposed to do, and it feels to me like these handlers
> > should just record what the guest is asking the kernel to do and the
> > logic of handling the additional timer should be moved into the run path
> > as much as possible.
> 
> I think it's a design decision. As you suggested, it's simple to do
> set up the background timer on entry to the VM, cancel it on exit, but
> since that's on the critical path it may have some impact on the
> performance, especially the world switch cost. To avoid
> canceling/setting up timer every world switch, I choose to schedule
> the physical timer here. I haven't compared the cost of the two
> alternatives, though.
> 

I'd definitely like to avoid us scheduling soft timers on the host if
that's not even necessary in the first place, so I'd like to get that
clear first, and as I said on the previous patch I think it's better to
get a working solution that we understand firt, and then optimize on
that later based on real results.

-Christoffer