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.
+ 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
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)
yes, it maybe need a re-spin, thanks
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.
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.
Other than a few questions, the patch looks good to me.
Thanks!
Marcel
--
Yours Sincerely,
Cao jin
