Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> writes:

> On Fri, Mar 04, 2016 at 09:42:02AM +0100, Markus Armbruster wrote:
>> Plugging an MSI-capable device into an MSI-incapable board works just
>> fine, both for physical and for virtual hardware.  What doesn't work is
>> plugging an MSI-capable device into an MSI-capable board with *broken*
>> MSI support.
>> 
>> As a convenience feature, we summarily and silently remove a device's
>> MSI capability when we detect such a broken board.  At least that's what
>> the commit message you quoted claims.
>
> And this makes sense, right?

Yes, except for the part where we ignore the user's explicit orders,
and, to a lesser degree, for the part where we create variants of
physical devices that don't exist physically.

>> In reality, we remove it not just for broken boards, but even for
>> MSI-incapable boards.
>> 
>> I take issue with "summarily and silently", and "even for MSI-incapable
>> boards".
>> 
>> When multiple variants of a device exist, and the user didn't ask for a
>> specific one, then picking the one that works best with the board is
>> just fine.
>> 
>> It's absolutely not fine when the user did ask for a specific one.  When
>> I ask for msi=on, I mean it.  If it can't work with this board, tell me.
>> But silently ignoring my orders is a bug.
>
> I agree. msi is not the only case either.  We really need - for any boolean
> flag - a way to figure out whether it was set by user.
> With that in place we could fix it.

QMP provides that directly as "optional bool", but qdev properties do
"optional" diffently, and when you see the default value, you don't know
whether it comes from the user or not.

Another solution is an on/off/auto type.  We got it already in the QAPI
schema, as OnOffAuto, and my recent "[PATCH 32/38] qdev: New
DEFINE_PROP_ON_OFF_AUTO" makes it available as qdev property.  With
default set to auto, we should be set.

> However, almost no one uses the msi/msi-x flag - we introduced
> them only for one reason - for backwards compatibility.
> The fact that each time we need a compatibility flag
> we also expose a new user interface is very unfortunate.
> IMO it was a design mistake made a long time ago and it won't
> be easy to fix now.

I agree there's no easy fix, but we can try to find a non-easy one.

For backward compatibility, we need to configure some device models for
certain machine types.  We use the only object configuration mechanism
we have: properties.  The properties we use for compatibility are all
exposed to the user.

We could easily provide a flag to mark properties private, and only
accept non-private properties at external interfaces.  This should help
stopping growth of the problem, but it's not an easy fix for reducing
it, because making a property private retroactively is problematic.  We
could have a flag to mark them deprecated instead, warn on use, remove
them from documentation, and perhaps drop them a couple of releases
later.

> And for the above reason I personally do not intend to
> spend time designing a specific hack just for the msi
> property.
>
>> It's fine to have emulations of MSI-capable boards where MSI doesn't yet
>> work.  Even if that means we have to reject MSI-capable devices.
>
> I don't know what does reject mean here. Removing msi capability?
> In that case I agree.

By "reject" I mean "reject the user's order to plug in an MSI-capable
device."

>> It's absolutely not fine to reject them for MSI-incapable boards, where
>> they'd work just fine.
>
> I think that as long as users did not ask for msi explicitly,
> and board is msi incapable, it does not matter much whether
> device has msi capability or not - guest will not try
> to use it anyway.

If the device comes in MSI-capable and MSI-incapable variants, and the
user didn't specify a variant, then picking one that fits the board is
fine.

If the device does not come in variants (and many physical devices
don't), then rejecting it just because it's MSI-capable and the board
isn't is not fine.

To fix, we'd have to limit what is now !msi_supported to the boards that
are nominally MSI-capable, except our emulation doesn't work, i.e. do
exactly what the commit message promised.

The PCI core encourages creating both variants, and most (but not all)
device models do, but:

>> Furthermore, I doubt the wisdom of creating virtual devices that don't
>> exist physically just to have something that works in our broken boards.
>> If the physical device exists in MSI-capable and MSI-incapable variants,
>> emulating both is fine.  But if it only ever existed MSI-capable, having
>> the PCI core(!) drop the MSI capability is a questionable idea.  I
>> suspect that the need for this dubious hack would be much smaller if we
>> didn't foolishly treat every MSI-incapable board as broken MSI-capable
>> board.
>> 
>> If you conclude that cleaning up this carelessly made mess is not worth
>> the bother now, then at least explain the mess in the code, please.
>> It's not obvious, and figuring out what's going on and why it is the way
>> it is has taken me several hours, and Marcel's help.
>
> I think it's worth cleaning up, or at least documenting.
> Fixing it will take much more than the patch proposed here,
> and we can not start with this patch as it will cause
> regressions.
> Adding a comment won't be too much work.
> How about the below?
>
> -->
>
> msi_supported -> msi_nonbroken
>
> Rename controller flag to make it clearer what it means.
> Add some documentation as well.
>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
>
> ---
>
> diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
> index 50e452b..8124908 100644
> --- a/include/hw/pci/msi.h
> +++ b/include/hw/pci/msi.h
> @@ -29,7 +29,7 @@ struct MSIMessage {
>      uint32_t data;
>  };
>  
> -extern bool msi_supported;
> +extern bool msi_nonbroken;
>  
>  void msi_set_message(PCIDevice *dev, MSIMessage msg);
>  MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector);
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> index 694d398..3c7c8fa 100644
> --- a/hw/i386/kvm/apic.c
> +++ b/hw/i386/kvm/apic.c
> @@ -186,7 +186,7 @@ static void kvm_apic_realize(DeviceState *dev, Error 
> **errp)
>                            APIC_SPACE_SIZE);
>  
>      if (kvm_has_gsi_routing()) {
> -        msi_supported = true;
> +        msi_nonbroken = true;
>      }
>  }
>  
> diff --git a/hw/i386/xen/xen_apic.c b/hw/i386/xen/xen_apic.c
> index 2b8d709..21d68ee 100644
> --- a/hw/i386/xen/xen_apic.c
> +++ b/hw/i386/xen/xen_apic.c
> @@ -44,7 +44,7 @@ static void xen_apic_realize(DeviceState *dev, Error **errp)
>      s->vapic_control = 0;
>      memory_region_init_io(&s->io_memory, OBJECT(s), &xen_apic_io_ops, s,
>                            "xen-apic-msi", APIC_SPACE_SIZE);
> -    msi_supported = true;
> +    msi_nonbroken = true;
>  }
>  
>  static void xen_apic_set_base(APICCommonState *s, uint64_t val)
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index a299462..28c2ea5 100644
> --- a/hw/intc/apic.c
> +++ b/hw/intc/apic.c
> @@ -874,7 +874,7 @@ static void apic_realize(DeviceState *dev, Error **errp)
>      s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, apic_timer, s);
>      local_apics[s->idx] = s;
>  
> -    msi_supported = true;
> +    msi_nonbroken = true;
>  }
>  
>  static void apic_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
> index 70c0b97..ebd368b 100644
> --- a/hw/intc/arm_gicv2m.c
> +++ b/hw/intc/arm_gicv2m.c
> @@ -148,7 +148,7 @@ static void gicv2m_realize(DeviceState *dev, Error **errp)
>          sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->spi[i]);
>      }
>  
> -    msi_supported = true;
> +    msi_nonbroken = true;
>      kvm_gsi_direct_mapping = true;
>      kvm_msi_via_irqfd_allowed = kvm_irqfds_enabled();
>  }
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index 903888c..7685250 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -1375,7 +1375,7 @@ static void fsl_common_init(OpenPICState *opp)
>  
>      opp->irq_msi = 224;
>  
> -    msi_supported = true;
> +    msi_nonbroken = true;
>      for (i = 0; i < opp->fsl->max_ext; i++) {
>          opp->src[i].level = false;
>      }
> diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
> index 4dcdb61..778af4a 100644
> --- a/hw/intc/openpic_kvm.c
> +++ b/hw/intc/openpic_kvm.c
> @@ -239,7 +239,7 @@ static void kvm_openpic_realize(DeviceState *dev, Error 
> **errp)
>      memory_listener_register(&opp->mem_listener, &address_space_memory);
>  
>      /* indicate pic capabilities */
> -    msi_supported = true;
> +    msi_nonbroken = true;
>      kvm_kernel_irqchip = true;
>      kvm_async_interrupts_allowed = true;
>  
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 100bb5e..862a2366 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -72,7 +72,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>          goto slotid_error;
>      }
>      if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
> -        msi_supported) {
> +        msi_nonbroken) {
>          err = msi_init(dev, 0, 1, true, true);
>          if (err < 0) {
>              goto msi_error;
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index 85f21b8..e0e64c2 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -34,8 +34,21 @@
>  
>  #define PCI_MSI_VECTORS_MAX     32
>  
> -/* Flag for interrupt controller to declare MSI/MSI-X support */
> -bool msi_supported;
> +/*
> + * Flag for interrupt controllers to declare broken MSI/MSI-X support.
> + * values: false - broken; true - non-broken.
> + *
> + * Setting this flag to false will remove MSI/MSI-X capability from all 
> devices.
> + *
> + * It is preferrable for controllers to set this to true (non-broken) even if
> + * they do not actually support MSI/MSI-X: guests normally probe the 
> controller
> + * type and do not attempt to enable MSI/MSI-X with interrupt controllers not
> + * supporting such, so removing the capability is not required, and
> + * it seems cleaner to have a given device look the same for all boards.
> + *
> + * TODO: some existing controllers violate the above rule. Identify and fix 
> them.
> + */
> +bool msi_nonbroken;

Good description.  I'd use FIXME rather than TODO, but that's detail.

>  
>  /* If we get rid of cap allocator, we won't need this. */
>  static inline uint8_t msi_cap_sizeof(uint16_t flags)
> @@ -160,7 +173,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
>      uint8_t cap_size;
>      int config_offset;
>  
> -    if (!msi_supported) {
> +    if (!msi_nonbroken) {
>          return -ENOTSUP;
>      }
>  
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 537fdba..b75f0e9 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -249,7 +249,7 @@ int msix_init(struct PCIDevice *dev, unsigned short 
> nentries,
>      uint8_t *config;
>  
>      /* Nothing to do if MSI is not supported by interrupt controller */
> -    if (!msi_supported) {
> +    if (!msi_nonbroken) {
>          return -ENOTSUP;
>      }
>  
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e9d4abf..c4fb206 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -439,7 +439,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>      _FDT((fdt_property_cell(fdt, "rtas-event-scan-rate",
>                              RTAS_EVENT_SCAN_RATE)));
>  
> -    if (msi_supported) {
> +    if (msi_nonbroken) {
>          _FDT((fdt_property(fdt, "ibm,change-msix-capable", NULL, 0)));
>      }
>  
> @@ -1743,7 +1743,7 @@ static void ppc_spapr_init(MachineState *machine)
>      bool kernel_le = false;
>      char *filename;
>  
> -    msi_supported = true;
> +    msi_nonbroken = true;
>  
>      QLIST_INIT(&spapr->phbs);
>  
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index e8edad3..3fc7895 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1790,7 +1790,7 @@ void spapr_pci_rtas_init(void)
>                          rtas_ibm_read_pci_config);
>      spapr_rtas_register(RTAS_IBM_WRITE_PCI_CONFIG, "ibm,write-pci-config",
>                          rtas_ibm_write_pci_config);
> -    if (msi_supported) {
> +    if (msi_nonbroken) {
>          spapr_rtas_register(RTAS_IBM_QUERY_INTERRUPT_SOURCE_NUMBER,
>                              "ibm,query-interrupt-source-number",
>                              rtas_ibm_query_interrupt_source_number);
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index dba0202..f5f679f 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -597,7 +597,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, 
> void *data)
>      k->init = s390_pcihost_init;
>      hc->plug = s390_pcihost_hot_plug;
>      hc->unplug = s390_pcihost_hot_unplug;
> -    msi_supported = true;
> +    msi_nonbroken = true;
>  }
>  
>  static const TypeInfo s390_pcihost_info = {

Much appreciated!

Reviewed-by: Markus Armbruster <address@hidden>