From mboxrd@z Thu Jan 1 00:00:00 1970 From: Leo Yan Subject: Re: [PATCH kvmtool v3 3/3] vfio-pci: Re-enable INTx mode when disable MSI/MSIX Date: Fri, 5 Apr 2019 11:46:09 +0800 Message-ID: <20190405034609.GA6963@leoy-ThinkPad-X240s> References: <20190326074131.4284-1-leo.yan@linaro.org> <20190326074131.4284-4-leo.yan@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: "kvm@vger.kernel.org" , Marc Zyngier , Will Deacon , Robin Murphy , "kvmarm@lists.cs.columbia.edu" To: Jean-Philippe Brucker Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu List-Id: kvm.vger.kernel.org Hi Jean-Philippe, On Thu, Apr 04, 2019 at 12:06:51PM +0100, Jean-Philippe Brucker wrote: > On 26/03/2019 07:41, Leo Yan wrote: > > Since PCI forbids enabling INTx, MSI or MSIX at the same time, it's by > > default to disable INTx mode when enable MSI/MSIX mode; but this logic is > > easily broken if the guest PCI driver detects the MSI/MSIX cannot work as > > expected and tries to rollback to use INTx mode. In this case, the INTx > > mode has been disabled and has no chance to re-enable it, thus both INTx > > mode and MSI/MSIX mode cannot work in vfio. > > > > Below shows the detailed flow for introducing this issue: > > > > vfio_pci_configure_dev_irqs() > > `-> vfio_pci_enable_intx() > > > > vfio_pci_enable_msis() > > `-> vfio_pci_disable_intx() > > > > vfio_pci_disable_msis() => Guest PCI driver disables MSI > > > > To fix this issue, when disable MSI/MSIX we need to check if INTx mode > > is available for this device or not; if the device can support INTx then > > re-enable it so that the device can fallback to use it. > > > > Since vfio_pci_disable_intx() / vfio_pci_enable_intx() pair functions > > may be called for multiple times, this patch uses 'intx_fd == -1' to > > denote the INTx is disabled, the pair functions can directly bail out > > when detect INTx has been disabled and enabled respectively. > > > > Suggested-by: Jean-Philippe Brucker > > Signed-off-by: Leo Yan > > --- > > vfio/pci.c | 41 ++++++++++++++++++++++++++++++----------- > > 1 file changed, 30 insertions(+), 11 deletions(-) > > > > diff --git a/vfio/pci.c b/vfio/pci.c > > index 3c39844..3b2b1e7 100644 > > --- a/vfio/pci.c > > +++ b/vfio/pci.c > > @@ -28,6 +28,7 @@ struct vfio_irq_eventfd { > > msi_update_state(state, val, VFIO_PCI_MSI_STATE_EMPTY) > > > > static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev); > > +static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev); > > > > static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev, > > bool msix) > > @@ -50,17 +51,14 @@ static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev, > > if (!msi_is_enabled(msis->virt_state)) > > return 0; > > > > - if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX) { > > - /* > > - * PCI (and VFIO) forbids enabling INTx, MSI or MSIX at the same > > - * time. Since INTx has to be enabled from the start (we don't > > - * have a reliable way to know when the user starts using it), > > - * disable it now. > > - */ > > + /* > > + * PCI (and VFIO) forbids enabling INTx, MSI or MSIX at the same > > + * time. Since INTx has to be enabled from the start (after enabling > > + * 'pdev->intx_fd' will be assigned to an eventfd and doesn't equal > > + * to the init value -1), disable it now. > > + */ > > I don't think the comment change is useful, we don't need that much > detail. The text that you replaced was trying to explain why we enable > INTx from the start, and would still apply (although it should have been > s/user/guest/) > > > + if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX) > > vfio_pci_disable_intx(kvm, vdev); > > - /* Permanently disable INTx */ > > - pdev->irq_modes &= ~VFIO_PCI_IRQ_MODE_INTX; > > - } > > > > eventfds = (void *)msis->irq_set + sizeof(struct vfio_irq_set); > > > > @@ -162,7 +160,16 @@ static int vfio_pci_disable_msis(struct kvm *kvm, struct vfio_device *vdev, > > msi_set_enabled(msis->phys_state, false); > > msi_set_empty(msis->phys_state, true); > > > > - return 0; > > + /* > > + * When MSI or MSIX is disabled, this might be called when > > + * PCI driver detects the MSI interrupt failure and wants to > > + * rollback to INTx mode. Thus enable INTx if the device > > + * supports INTx mode in this case. > > + */ > > + if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX) > > + ret = vfio_pci_enable_intx(kvm, vdev); > > + > > + return ret >= 0 ? 0 : ret; > > } > > > > static int vfio_pci_update_msi_entry(struct kvm *kvm, struct vfio_device *vdev, > > @@ -1002,6 +1009,10 @@ static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev) > > .index = VFIO_PCI_INTX_IRQ_INDEX, > > }; > > > > + /* INTx mode has been disabled */ > > Here as well, the comments on intx_fd seem unnecessary. But these are > only nits, the code is fine and I tested for regressions on my hardware, so: > > Reviewed-by: Jean-Philippe Brucker Thanks for reviewing. I will spin a new patch set to drop unnecessary comment to let changes as simple as possible. Thanks, Leo Yan 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=-8.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT 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 171DEC4360F for ; Fri, 5 Apr 2019 03:46:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D004120854 for ; Fri, 5 Apr 2019 03:46:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="mFNdA4XZ" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729803AbfDEDqQ (ORCPT ); Thu, 4 Apr 2019 23:46:16 -0400 Received: from mail-pl1-f196.google.com ([209.85.214.196]:33794 "EHLO mail-pl1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728735AbfDEDqQ (ORCPT ); Thu, 4 Apr 2019 23:46:16 -0400 Received: by mail-pl1-f196.google.com with SMTP id y6so2275908plt.1 for ; Thu, 04 Apr 2019 20:46:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=sLfe6SZAf6VhqRnk/N6YFcC4Tn6aQmnNWFtqLTWWIds=; b=mFNdA4XZoKejW5KJpNJWuouJKKfISENV8YayCAJFhP7bwR2UHh6omrKvnstUfO5DCq Q26Y30fyuPzXG2sav/05HgnGXvvxxif5aCUC99ZAJdfeXtElNX9hR8PJkXC7P24sjWRI zSk65flJj4oLsT0wWuALwrwB1QXJUjAfA+7WvQlYCUejQgRcgBSULMMSgOo7JjOJWyF5 ScLhFvs+q3xXJirXcx290ywXAQG5ceOH4RVLlVUTOSv7DHNhn3bhhsQX8iCz7/dwhPF8 qy3JJn8UR+AV0XJ53mO5XIV9iMi/CDQnOJBYJx/72UbHp/DPfabJN7LHILKzBpin8r56 Hbyg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=sLfe6SZAf6VhqRnk/N6YFcC4Tn6aQmnNWFtqLTWWIds=; b=qI8HRmsApO/DU5UFgZ2+oUUzzbWEC2aikcqf2Hen5JHv78UZHyKqu8KnXoq95XRXlx xBhx3O33Msn4ho0RAXjCB2RBuY5VVUi1g+bReo0+cPqWFEY/uSNua8Kf7cl5j1B0LYqI BSTeS3AMh/7FQatFILQa0hHCwRYwxU4YyvuVZwD9tzvV8sDxufDoVCCxU0qUSuSjmGQY 7IFH/v88e72FQV5UQ4o6HFP+toVBMZQSOvAtm+hXlS6DmSIVhdBgiiCXKMG3rcVXhBCU t0NSJ6kEV6s7yuXpZzBk2teU+mcGroqbPcffs0YpXGnQCBI0q+ae7Qkaov3o5IpJdwzo FX3g== X-Gm-Message-State: APjAAAUP2A4KtYRRnyoa5xVZuoAw0i3uBp+ReeDM/n4S/9cLoOxijDl1 n6U73OwC6r3H4xJlIcSkXu6Lxw== X-Google-Smtp-Source: APXvYqz52vDcs8UBZEStpBVlRs320LMaC20TS35Z7FNZrX3LannxeS0tFm5CWvJaC6dpbqs4THejow== X-Received: by 2002:a17:902:a81:: with SMTP id 1mr10165281plp.308.1554435975219; Thu, 04 Apr 2019 20:46:15 -0700 (PDT) Received: from leoy-ThinkPad-X240s ([147.50.13.10]) by smtp.gmail.com with ESMTPSA id o67sm29390308pga.55.2019.04.04.20.46.11 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 04 Apr 2019 20:46:14 -0700 (PDT) Date: Fri, 5 Apr 2019 11:46:09 +0800 From: Leo Yan To: Jean-Philippe Brucker Cc: "kvm@vger.kernel.org" , "kvmarm@lists.cs.columbia.edu" , Will Deacon , Marc Zyngier , Eric Auger , Robin Murphy Subject: Re: [PATCH kvmtool v3 3/3] vfio-pci: Re-enable INTx mode when disable MSI/MSIX Message-ID: <20190405034609.GA6963@leoy-ThinkPad-X240s> References: <20190326074131.4284-1-leo.yan@linaro.org> <20190326074131.4284-4-leo.yan@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org Message-ID: <20190405034609.8K2CAfNY3mzVqDx0xfm7XH85t1QCGPeYcYcOqEKf4Rg@z> Hi Jean-Philippe, On Thu, Apr 04, 2019 at 12:06:51PM +0100, Jean-Philippe Brucker wrote: > On 26/03/2019 07:41, Leo Yan wrote: > > Since PCI forbids enabling INTx, MSI or MSIX at the same time, it's by > > default to disable INTx mode when enable MSI/MSIX mode; but this logic is > > easily broken if the guest PCI driver detects the MSI/MSIX cannot work as > > expected and tries to rollback to use INTx mode. In this case, the INTx > > mode has been disabled and has no chance to re-enable it, thus both INTx > > mode and MSI/MSIX mode cannot work in vfio. > > > > Below shows the detailed flow for introducing this issue: > > > > vfio_pci_configure_dev_irqs() > > `-> vfio_pci_enable_intx() > > > > vfio_pci_enable_msis() > > `-> vfio_pci_disable_intx() > > > > vfio_pci_disable_msis() => Guest PCI driver disables MSI > > > > To fix this issue, when disable MSI/MSIX we need to check if INTx mode > > is available for this device or not; if the device can support INTx then > > re-enable it so that the device can fallback to use it. > > > > Since vfio_pci_disable_intx() / vfio_pci_enable_intx() pair functions > > may be called for multiple times, this patch uses 'intx_fd == -1' to > > denote the INTx is disabled, the pair functions can directly bail out > > when detect INTx has been disabled and enabled respectively. > > > > Suggested-by: Jean-Philippe Brucker > > Signed-off-by: Leo Yan > > --- > > vfio/pci.c | 41 ++++++++++++++++++++++++++++++----------- > > 1 file changed, 30 insertions(+), 11 deletions(-) > > > > diff --git a/vfio/pci.c b/vfio/pci.c > > index 3c39844..3b2b1e7 100644 > > --- a/vfio/pci.c > > +++ b/vfio/pci.c > > @@ -28,6 +28,7 @@ struct vfio_irq_eventfd { > > msi_update_state(state, val, VFIO_PCI_MSI_STATE_EMPTY) > > > > static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev); > > +static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev); > > > > static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev, > > bool msix) > > @@ -50,17 +51,14 @@ static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev, > > if (!msi_is_enabled(msis->virt_state)) > > return 0; > > > > - if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX) { > > - /* > > - * PCI (and VFIO) forbids enabling INTx, MSI or MSIX at the same > > - * time. Since INTx has to be enabled from the start (we don't > > - * have a reliable way to know when the user starts using it), > > - * disable it now. > > - */ > > + /* > > + * PCI (and VFIO) forbids enabling INTx, MSI or MSIX at the same > > + * time. Since INTx has to be enabled from the start (after enabling > > + * 'pdev->intx_fd' will be assigned to an eventfd and doesn't equal > > + * to the init value -1), disable it now. > > + */ > > I don't think the comment change is useful, we don't need that much > detail. The text that you replaced was trying to explain why we enable > INTx from the start, and would still apply (although it should have been > s/user/guest/) > > > + if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX) > > vfio_pci_disable_intx(kvm, vdev); > > - /* Permanently disable INTx */ > > - pdev->irq_modes &= ~VFIO_PCI_IRQ_MODE_INTX; > > - } > > > > eventfds = (void *)msis->irq_set + sizeof(struct vfio_irq_set); > > > > @@ -162,7 +160,16 @@ static int vfio_pci_disable_msis(struct kvm *kvm, struct vfio_device *vdev, > > msi_set_enabled(msis->phys_state, false); > > msi_set_empty(msis->phys_state, true); > > > > - return 0; > > + /* > > + * When MSI or MSIX is disabled, this might be called when > > + * PCI driver detects the MSI interrupt failure and wants to > > + * rollback to INTx mode. Thus enable INTx if the device > > + * supports INTx mode in this case. > > + */ > > + if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX) > > + ret = vfio_pci_enable_intx(kvm, vdev); > > + > > + return ret >= 0 ? 0 : ret; > > } > > > > static int vfio_pci_update_msi_entry(struct kvm *kvm, struct vfio_device *vdev, > > @@ -1002,6 +1009,10 @@ static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev) > > .index = VFIO_PCI_INTX_IRQ_INDEX, > > }; > > > > + /* INTx mode has been disabled */ > > Here as well, the comments on intx_fd seem unnecessary. But these are > only nits, the code is fine and I tested for regressions on my hardware, so: > > Reviewed-by: Jean-Philippe Brucker Thanks for reviewing. I will spin a new patch set to drop unnecessary comment to let changes as simple as possible. Thanks, Leo Yan