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

[Michael S. Tsirkin]

On Thu, Mar 03, 2016 at 01:19:09PM +0200, Marcel Apfelbaum wrote:
> On 03/03/2016 12:45 PM, Michael S. Tsirkin wrote:
> >On Thu, Mar 03, 2016 at 12:12:27PM +0200, Marcel Apfelbaum wrote:
> >>>>+int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int 
> >>>>nr_vectors,
> >>>>+             bool msi64bit, bool msi_per_vector_mask, Error **errp)
> >>>>  {
> >>>>      unsigned int vectors_order;
> >>>>-    uint16_t flags;
> >>>>+    uint16_t flags; /* Message Control register value */
> >>>>      uint8_t cap_size;
> >>>>      int config_offset;
> >>>>
> >>>>      if (!msi_supported) {
> >>>>+        error_setg(errp, "MSI is not supported by interrupt controller");
> >>>>          return -ENOTSUP;
> >>>
> >>>First failure mode: board does not support MSI (-ENOTSUP).
> >>>
> >>>Question to the PCI guys: why is this even an error?  A device with
> >>>capability MSI should work just fine in such a board.
> >>
> >>Hi Markus,
> >>
> >>Adding Jan Kiszka, maybe he can help.
> >>
> >>That's a fair question. Is there any history for this decision?
> >>The board not supporting MSI has nothing to do with the capability being 
> >>there.
> >>The HW should not change because the board doe not support it.
> >>
> >>The capability should be present but not active.
> >
> >Digging in git log will tell you why we have the msi_supported flag:
> >
> >commit 02eb84d0ec97f183ac23ee939403a139e8849b1d ("qemu/pci: MSI-X support 
> >functions")
> >
> >     This is a safety measure to avoid breaking platforms which should 
> > support
> >     MSI-X but currently lack this in the interrupt controller emulation.
> >
> >in other words, on some platforms we must hide msi support from devices
> >because otherwise guests will try to use it, and our emulation is
> >incomplete.
> 
> 
> OK, thanks. So the flag should be "msi_broken" or 
> "msi_present_but_not_implemented" and not
> "msi_supported" that leads (at least me) to the assumption that some platform 
> *does not support msi*
> rather than it supports it, but we don't emulate it.
> 
> 
> >
> >And the conclusion from that is that for msi_init to fail silently is
> >at the moment the right thing to do.
> 
> But this is not the only thing we do, we are modifying the PCI devices. We 
> could fail starting the VM
> if a device supporting MSI is added on a platform with broken msi, but this 
> will prevent us to use
> assigned devices. Emulated devices should be created with a specific 
> "msi=off" flag.
> 
> Thanks,
> Marcel

That will just break a bunch of valid configurations, for no real
benefit to users.

> >
> >The only other reason for it to fail is pci config space corruption,
> >this probably never happens in practice.
> >