| Message ID | 1473839464-8670-3-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Cao jin <caoj.fnst@cn.fujitsu.com> writes: > Param checking/correcting code of xchi->numintrs should be placed before > it is used. > Also move some resource-alloc code down, save the strenth to free them > on msi_init's failure. > > CC: Gerd Hoffmann <kraxel@redhat.com> > CC: Markus Armbruster <armbru@redhat.com> > CC: Marcel Apfelbaum <marcel@redhat.com> > CC: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > hw/usb/hcd-xhci.c | 39 +++++++++++++++++++-------------------- > 1 file changed, 19 insertions(+), 20 deletions(-) > > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > index 188f954..95b1954 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); > - } > - Outside this patch's scope, but here goes anyway: > if (xhci->numintrs > MAXINTRS) { > xhci->numintrs = MAXINTRS; > } while (xhci->numintrs & (xhci->numintrs - 1)) { /* ! power of 2 */ xhci->numintrs++; } 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 { > @@ -3634,7 +3615,22 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) > xhci->max_pstreams_mask = 0; > } If the user specifies invalid intrs, slots or streams properties, his configuration is silently corrected to a valid one. I hate that. Invalid configuration should be rejected cleanly. By the way, calling the property "intrs" differs needlessly from virtio, which calls it "vectors". Anyway, nothing wrong with the patch here. Before this patch, we can have msi_init() choke on invalid xhci->numintrs before we reach the configuration correction code. Your patch fixes it. > > - 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); > + } > > memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS); > memory_region_init_io(&xhci->mem_cap, OBJECT(xhci), &xhci_cap_ops, xhci, > @@ -3664,6 +3660,9 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) > PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_TYPE_64, > &xhci->mem); > > + usb_xhci_init(xhci); > + xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci); > + Before your patch, resources allocated by usb_xhci_init() were leaked on msi_init() failure. Your patch also delays it and timer_new_ns() until after we can't fail anymore. That's a good idea unless their work is actually used earlier. It isn't as far as I can see. > if (pci_bus_is_express(dev->bus) || > xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) { > ret = pcie_endpoint_cap_init(dev, 0xa0); I can take this through my tree if Gerd provides at least his Acked-by, preferably Reviewed-by. Gerd, if you'd rather take it through yours, let me know. I'll take this series into my tree then, wait for the patch to make its way through yours, and rebase.
Hi, > I can take this through my tree if Gerd provides at least his Acked-by, > preferably Reviewed-by. Gerd, if you'd rather take it through yours, > let me know. I'll take this series into my tree then, wait for the > patch to make its way through yours, and rebase. I'm fine with you merging the patch. Splitting the series only makes things more complicated for no good reason. Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> cheers, Gerd
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 188f954..95b1954 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; } @@ -3634,7 +3615,22 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) 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); + } memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS); memory_region_init_io(&xhci->mem_cap, OBJECT(xhci), &xhci_cap_ops, xhci, @@ -3664,6 +3660,9 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_TYPE_64, &xhci->mem); + usb_xhci_init(xhci); + xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci); + if (pci_bus_is_express(dev->bus) || xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) { ret = pcie_endpoint_cap_init(dev, 0xa0);
Param checking/correcting code of xchi->numintrs should be placed before it is used. Also move some resource-alloc code down, save the strenth to free them on msi_init's failure. CC: Gerd Hoffmann <kraxel@redhat.com> CC: Markus Armbruster <armbru@redhat.com> CC: Marcel Apfelbaum <marcel@redhat.com> CC: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- hw/usb/hcd-xhci.c | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-)