[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_noti
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs |
Date: |
Tue, 19 Feb 2019 22:35:45 -0500 |
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