Cao jin <caoj.f...@cn.fujitsu.com> writes:
> msi_init() reports errors with error_report(), which is wrong
> when it's used in realize().
>
> Fix by converting it to Error.
>
> Fix its callers to handle failure instead of ignoring it.
>
> For those callers who don't handle the failure, it might happen:
> when user want msi on, but he doesn't get what he want because of
> msi_init fails silently.
>
> cc: Gerd Hoffmann <kra...@redhat.com>
> cc: John Snow <js...@redhat.com>
> cc: Dmitry Fleytman <dmi...@daynix.com>
> cc: Jason Wang <jasow...@redhat.com>
> cc: Michael S. Tsirkin <m...@redhat.com>
> cc: Hannes Reinecke <h...@suse.de>
> cc: Paolo Bonzini <pbonz...@redhat.com>
> cc: Alex Williamson <alex.william...@redhat.com>
> cc: Markus Armbruster <arm...@redhat.com>
> cc: Marcel Apfelbaum <mar...@redhat.com>
>
> Signed-off-by: Cao jin <caoj.f...@cn.fujitsu.com>
[...]
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 26119bf..5c57106 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -29,7 +29,7 @@
> #include "hw/scsi/scsi.h"
> #include "block/scsi.h"
> #include "trace.h"
> -
> +#include "qapi/error.h"
> #include "mfi.h"
>
> #define MEGASAS_VERSION_GEN1 "1.70"
> @@ -2330,6 +2330,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error
> **errp)
> MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s);
> uint8_t *pci_conf;
> int i, bar_type;
> + Error *err = NULL;
> + int ret;
>
> pci_conf = dev->config;
>
> @@ -2338,6 +2340,25 @@ static void megasas_scsi_realize(PCIDevice *dev, Error
> **errp)
> /* Interrupt pin 1 */
> pci_conf[PCI_INTERRUPT_PIN] = 0x01;
>
> + if (megasas_use_msi(s)) {
> + ret = msi_init(dev, 0x50, 1, true, false, &err);
> + /* Any error other than -ENOTSUP(board's MSI support is broken)
> + * is a programming error */
> + assert(!ret || ret == -ENOTSUP);
> + if (ret && s->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;
> + } else if (ret) {
> + s->msi = ON_OFF_AUTO_OFF;
> + }
> +
> + /* With msi=auto, we fall back to MSI off silently */
> + error_free(err);
Let's move this into the "else if (ret)" conditional, where the comment
serves to explain the s->msi = ON_OFF_AUTO_OFF.
> + }
> +
> memory_region_init_io(&s->mmio_io, OBJECT(s), &megasas_mmio_ops, s,
> "megasas-mmio", 0x4000);
> memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s,
> @@ -2345,10 +2366,6 @@ static void megasas_scsi_realize(PCIDevice *dev, Error
> **errp)
> memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
> "megasas-queue", 0x40000);
>
> - if (megasas_use_msi(s) &&
> - msi_init(dev, 0x50, 1, true, false)) {
> - s->msi = ON_OFF_AUTO_OFF;
> - }
> if (megasas_use_msix(s) &&
> msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
> &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
[...]
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index c2a387a..c384442 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1044,12 +1044,14 @@ static void
> pvscsi_init_msi(PVSCSIState *s)
> {
> int res;
> + Error *err = NULL;
> PCIDevice *d = PCI_DEVICE(s);
>
> res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS,
> - PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
> + PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &err);
> if (res < 0) {
> trace_pvscsi_init_msi_fail(res);
> + error_free(err);
> s->msi_used = false;
> } else {
> s->msi_used = true;
Since you're not doing anything with err:
pvscsi_init_msi(PVSCSIState *s)
{
int res;
PCIDevice *d = PCI_DEVICE(s);
res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS,
- PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
+ PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, NULL);
if (res < 0) {
trace_pvscsi_init_msi_fail(res);
s->msi_used = false;
} else {
s->msi_used = true;
[...]
