From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33475) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c2GMQ-0000bW-IQ for qemu-devel@nongnu.org; Thu, 03 Nov 2016 07:39:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c2GMM-0004cD-LZ for qemu-devel@nongnu.org; Thu, 03 Nov 2016 07:39:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60444) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c2GMM-0004bz-Dj for qemu-devel@nongnu.org; Thu, 03 Nov 2016 07:39:06 -0400 References: <1478145997-28865-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1478145997-28865-4-git-send-email-caoj.fnst@cn.fujitsu.com> From: Marcel Apfelbaum Message-ID: <99c7ca3e-47ac-7c35-99b8-93fbda09a48f@redhat.com> Date: Thu, 3 Nov 2016 13:38:48 +0200 MIME-Version: 1.0 In-Reply-To: <1478145997-28865-4-git-send-email-caoj.fnst@cn.fujitsu.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 03/10] 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: Cao jin , qemu-devel@nongnu.org Cc: Jiri Pirko , Gerd Hoffmann , Dmitry Fleytman , Jason Wang , "Michael S. Tsirkin" , Hannes Reinecke , Paolo Bonzini , Alex Williamson , Markus Armbruster On 11/03/2016 06:06 AM, Cao jin wrote: > msix_init() reports errors with error_report(), which is wrong when > it's used in realize(). The same issue was fixed for msi_init() in > commit 1108b2f. > > For some devices(like e1000e, vmxnet3) who won't fail because of > msix_init's failure, suppress the error report by passing NULL error object. > > Bonus: add comment for msix_init. > > CC: Jiri Pirko > CC: Gerd Hoffmann > CC: Dmitry Fleytman > CC: Jason Wang > CC: Michael S. Tsirkin > CC: Hannes Reinecke > CC: Paolo Bonzini > CC: Alex Williamson > CC: Markus Armbruster > CC: Marcel Apfelbaum > > Reviewed-by: Markus Armbruster > Signed-off-by: Cao jin > --- > hw/block/nvme.c | 5 ++++- > hw/misc/ivshmem.c | 8 ++++---- > hw/net/e1000e.c | 6 +++++- > hw/net/rocker/rocker.c | 7 ++++++- > hw/net/vmxnet3.c | 8 ++++++-- > hw/pci/msix.c | 34 +++++++++++++++++++++++++++++----- > hw/scsi/megasas.c | 5 ++++- > hw/usb/hcd-xhci.c | 13 ++++++++----- > hw/vfio/pci.c | 4 +++- > hw/virtio/virtio-pci.c | 11 +++++------ > include/hw/pci/msix.h | 5 +++-- > 11 files changed, 77 insertions(+), 29 deletions(-) > [...] > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > index 52a4123..fada834 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -2360,9 +2360,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) > > if (megasas_use_msix(s) && > msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, > - &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { > + &s->mmio_io, b->mmio_bar, 0x3800, 0x68, &err)) { > + /*TODO: check msix_init's error, and should fail on msix=on */ Why this "TODO", can't we do something similar to other changes already done in this patch? > + error_report_err(err); > s->msix = ON_OFF_AUTO_OFF; > } > + > if (pci_is_express(dev)) { > pcie_endpoint_cap_init(dev, 0xa0); > } > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > index eb1dca5..05dc944 100644 > --- a/hw/usb/hcd-xhci.c > +++ b/hw/usb/hcd-xhci.c > @@ -3703,11 +3703,14 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) > } > > if (xhci->msix != ON_OFF_AUTO_OFF) { > - /* TODO check for errors */ > - msix_init(dev, xhci->numintrs, > - &xhci->mem, 0, OFF_MSIX_TABLE, > - &xhci->mem, 0, OFF_MSIX_PBA, > - 0x90); > + /* TODO check for errors, and should fail when msix=on */ > + ret = msix_init(dev, xhci->numintrs, > + &xhci->mem, 0, OFF_MSIX_TABLE, > + &xhci->mem, 0, OFF_MSIX_PBA, > + 0x90, &err); > + if (ret) { > + error_report_err(err); > + } > } > } > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index d7dbe0e..6fbd30f 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -1432,6 +1432,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) > static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp) > { > int ret; > + Error *err = NULL; > > vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) * > sizeof(unsigned long)); > @@ -1439,7 +1440,8 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp) > vdev->bars[vdev->msix->table_bar].region.mem, > vdev->msix->table_bar, vdev->msix->table_offset, > vdev->bars[vdev->msix->pba_bar].region.mem, > - vdev->msix->pba_bar, vdev->msix->pba_offset, pos); > + vdev->msix->pba_bar, vdev->msix->pba_offset, pos, > + &err); Do we pass the err pointer to msix_init, but we don't do anything with it? Also since we do have an *errp in the function already, I suggest renaming err -> local_err or something. (only if the series needs a re-spin) > if (ret < 0) { > if (ret == -ENOTSUP) { > return 0; > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 06831de..5acce38 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1693,13 +1693,12 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) > > if (proxy->nvectors) { > int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, > - proxy->msix_bar_idx); > + proxy->msix_bar_idx, errp); > + /* Any error other than -ENOTSUP(board's MSI support is broken) > + * is a programming error. */ > + assert(!err || err == -ENOTSUP); > if (err) { > - /* Notice when a system that supports MSIx can't initialize it. */ > - if (err != -ENOTSUP) { > - error_report("unable to init msix vectors to %" PRIu32, > - proxy->nvectors); > - } > + error_report_err(*errp); Why do we report the error here and we don't let the propagation mechanism do its thing? We can prep-end the current message, I think. > proxy->nvectors = 0; > } > } > diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h > index 048a29d..1f27658 100644 > --- a/include/hw/pci/msix.h > +++ b/include/hw/pci/msix.h > @@ -9,9 +9,10 @@ MSIMessage msix_get_message(PCIDevice *dev, unsigned int vector); > int msix_init(PCIDevice *dev, unsigned short nentries, > MemoryRegion *table_bar, uint8_t table_bar_nr, > unsigned table_offset, MemoryRegion *pba_bar, > - uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos); > + uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos, > + Error **errp); > int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > - uint8_t bar_nr); > + uint8_t bar_nr, Error **errp); > > void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len); > > Other than a few questions, the patch looks good to me. Thanks! Marcel