From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59544) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjgpj-0004Sn-76 for qemu-devel@nongnu.org; Tue, 13 Sep 2016 02:04:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjgpf-0007ZS-Nj for qemu-devel@nongnu.org; Tue, 13 Sep 2016 02:04:38 -0400 Received: from [59.151.112.132] (port=57719 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjgpe-0007Z4-Ot for qemu-devel@nongnu.org; Tue, 13 Sep 2016 02:04:35 -0400 References: <1471944454-13895-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1471944454-13895-4-git-send-email-caoj.fnst@cn.fujitsu.com> <87oa3tw7oz.fsf@dusky.pond.sub.org> From: Cao jin Message-ID: <57D79700.3060605@cn.fujitsu.com> Date: Tue, 13 Sep 2016 14:04:48 +0800 MIME-Version: 1.0 In-Reply-To: <87oa3tw7oz.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 3/5] pci: Convert msix_init() to Error and fix callers to check it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Jiri Pirko , "Michael S. Tsirkin" , Jason Wang , Marcel Apfelbaum , Alex Williamson , Hannes Reinecke , Dmitry Fleytman , Paolo Bonzini , Gerd Hoffmann On 09/12/2016 09:47 PM, Markus Armbruster wrote: > Cao jin writes: > >> static void >> vmxnet3_cleanup_msix(VMXNET3State *s) >> { >> @@ -2315,9 +2289,19 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) >> * is a programming error. Fall back to INTx silently on -ENOTSUP */ >> assert(!ret || ret == -ENOTSUP); >> >> - if (!vmxnet3_init_msix(s)) { >> - VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); >> - } >> + ret = msix_init(pci_dev, VMXNET3_MAX_INTRS, >> + &s->msix_bar, >> + VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_TABLE, >> + &s->msix_bar, >> + VMXNET3_MSIX_BAR_IDX, VMXNET3_OFF_MSIX_PBA(s), >> + VMXNET3_MSIX_OFFSET(s), NULL); >> + /* Any error other than -ENOTSUP(board's MSI support is broken) >> + * is a programming error. Fall back to INTx silently on -ENOTSUP */ >> + assert(!ret || ret == -ENOTSUP); >> + s->msix_used = !ret; >> + /* VMXNET3_MAX_INTRS is passed, so it will never fail when mark vector. >> + * For simplicity, no need to check return value. */ >> + vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS); >> >> vmxnet3_net_init(s); > > Uh, this is more than just a conversion to Error. Before, the code > falls back to not using MSI-X on any error, with a warning. After, it > falls back on ENOTSUP only, silently, and crashes on any other error. > Such a change needs to be documented in the commit message, or be in a > separate patch. I prefer separate patch. > Dmitry has option that we should check the return value of msix_vector_use and prefer to keep init function, so I will withdraw this modification. >> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c >> index 188f954..4280c5d 100644 >> --- a/hw/usb/hcd-xhci.c >> +++ b/hw/usb/hcd-xhci.c >> @@ -3594,25 +3594,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) >> dev->config[PCI_CACHE_LINE_SIZE] = 0x10; >> dev->config[0x60] = 0x30; /* release number */ >> >> - usb_xhci_init(xhci); >> - >> - if (xhci->msi != ON_OFF_AUTO_OFF) { >> - ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err); >> - /* Any error other than -ENOTSUP(board's MSI support is broken) >> - * is a programming error */ >> - assert(!ret || ret == -ENOTSUP); >> - if (ret && xhci->msi == ON_OFF_AUTO_ON) { >> - /* Can't satisfy user's explicit msi=on request, fail */ >> - error_append_hint(&err, "You have to use msi=auto (default) or " >> - "msi=off with this machine type.\n"); >> - error_propagate(errp, err); >> - return; >> - } >> - assert(!err || xhci->msi == ON_OFF_AUTO_AUTO); >> - /* With msi=auto, we fall back to MSI off silently */ >> - error_free(err); >> - } >> - >> if (xhci->numintrs > MAXINTRS) { >> xhci->numintrs = MAXINTRS; >> } >> @@ -3622,21 +3603,60 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) >> if (xhci->numintrs < 1) { >> xhci->numintrs = 1; >> } >> + >> if (xhci->numslots > MAXSLOTS) { >> xhci->numslots = MAXSLOTS; >> } >> if (xhci->numslots < 1) { >> xhci->numslots = 1; >> } >> + >> if (xhci_get_flag(xhci, XHCI_FLAG_ENABLE_STREAMS)) { >> xhci->max_pstreams_mask = 7; /* == 256 primary streams */ >> } else { >> xhci->max_pstreams_mask = 0; >> } >> >> - xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci); >> + if (xhci->msi != ON_OFF_AUTO_OFF) { >> + ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err); >> + /* Any error other than -ENOTSUP(board's MSI support is broken) >> + * is a programming error */ >> + assert(!ret || ret == -ENOTSUP); >> + if (ret && xhci->msi == ON_OFF_AUTO_ON) { >> + /* Can't satisfy user's explicit msi=on request, fail */ >> + error_append_hint(&err, "You have to use msi=auto (default) or " >> + "msi=off with this machine type.\n"); >> + error_propagate(errp, err); >> + return; >> + } >> + assert(!err || xhci->msi == ON_OFF_AUTO_AUTO); >> + /* With msi=auto, we fall back to MSI off silently */ >> + error_free(err); >> + } > > Can you explain why you're moving this code? > Sorry I forget to mention this: msi_init() uses xhci->numintrs, but there is value checking/correcting on xhci->numintrs, it should be done before using. -- Yours Sincerely, Cao jin