Re: [Qemu-devel] [RFC][PATCH 42/45] msix: Introduce msix_init_simple

[Michael S. Tsirkin]

On Mon, Oct 17, 2011 at 09:21:34PM +0200, Jan Kiszka wrote:
> On 2011-10-17 16:28, Michael S. Tsirkin wrote:
> > On Mon, Oct 17, 2011 at 01:27:31PM +0200, Jan Kiszka wrote:
> >> On 2011-10-17 13:22, Michael S. Tsirkin wrote:
> >>> On Mon, Oct 17, 2011 at 11:28:16AM +0200, Jan Kiszka wrote:
> >>>> Devices models are usually not interested in specifying MSI-X
> >>>> configuration details beyond the number of vectors to provide and the
> >>>> BAR number to use. Layout of an exclusively used BAR and its
> >>>> registration can also be handled centrally.
> >>>>
> >>>> This is the purpose of msix_init_simple. It provides handy services to
> >>>> the existing users. Future users like device assignment may require more
> >>>> detailed setup specification. For them we will (re-)introduce msix_init
> >>>> with the full list of configuration option (in contrast to the current
> >>>> code).
> >>>>
> >>>> Signed-off-by: Jan Kiszka <address@hidden>
> >>>
> >>> Well, this seems a bit of a code churn then, doesn't it?
> >>> We are also discussing using memory BAR for virtio-pci for other
> >>> stuff besides MSI-X, so the last user of the _simple variant
> >>> will be ivshmem then?
> >>
> >> We will surely see more MSI-X users over the time. Not sure if they all
> >> mix their MSIX-X BARs with other stuff. But e.g. the e1000 variant I
> >> have here does not. So there should be users in the future.
> >>
> >> Jan
> > 
> > Question is, how hard is to pass in the BAR and the offset?
> 
> That is trivial. But have a look at the final simple implementation. It
> also manages the container memory region for table and PBA and
> registers/unregisters that container as BAR. So there is measurable
> added-value.
> 
> Jan
> 

Yes, I agree. In particular it's not very nice that the user has to know
the size of the bar to create. But the API is very unfortunate IMO.

I am also more interested in solutions that help all devices
and not just those that have a dedicated bar for msix + pba.

We should probably pass in the size of the memory region allocated to
the msix table, and verify that the table fits there.
We can also avoid passing in bar number, like this:

diff --git a/hw/pci.c b/hw/pci.c
index 749e8d8..d0d893e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -903,6 +903,17 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
         : pci_dev->bus->address_space_mem;
 }
 
+int pci_get_bar_nr(PCIDevice *pci_dev, MemoryRegion *bar)
+{
+    int region_num;
+    for (region_num = 0; region_num < PCI_NUM_REGIONS; ++region_num) {
+        if (pci_dev->io_regions[region_num].memory == bar) {
+            return region_num;
+        }
+    }
+    return -1;
+}
+
 pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num)
 {
     return pci_dev->io_regions[region_num].addr;
-- 
MST