From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1756880AbcLUMop (ORCPT <rfc822;w@1wt.eu>);
        Wed, 21 Dec 2016 07:44:45 -0500
Received: from mx1.redhat.com ([209.132.183.28]:52206 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1754359AbcLUMom (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 21 Dec 2016 07:44:42 -0500
Date: Wed, 21 Dec 2016 13:44:38 +0100
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: Wanpeng Li <kernellwp@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        kvm <kvm@vger.kernel.org>, stable@vger.kernel.org,
        Dmitry Vyukov <dvyukov@google.com>,
        Steve Rutherford <srutherford@google.com>
Subject: Re: [PATCH] KVM: x86: check for pic and ioapic presence before use
Message-ID: <20161221124437.GA13185@potion>
References: <20161123202548.29324-1-rkrcmar@redhat.com>
 <06c15e9d-43fc-0d08-57e4-26440c441eb0@redhat.com>
 <20161124124206.GA16974@potion>
 <CANRm+Cx6L6O52uJQJ1WQHTYO0WfSZLTLwemFbhSPnOvv+Ds=KA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CANRm+Cx6L6O52uJQJ1WQHTYO0WfSZLTLwemFbhSPnOvv+Ds=KA@mail.gmail.com>
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Wed, 21 Dec 2016 12:44:41 +0000 (UTC)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

2016-12-20 19:59+0800, Wanpeng Li:
> 2016-11-24 20:42 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
>> 2016-11-23 22:58+0100, Paolo Bonzini:
>>> On 23/11/2016 21:25, Radim Krčmář wrote:
>>>> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
>>>> index 25810b144b58..ddd63b8b176e 100644
>>>> --- a/arch/x86/kvm/irq_comm.c
>>>> +++ b/arch/x86/kvm/irq_comm.c
>>>> @@ -41,6 +41,15 @@ static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
>>>>                         bool line_status)
>>>>  {
>>>>      struct kvm_pic *pic = pic_irqchip(kvm);
>>>> +
>>>> +    /*
>>>> +     * XXX: rejecting pic routes when pic isn't in use would be better,
>>>> +     * but the default routing table is installed while kvm->arch.vpic is
>>>> +     * NULL and KVM_CREATE_IRQCHIP can race with KVM_SET_GSI_ROUTING.
>>>> +     */
>>>> +    if (!pic)
>>>> +            return -1;
>>>> +
>>>>      return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
>>>>  }
>>>>
>>>
>>> Can you explain the race with the default routing table better?  It
>>> seems to me that it can only make the routing table go from invalid to
>>> valid (there is no KVM_DESTROY_IRQCHIP) so it's benign.
>>
>> Oops, I wrote the race with wrong IOCTL -- it should be KVM_IRQ_LINE.
>>
>>  1) set KVM_CAP_SPLIT_IRQCHIP (unlocks KVM_IRQ_LINE)
>>  a) call KVM_CREATE_IRQCHIP (creates routes while !kvm->arch.vpic)
>>  b) concurrently call KVM_IRQ_LINE for PIO routes (dereferences NULL)
> 
> If we should not go through irqfd if irqchip is split?

I also remember hearing about that -- do you remember where it was?

The documentation does not say that and irqfd is mostly optimization for
KVM_IRQ_LINE ... QEMU uses KVM_IRQ_LINE_STATUS with split irqchip, so we
can't easily say the opposite now.