From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Rutherford Subject: Re: [PATCH 4/5] KVM: x86: ioapic: Clear Remote IRR when entry is switched to edge-triggered Date: Wed, 8 Nov 2017 13:25:34 -0800 Message-ID: References: <20171105135233.34572-1-nikita.leshchenko@oracle.com> <20171105135233.34572-5-nikita.leshchenko@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: Wanpeng Li , kvm , Paolo Bonzini , Radim Krcmar , idan.brown@oracle.com, Konrad Rzeszutek Wilk To: Nikita Leshchenko Return-path: Received: from mail-io0-f193.google.com ([209.85.223.193]:49461 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752694AbdKHV0P (ORCPT ); Wed, 8 Nov 2017 16:26:15 -0500 Received: by mail-io0-f193.google.com with SMTP id n137so7527975iod.6 for ; Wed, 08 Nov 2017 13:26:15 -0800 (PST) In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: Since there are two issues competing here, there's no reason to reorder these. On Wed, Nov 8, 2017 at 1:24 PM, Steve Rutherford wrote: > If an eoi on one cpu for irq x races with a guest reconfiguring the > redir entry that points to irq x, the reconfiguration might writeback > the high remote irr value that it read out in the first place. This > would leave the remote irr stuck high since there is no pending eoi > waiting to clear that. > > On Wed, Nov 8, 2017 at 1:52 AM, Nikita Leshchenko > wrote: >> >>> On 8 Nov 2017, at 2:16, Steve Rutherford wrote: >>> >>> This is cleaner. Thanks for doing this. (Note that this is racy >>> without the read only remote irr change, so I have a slight preference >>> that swap the order of these patches.) >>> Reviewed-by: Steve Rutherford >>> >> Thanks for your review. >> >> Can you explain why this is racy? >> >> I think that swapping the order of patches 4 (clearing remote irr) and >> 5 (making remote irr read only) will break Xen (and other guests that >> do explicit EOI) between the patches. Even though the current code is >> not semantically correct regarding remote irr behavior, it still makes >> explicit EOI work. If we swap the patches and apply patch 5 first, >> explicit EOI will break until we apply patch 4. >> >> Nikita >>> On Sun, Nov 5, 2017 at 7:30 PM, Wanpeng Li wrote: >>>> 2017-11-05 21:52 GMT+08:00 Nikita Leshenko : >>>>> Some OSes (Linux, Xen) use this behavior to clear the Remote IRR bit for >>>>> IOAPICs without an EOI register. They simulate the EOI message manually >>>>> by changing the trigger mode to edge and then back to level, with the >>>>> entry being masked during this. >>>>> >>>>> QEMU implements this feature in commit ed1263c363c9 >>>>> ("ioapic: clear remote irr bit for edge-triggered interrupts") >>>>> >>>>> As a side effect, this commit removes an incorrect behavior where Remote >>>>> IRR was cleared when the redirection table entry was rewritten. This is not >>>>> consistent with the manual and also opens an opportunity for a strange >>>>> behavior when a redirection table entry is modified from an interrupt >>>>> handler that handles the same entry: The modification will clear the >>>>> Remote IRR bit even though the interrupt handler is still running. >>>>> >>>>> Signed-off-by: Nikita Leshenko >>>>> Reviewed-by: Liran Alon >>>>> Signed-off-by: Konrad Rzeszutek Wilk >>>> >>>> Reviewed-by: Wanpeng Li >>>> >>>>> --- >>>>> arch/x86/kvm/ioapic.c | 11 ++++++++++- >>>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c >>>>> index 6df150eaaa78..163d340ee5f8 100644 >>>>> --- a/arch/x86/kvm/ioapic.c >>>>> +++ b/arch/x86/kvm/ioapic.c >>>>> @@ -304,8 +304,17 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val) >>>>> } else { >>>>> e->bits &= ~0xffffffffULL; >>>>> e->bits |= (u32) val; >>>>> - e->fields.remote_irr = 0; >>>>> } >>>>> + >>>>> + /* >>>>> + * Some OSes (Linux, Xen) assume that Remote IRR bit will >>>>> + * be cleared by IOAPIC hardware when the entry is configured >>>>> + * as edge-triggered. This behavior is used to simulate an >>>>> + * explicit EOI on IOAPICs that don't have the EOI register. >>>>> + */ >>>>> + if (e->fields.trig_mode == IOAPIC_EDGE_TRIG) >>>>> + e->fields.remote_irr = 0; >>>>> + >>>>> mask_after = e->fields.mask; >>>>> if (mask_before != mask_after) >>>>> kvm_fire_mask_notifiers(ioapic->kvm, KVM_IRQCHIP_IOAPIC, index, mask_after); >>>>> -- >>>>> 2.13.3 >>>>> >>