Re: [Qemu-devel] [PATCH v5 03/10] pci: Convert msix_init() to Error and fix callers to check it

2016-11-05 Thread Marcel Apfelbaum 

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, >mmio_io, b->mmio_bar, 0x2000,
-  >mmio_io, b->mmio_bar, 0x3800, 0x68)) {
+  >mmio_io, b->mmio_bar, 0x3800, 0x68, )) {
+/*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.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,
+);


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(>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

Re: [Qemu-devel] [PATCH v5 03/10] pci: Convert msix_init() to Error and fix callers to check it

2016-11-03 Thread Cao jin 



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, >mmio_io, b->mmio_bar, 0x2000,
-  >mmio_io, b->mmio_bar, 0x3800, 0x68)) {
+  >mmio_io, b->mmio_bar, 0x3800, 0x68, )) {
+/*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,
+);


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(>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

Re: [Qemu-devel] [PATCH v5 03/10] pci: Convert msix_init() to Error and fix callers to check it

2016-11-03 Thread Marcel Apfelbaum 

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, >mmio_io, b->mmio_bar, 0x2000,
-  >mmio_io, b->mmio_bar, 0x3800, 0x68)) {
+  >mmio_io, b->mmio_bar, 0x3800, 0x68, )) {
+/*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,
-  >mem, 0, OFF_MSIX_TABLE,
-  >mem, 0, OFF_MSIX_PBA,
-  0x90);
+/* TODO check for errors, and should fail when msix=on */
+ret = msix_init(dev, xhci->numintrs,
+>mem, 0, OFF_MSIX_TABLE,
+>mem, 0, OFF_MSIX_PBA,
+0x90, );
+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,
+);


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(>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,
-

[Qemu-devel] [PATCH v5 03/10] pci: Convert msix_init() to Error and fix callers to check it

2016-11-02 Thread Cao jin 
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/block/nvme.c b/hw/block/nvme.c
index d479fd2..2d703c8 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -831,6 +831,7 @@ static int nvme_init(PCIDevice *pci_dev)
 {
 NvmeCtrl *n = NVME(pci_dev);
 NvmeIdCtrl *id = >id_ctrl;
+Error *err = NULL;
 
 int i;
 int64_t bs_size;
@@ -872,7 +873,9 @@ static int nvme_init(PCIDevice *pci_dev)
 pci_register_bar(>parent_obj, 0,
 PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
 >iomem);
-msix_init_exclusive_bar(>parent_obj, n->num_queues, 4);
+if (msix_init_exclusive_bar(>parent_obj, n->num_queues, 4, )) {
+error_report_err(err);
+}
 
 id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
 id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 230e51b..ca6f804 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -749,13 +749,13 @@ static void ivshmem_reset(DeviceState *d)
 }
 }
 
-static int ivshmem_setup_interrupts(IVShmemState *s)
+static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
 {
 /* allocate QEMU callback data for receiving interrupts */
 s->msi_vectors = g_malloc0(s->vectors * sizeof(MSIVector));
 
 if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
-if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
+if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, errp)) {
 return -1;
 }
 
@@ -897,8 +897,8 @@ static void ivshmem_common_realize(PCIDevice *dev, Error 
**errp)
 qemu_chr_fe_set_handlers(>server_chr, ivshmem_can_receive,
  ivshmem_read, NULL, s, NULL, true);
 
-if (ivshmem_setup_interrupts(s) < 0) {
-error_setg(errp, "failed to initialize interrupts");
+if (ivshmem_setup_interrupts(s, errp) < 0) {
+error_prepend(errp, "Failed to initialize interrupts: ");
 return;
 }
 }
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 4994e1c..74cbbef 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -292,7 +292,11 @@ e1000e_init_msix(E1000EState *s)
 E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
 >msix,
 E1000E_MSIX_IDX, E1000E_MSIX_PBA,
-0xA0);
+0xA0, NULL);
+
+/* Any error other than -ENOTSUP(board's MSI support is broken)
+ * is a programming error. Fall back to INTx silently on -ENOTSUP */
+assert(!res || res == -ENOTSUP);
 
 if (res < 0) {
 trace_e1000e_msix_init_fail(res);
diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index e9d215a..8f829f2 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -1256,14 +1256,19 @@ static int rocker_msix_init(Rocker *r)
 {
 PCIDevice *dev = PCI_DEVICE(r);
 int err;
+Error *local_err = NULL;
 
 err = msix_init(dev, ROCKER_MSIX_VEC_COUNT(r->fp_ports),
 >msix_bar,
 ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_TABLE_OFFSET,
 >msix_bar,
 ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
-0);
+0, _err);
+/* Any error other than -ENOTSUP(board's MSI support is broken)
+ * is a programming error. */
+assert(!err || err == -ENOTSUP);
 if (err) {
+error_report_err(local_err);
 return err;
 }
 
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 92f6af9..a433cc0 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2191,10 +2191,14 @@