From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40253) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpbgz-0008I0-SQ for qemu-devel@nongnu.org; Thu, 29 Sep 2016 09:48:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bpbgu-0007m3-Q9 for qemu-devel@nongnu.org; Thu, 29 Sep 2016 09:48:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53816) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpbgu-0007lo-HR for qemu-devel@nongnu.org; Thu, 29 Sep 2016 09:48:00 -0400 From: Markus Armbruster References: <1473839464-8670-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1473839464-8670-3-git-send-email-caoj.fnst@cn.fujitsu.com> Date: Thu, 29 Sep 2016 15:47:56 +0200 In-Reply-To: <1473839464-8670-3-git-send-email-caoj.fnst@cn.fujitsu.com> (Cao jin's message of "Wed, 14 Sep 2016 15:50:58 +0800") Message-ID: <87bmz695rn.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v3 2/8] hcd-xhci: check & correct param before using it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin Cc: qemu-devel@nongnu.org, Marcel Apfelbaum , "Michael S. Tsirkin" , Gerd Hoffmann Cao jin 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 > CC: Markus Armbruster > CC: Marcel Apfelbaum > CC: Michael S. Tsirkin > Signed-off-by: Cao jin > --- > 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.