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: <CAHyh4xhxLr9ngh2YeERUSjzyv2S0bk6MqcNjP555vRGLEZDieA@mail.gmail.com>
Date:   Tue, 10 Jan 2017 15:10:36 -0500
From:   Jintack Lim <jintack@...columbia.edu>
To:     Christoffer Dall <christoffer.dall@...aro.org>
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 2:40 PM, Christoffer Dall
<christoffer.dall@...aro.org> wrote:
> 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.

Ok, it makes sense. I'll respin!

>
> -Christoffer
>