linux-kernel - Re: [RFC v2 02/10] KVM: arm/arm64: Move cntvoff to each timer context

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: <CAHyh4xjixnBDS-wt5AoE2vhShKbZm+ngtJotyX9w2YMBrS-7WQ@mail.gmail.com>
Date:   Mon, 30 Jan 2017 12:40:50 -0500
From:   Jintack Lim <jintack@...columbia.edu>
To:     Marc Zyngier <marc.zyngier@....com>
Cc:     Christoffer Dall <christoffer.dall@...aro.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        linux@...linux.org.uk, Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Andre Przywara <andre.przywara@....com>,
        KVM General <kvm@...r.kernel.org>,
        arm-mail-list <linux-arm-kernel@...ts.infradead.org>,
        kvmarm@...ts.cs.columbia.edu,
        lkml - Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC v2 02/10] KVM: arm/arm64: Move cntvoff to each timer context

On Mon, Jan 30, 2017 at 9:51 AM, Marc Zyngier <marc.zyngier@....com> wrote:
> On 30/01/17 14:45, Christoffer Dall wrote:
>> On Sun, Jan 29, 2017 at 11:54:05AM +0000, Marc Zyngier wrote:
>>> On Fri, Jan 27 2017 at 01:04:52 AM, Jintack Lim <jintack@...columbia.edu> wrote:
>>>> Make cntvoff per each timer context. This is helpful to abstract kvm
>>>> timer functions to work with timer context without considering timer
>>>> types (e.g. physical timer or virtual timer).
>>>>
>>>> This also would pave the way for ever doing adjustments of the cntvoff
>>>> on a per-CPU basis if that should ever make sense.
>>>>
>>>> Signed-off-by: Jintack Lim <jintack@...columbia.edu>
>>>> ---
>>>>  arch/arm/include/asm/kvm_host.h   |  6 +++---
>>>>  arch/arm64/include/asm/kvm_host.h |  4 ++--
>>>>  include/kvm/arm_arch_timer.h      |  8 +++-----
>>>>  virt/kvm/arm/arch_timer.c         | 26 ++++++++++++++++++++------
>>>>  virt/kvm/arm/hyp/timer-sr.c       |  3 +--
>>>>  5 files changed, 29 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>>> index d5423ab..f5456a9 100644
>>>> --- a/arch/arm/include/asm/kvm_host.h
>>>> +++ b/arch/arm/include/asm/kvm_host.h
>>>> @@ -60,9 +60,6 @@ struct kvm_arch {
>>>>     /* The last vcpu id that ran on each physical CPU */
>>>>     int __percpu *last_vcpu_ran;
>>>>
>>>> -   /* Timer */
>>>> -   struct arch_timer_kvm   timer;
>>>> -
>>>>     /*
>>>>      * Anything that is not used directly from assembly code goes
>>>>      * here.
>>>> @@ -75,6 +72,9 @@ struct kvm_arch {
>>>>     /* Stage-2 page table */
>>>>     pgd_t *pgd;
>>>>
>>>> +   /* A lock to synchronize cntvoff among all vtimer context of vcpus */
>>>> +   spinlock_t cntvoff_lock;
>>>
>>> Is there any condition where we need this to be a spinlock? I would have
>>> thought that a mutex should have been enough, as this should only be
>>> updated on migration or initialization. Not that it matters much in this
>>> case, but I wondered if there is something I'm missing.
>>>
>>
>> I would think the critical section is small enough that a spinlock makes
>> sense, but what I don't think we need is to add the additional lock.
>>
>> I think just taking the kvm->lock should be sufficient, which happens to
>> be a mutex, and while that may be a bit slower to take than the
>> spinlock, it's not in the critical path so let's just keep things
>> simple.
>>
>> Perhaps this what Marc also meant.
>
> That would be the logical conclusion, assuming that we can sleep on this
> path.

All right. I'll take kvm->lock there.

Thanks,
Jintack

>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>