From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavel Fedin Subject: RE: [PATCH] KVM: arm/arm64: BUG: Fix losing level-sensitive interrupts Date: Wed, 26 Aug 2015 14:33:06 +0300 Message-ID: <00d901d0dff2$fa5616d0$ef024470$@samsung.com> References: <1440571563-7004-1-git-send-email-p.fedin@samsung.com> <20150826092721.43e8cbfd@arm.com> <00d701d0dfee$2f464810$8dd2d830$@samsung.com> <55DD9F5B.4080602@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, 'Christoffer Dall' , =?ISO-8859-1?Q?'Alex_Benn=E9e'?= To: 'Marc Zyngier' Return-path: Received: from mailout2.w1.samsung.com ([210.118.77.12]:20229 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756539AbbHZLdJ (ORCPT ); Wed, 26 Aug 2015 07:33:09 -0400 Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout2.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0NTO00FXZTF7BY70@mailout2.w1.samsung.com> for kvm@vger.kernel.org; Wed, 26 Aug 2015 12:33:07 +0100 (BST) In-reply-to: <55DD9F5B.4080602@arm.com> Content-language: ru Sender: kvm-owner@vger.kernel.org List-ID: Hello! > As for v4.1 not having that problem, the pl011 driver has gone though a > lot if rework lately, and I wouldn't be surprised if it now exhibited a > different behaviour thanks to the broken userspace behaviour. Sorry, you misunderstood me. Or i wrote badly. I meant that _KVM_ did not have this particular problem in kernel v4.0, because: http://lxr.free-electrons.com/source/virt/kvm/arm/vgic.c?v=4.0#L998 you see, LR_STATE_PENDING is assigned unconditionally. Is this code correct? I believe yes. Compare with: http://lxr.free-electrons.com/source/virt/kvm/arm/vgic.c#L1104 Now it is possible to have neither PENDING nor ACTIVE irq. Does it even make sense? So what is wrong with the modification as follows? --- cut --- if (vgic_irq_is_active(vcpu, irq)) { vlr.state |= LR_STATE_ACTIVE; kvm_debug("Set active, clear distributor: 0x%x\n", vlr.state); vgic_irq_clear_active(vcpu, irq); vgic_update_state(vcpu->kvm); } else { vlr.state |= LR_STATE_PENDING; kvm_debug("Set pending: 0x%x\n", vlr.state); } --- cut --- Alex, are you reading us? Can you explain, why you introduced that extra check? > And what you're suggesting is to actually introduce a bug. Why would that be a bug, if it was not a bug in kernel 4.0? Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia