|
From: | Marcel Apfelbaum |
Subject: | Re: [Qemu-devel] [PATCH v5 03/10] pci: Convert msix_init() to Error and fix callers to check it |
Date: | Sat, 5 Nov 2016 18:52:36 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
On 11/04/2016 05:01 AM, Cao jin wrote:
On 11/03/2016 07:38 PM, Marcel Apfelbaum wrote:On 11/03/2016 06:06 AM, Cao jin wrote:[...]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?The 1st version of this series handles the error in this patch. look at the comments: http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg03192.html *First convert msix_init() without changing behavior, by having every caller of msix_init() immediately pass the error received to error_report_err(). Then clean up the callers one after the other.* So later, this patch looks like what it is now. I feel it also make this patch thinner, easier to review.
I thought it can be solved in the same way as in the other places, however I am OK with it.
+ 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.cdiff --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)yes, it maybe need a re-spin, thanksif (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.The original behaviour won't fail on msix init failure, so, report & free the Error keep the behaviour same as before, propagation will results in failing to create virtio device.
Got it Thanks, Marcel
Other than a few questions, the patch looks good to me. Thanks! Marcel
[Prev in Thread] | Current Thread | [Next in Thread] |