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

[Marcel Apfelbaum]

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

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