Re: [Qemu-trivial] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Feb 19, 2019 at 06:19:30PM -0500, Paolo Bonzini wrote:
> 
> > > Makes sense, but it is also abstraction time. :)  What if instead there
> > > was a function
> > > 
> > > void msi_allocate_irqs(PCIDevice *pdev, int num, bool fallback_to_intx);
> > > 
> > > and then ich.c did
> > > 
> > >     irqs = msi_allocate_irqs(pdev, 1, true);
> > >     s->irq = irqs[0];
> > >     g_free(irqs);
> > > 
> > > ?  "if msi_enabled raise MSI else raise INTX" is really a common idiom.
> > > 
> > > Thanks,
> > > 
> > > Paolo
> > 
> > Maybe it is but the specific issue is not about fallback to INTX of PCI
> > (is the fallback broken for ahci? I don't know).
> 
> It works, the above is just a new abstraction.
> 
> > The trick is there's no pdev at all.
> 
> The trick :) is that in ich.c there is a pdev.  Right now we are assigning to
> s->irq either the INTX irq (if PCI) or a sysbus irq (if sysbus), but then
> we need to know about MSI with a wrapper around s->irq.

Oh you mean just for PCI.

> Instead, my suggestion is to put the wrapper in the PCI core as a qemu_irq
> callback---or perhaps in ich.c, but anyway ahci.c should not care that there
> could be a PCI AHCI device and it would have two different interrupt modes.

I like it very much that devices call pci_set_irq, I'd rather not
have callbacks.


I think the wrapper thay calls either pci_set_irq isn't a problem,
problem is MSI/X has multiple vectors, INTX doesn't.
So for many devices there's something extra that happens
just in one mode but not the other to deal with multiple vectors.

So I don't think it can be an abstraction that everyone
uses. But yes it can be a helper function.

In fact mptsas_update_interrupt seems not to be
PCI spec compliant: it sets both MSI and INTX.

CC original contributor with this question.



> In fact, doing this would also remove the need for s->container, I think.
> 
> Paolo