On 09/12/2016 09:47 PM, Markus Armbruster wrote:
Cao jin <[email protected]> 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
