From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 92953C04AB6 for ; Tue, 28 May 2019 16:09:01 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id 22DDC208C3 for ; Tue, 28 May 2019 16:09:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 22DDC208C3 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvmarm-bounces@lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 7586B4A4E8; Tue, 28 May 2019 12:09:00 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 7nfJwfsnY4el; Tue, 28 May 2019 12:08:59 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id F17F44A331; Tue, 28 May 2019 12:08:58 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 2E8174A331 for ; Tue, 28 May 2019 12:08:58 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id KRfGUYeRwRya for ; Tue, 28 May 2019 12:08:56 -0400 (EDT) Received: from foss.arm.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 6E7FD4A32E for ; Tue, 28 May 2019 12:08:56 -0400 (EDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B3026341; Tue, 28 May 2019 09:08:55 -0700 (PDT) Received: from [10.1.197.61] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E8DB83F59C; Tue, 28 May 2019 09:08:54 -0700 (PDT) Subject: Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection To: Andrew Jones , Christoffer Dall References: <20190527114619.16252-1-drjones@redhat.com> <20190528110152.GA6775@e113682-lin.lund.arm.com> <3f9d398b-3be2-5988-d3f9-01b28c4ccb1c@arm.com> <20190528131215.GB6775@e113682-lin.lund.arm.com> <20190528134044.olzox3c5xdhf3b4l@kamzik.brq.redhat.com> From: Marc Zyngier Openpgp: preference=signencrypt Autocrypt: addr=marc.zyngier@arm.com; prefer-encrypt=mutual; keydata= mQINBE6Jf0UBEADLCxpix34Ch3kQKA9SNlVQroj9aHAEzzl0+V8jrvT9a9GkK+FjBOIQz4KE g+3p+lqgJH4NfwPm9H5I5e3wa+Scz9wAqWLTT772Rqb6hf6kx0kKd0P2jGv79qXSmwru28vJ t9NNsmIhEYwS5eTfCbsZZDCnR31J6qxozsDHpCGLHlYym/VbC199Uq/pN5gH+5JHZyhyZiNW ozUCjMqC4eNW42nYVKZQfbj/k4W9xFfudFaFEhAf/Vb1r6F05eBP1uopuzNkAN7vqS8XcgQH qXI357YC4ToCbmqLue4HK9+2mtf7MTdHZYGZ939OfTlOGuxFW+bhtPQzsHiW7eNe0ew0+LaL 3wdNzT5abPBscqXWVGsZWCAzBmrZato+Pd2bSCDPLInZV0j+rjt7MWiSxEAEowue3IcZA++7 ifTDIscQdpeKT8hcL+9eHLgoSDH62SlubO/y8bB1hV8JjLW/jQpLnae0oz25h39ij4ijcp8N t5slf5DNRi1NLz5+iaaLg4gaM3ywVK2VEKdBTg+JTg3dfrb3DH7ctTQquyKun9IVY8AsxMc6 lxl4HxrpLX7HgF10685GG5fFla7R1RUnW5svgQhz6YVU33yJjk5lIIrrxKI/wLlhn066mtu1 DoD9TEAjwOmpa6ofV6rHeBPehUwMZEsLqlKfLsl0PpsJwov8TQARAQABtCNNYXJjIFp5bmdp ZXIgPG1hcmMuenluZ2llckBhcm0uY29tPokCOwQTAQIAJQIbAwYLCQgHAwIGFQgCCQoLBBYC AwECHgECF4AFAk6NvYYCGQEACgkQI9DQutE9ekObww/+NcUATWXOcnoPflpYG43GZ0XjQLng LQFjBZL+CJV5+1XMDfz4ATH37cR+8gMO1UwmWPv5tOMKLHhw6uLxGG4upPAm0qxjRA/SE3LC 22kBjWiSMrkQgv5FDcwdhAcj8A+gKgcXBeyXsGBXLjo5UQOGvPTQXcqNXB9A3ZZN9vS6QUYN TXFjnUnzCJd+PVI/4jORz9EUVw1q/+kZgmA8/GhfPH3xNetTGLyJCJcQ86acom2liLZZX4+1 6Hda2x3hxpoQo7pTu+XA2YC4XyUstNDYIsE4F4NVHGi88a3N8yWE+Z7cBI2HjGvpfNxZnmKX 6bws6RQ4LHDPhy0yzWFowJXGTqM/e79c1UeqOVxKGFF3VhJJu1nMlh+5hnW4glXOoy/WmDEM UMbl9KbJUfo+GgIQGMp8mwgW0vK4HrSmevlDeMcrLdfbbFbcZLNeFFBn6KqxFZaTd+LpylIH bOPN6fy1Dxf7UZscogYw5Pt0JscgpciuO3DAZo3eXz6ffj2NrWchnbj+SpPBiH4srfFmHY+Y LBemIIOmSqIsjoSRjNEZeEObkshDVG5NncJzbAQY+V3Q3yo9og/8ZiaulVWDbcpKyUpzt7pv cdnY3baDE8ate/cymFP5jGJK++QCeA6u6JzBp7HnKbngqWa6g8qDSjPXBPCLmmRWbc5j0lvA 6ilrF8m5Ag0ETol/RQEQAM/2pdLYCWmf3rtIiP8Wj5NwyjSL6/UrChXtoX9wlY8a4h3EX6E3 64snIJVMLbyr4bwdmPKULlny7T/R8dx/mCOWu/DztrVNQiXWOTKJnd/2iQblBT+W5W8ep/nS w3qUIckKwKdplQtzSKeE+PJ+GMS+DoNDDkcrVjUnsoCEr0aK3cO6g5hLGu8IBbC1CJYSpple VVb/sADnWF3SfUvJ/l4K8Uk4B4+X90KpA7U9MhvDTCy5mJGaTsFqDLpnqp/yqaT2P7kyMG2E w+eqtVIqwwweZA0S+tuqput5xdNAcsj2PugVx9tlw/LJo39nh8NrMxAhv5aQ+JJ2I8UTiHLX QvoC0Yc/jZX/JRB5r4x4IhK34Mv5TiH/gFfZbwxd287Y1jOaD9lhnke1SX5MXF7eCT3cgyB+ hgSu42w+2xYl3+rzIhQqxXhaP232t/b3ilJO00ZZ19d4KICGcakeiL6ZBtD8TrtkRiewI3v0 o8rUBWtjcDRgg3tWx/PcJvZnw1twbmRdaNvsvnlapD2Y9Js3woRLIjSAGOijwzFXSJyC2HU1 AAuR9uo4/QkeIrQVHIxP7TJZdJ9sGEWdeGPzzPlKLHwIX2HzfbdtPejPSXm5LJ026qdtJHgz BAb3NygZG6BH6EC1NPDQ6O53EXorXS1tsSAgp5ZDSFEBklpRVT3E0NrDABEBAAGJAh8EGAEC AAkFAk6Jf0UCGwwACgkQI9DQutE9ekMLBQ//U+Mt9DtFpzMCIHFPE9nNlsCm75j22lNiw6mX mx3cUA3pl+uRGQr/zQC5inQNtjFUmwGkHqrAw+SmG5gsgnM4pSdYvraWaCWOZCQCx1lpaCOl MotrNcwMJTJLQGc4BjJyOeSH59HQDitKfKMu/yjRhzT8CXhys6R0kYMrEN0tbe1cFOJkxSbV 0GgRTDF4PKyLT+RncoKxQe8lGxuk5614aRpBQa0LPafkirwqkUtxsPnarkPUEfkBlnIhAR8L kmneYLu0AvbWjfJCUH7qfpyS/FRrQCoBq9QIEcf2v1f0AIpA27f9KCEv5MZSHXGCdNcbjKw1 39YxYZhmXaHFKDSZIC29YhQJeXWlfDEDq6nIhvurZy3mSh2OMQgaIoFexPCsBBOclH8QUtMk a3jW/qYyrV+qUq9Wf3SKPrXf7B3xB332jFCETbyZQXqmowV+2b3rJFRWn5hK5B+xwvuxKyGq qDOGjof2dKl2zBIxbFgOclV7wqCVkhxSJi/QaOj2zBqSNPXga5DWtX3ekRnJLa1+ijXxmdjz hApihi08gwvP5G9fNGKQyRETePEtEAWt0b7dOqMzYBYGRVr7uS4uT6WP7fzOwAJC4lU7ZYWZ yVshCa0IvTtp1085RtT3qhh9mobkcZ+7cQOY+Tx2RGXS9WeOh2jZjdoWUv6CevXNQyOUXMM= Organization: ARM Ltd Message-ID: Date: Tue, 28 May 2019 17:08:53 +0100 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: <20190528134044.olzox3c5xdhf3b4l@kamzik.brq.redhat.com> Content-Language: en-US Cc: kvmarm@lists.cs.columbia.edu X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu On 28/05/2019 14:40, Andrew Jones wrote: > On Tue, May 28, 2019 at 03:12:15PM +0200, Christoffer Dall wrote: >> On Tue, May 28, 2019 at 01:25:52PM +0100, Marc Zyngier wrote: >>> On 28/05/2019 12:01, Christoffer Dall wrote: >>>> On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote: >>>>> The emulated ptimer needs to track the level changes, otherwise the >>>>> the interrupt will never get deasserted, resulting in the guest getting >>>>> stuck in an interrupt storm if it enables ptimer interrupts. This was >>>>> found with kvm-unit-tests; the ptimer tests hung as soon as interrupts >>>>> were enabled. Typical Linux guests don't have a problem as they prefer >>>>> using the virtual timer. >>>>> >>>>> Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to use a timer_map") >>>>> Signed-off-by: Andrew Jones >>>>> --- >>>>> virt/kvm/arm/arch_timer.c | 7 ++++++- >>>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c >>>>> index 7fc272ecae16..9f5d8cc8b5e5 100644 >>>>> --- a/virt/kvm/arm/arch_timer.c >>>>> +++ b/virt/kvm/arm/arch_timer.c >>>>> @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level, >>>>> static void timer_emulate(struct arch_timer_context *ctx) >>>>> { >>>>> bool should_fire = kvm_timer_should_fire(ctx); >>>>> + struct timer_map map; >>>>> + >>>>> + get_timer_map(ctx->vcpu, &map); >>>>> >>>>> trace_kvm_timer_emulate(ctx, should_fire); >>>>> >>>>> - if (should_fire) { >>>>> + if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) { >>>>> + kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx); >>>>> + } else if (should_fire) { >>>>> kvm_timer_update_irq(ctx->vcpu, true, ctx); >>>>> return; >>>>> } >>>> >>>> Hmm, this doesn't feel completely right. > > I won't try to argue that this is the right fix, as I haven't fully > grasped how all this code works, but, afaict, this is how it worked > prior to bee038a6. > >>>> >>>> Lowering the line of an emulated timer should only ever happen when the >>>> guest (or user space) writes to one of the system registers for that >>>> timer, which should be trapped and that should cause an update of the >>>> line. >>>> >>>> Are we missing a call to kvm_timer_update_irq() from >>>> kvm_arm_timer_set_reg() ? >>> >>> Which is exactly what we removed in 6bc210003dff, for good reasons. >>> >> >> Ah well, I can be wrong twice. Or even three times. >> >>> Looking at kvm_arm_timer_write_sysreg(), we end-up calling kvm_timer_vcpu_load, but not updating the irq status. >>> >>> How about something like this instead (untested): >>> >>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c >>> index 7fc272ecae16..6a418dcc5433 100644 >>> --- a/virt/kvm/arm/arch_timer.c >>> +++ b/virt/kvm/arm/arch_timer.c >>> @@ -882,10 +882,14 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu *vcpu, >>> enum kvm_arch_timer_regs treg, >>> u64 val) >>> { >>> + struct arch_timer_context *timer; >>> + >>> preempt_disable(); >>> kvm_timer_vcpu_put(vcpu); >>> >>> - kvm_arm_timer_write(vcpu, vcpu_get_timer(vcpu, tmr), treg, val); >>> + timer = vcpu_get_timer(vcpu, tmr); >>> + kvm_arm_timer_write(vcpu, timer, treg, val); >>> + kvm_timer_update_irq(vcpu, kvm_timer_should_fire(timer), timer); >>> >>> kvm_timer_vcpu_load(vcpu); >>> preempt_enable(); >>> > > Marc, I've tested this and it resolves the issue for me. If/when you post > it you can add a t-b from me if you like. > >> >> Yes, that looks reasonable. Basically, in 6bc210003dff we should have >> only removed the call to timer_emulate, and not messed around with >> kvm_timer_update_irq()? >> >> After this patch, we'll have moved the call to kvm_timer_update_irq() >> from kvm_arm_timer_set_reg() to kvm_arm_timer_write_sysreg(). I can't >> seem to decide if clearly belongs in one place or the other. >> > > Isn't kvm_arm_timer_set_reg() only for userspace setting of the register? > In this test case I don't think userspace is involved at that point. It still remains that userspace writing to any of the registers has an effect on the interrupt line. Or rather that it should. And the more I look at this, the more I have the feeling this thing should happen on kvm_timer_vcpu_load(), wherever the writes comes from. It'd have slightly more overhead than doing it from every register access path, but at least it'd be clearer... Untested, again. diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index 7fc272ecae16..8244e40af196 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -557,8 +557,12 @@ void kvm_timer_vcpu_load(struct kvm_vcpu *vcpu) if (map.direct_ptimer) timer_restore_state(map.direct_ptimer); - if (map.emul_ptimer) + if (map.emul_ptimer) { + kvm_timer_update_irq(vcpu, + kvm_timer_should_fire(map.emul_ptimer), + map.emul_ptimer); timer_emulate(map.emul_ptimer); + } } bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu) Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm