Patchwork [RFC,42/45] msix: Introduce msix_init_simple

login
register
mail settings
Submitter Jan Kiszka
Date Oct. 17, 2011, 9:28 a.m.
Message ID <c6418267e763286248166985b25b1dc0a357fc4c.1318843694.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/120126/
State New
Headers show

Comments

Jan Kiszka - Oct. 17, 2011, 9:28 a.m.
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 <jan.kiszka@siemens.com>
---
 hw/ivshmem.c    |    6 +-----
 hw/msix.c       |   35 ++++++++++++++---------------------
 hw/msix.h       |    7 +++----
 hw/virtio-pci.c |   15 +++++----------
 hw/virtio-pci.h |    1 -
 5 files changed, 23 insertions(+), 41 deletions(-)
Michael S. Tsirkin - Oct. 17, 2011, 11:22 a.m.
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 <jan.kiszka@siemens.com>

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?

> ---
>  hw/ivshmem.c    |    6 +-----
>  hw/msix.c       |   35 ++++++++++++++---------------------
>  hw/msix.h       |    7 +++----
>  hw/virtio-pci.c |   15 +++++----------
>  hw/virtio-pci.h |    1 -
>  5 files changed, 23 insertions(+), 41 deletions(-)
> 
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index a402c98..d9dbd18 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -65,7 +65,6 @@ typedef struct IVShmemState {
>       */
>      MemoryRegion bar;
>      MemoryRegion ivshmem;
> -    MemoryRegion msix_bar;
>      uint64_t ivshmem_size; /* size of shared memory region */
>      int shm_fd; /* shared memory file descriptor */
>  
> @@ -539,10 +538,7 @@ static void ivshmem_setup_msi(IVShmemState *s)
>  {
>      /* allocate the MSI-X vectors */
>  
> -    memory_region_init(&s->msix_bar, "ivshmem-msix", 4096);
> -    if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) {
> -        pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
> -                         &s->msix_bar);
> +    if (!msix_init_simple(&s->dev, s->vectors, 1)) {
>          IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
>      } else {
>          IVSHMEM_DPRINTF("msix initialization failed\n");
> diff --git a/hw/msix.c b/hw/msix.c
> index bccd8b1..258b9c1 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -244,17 +244,6 @@ static const MemoryRegionOps msix_mmio_ops = {
>      },
>  };
>  
> -static void msix_mmio_setup(PCIDevice *d, MemoryRegion *bar)
> -{
> -    uint8_t *config = d->config + d->msix_cap;
> -    uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
> -    uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
> -    /* TODO: for assigned devices, we'll want to make it possible to map
> -     * pending bits separately in case they are in a separate bar. */
> -
> -    memory_region_add_subregion(bar, offset, &d->msix_mmio);
> -}
> -
>  static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>  {
>      int vector;
> @@ -272,11 +261,9 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>      }
>  }
>  
> -/* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is
> - * modified, it should be retrieved with msix_bar_size. */
> -int msix_init(struct PCIDevice *dev, unsigned short nentries,
> -              MemoryRegion *bar,
> -              unsigned bar_nr, unsigned bar_size)
> +/* Initialize the MSI-X structures in a single dedicated BAR
> + * and register it. */
> +int msix_init_simple(PCIDevice *dev, unsigned short nentries, unsigned bar_nr)
>  {
>      int ret;
>  
> @@ -296,14 +283,16 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>                            "msix", MSIX_PAGE_SIZE);
>  
>      dev->msix_entries_nr = nentries;
> -    ret = msix_add_config(dev, nentries, bar_nr, bar_size);
> +    ret = msix_add_config(dev, nentries, bar_nr, 0);
>      if (ret)
>          goto err_config;
>  
>      dev->msix_cache = g_malloc0(nentries * sizeof *dev->msix_cache);
>  
>      dev->cap_present |= QEMU_PCI_CAP_MSIX;
> -    msix_mmio_setup(dev, bar);
> +
> +    pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
> +                     &dev->msix_mmio);
>      return 0;
>  
>  err_config:
> @@ -315,10 +304,10 @@ err_config:
>  }
>  
>  /* Clean up resources for the device. */
> -int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> +void msix_uninit(PCIDevice *dev, MemoryRegion *bar)
>  {
>      if (!msix_present(dev)) {
> -        return 0;
> +        return;
>      }
>      pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
>      dev->msix_cap = 0;
> @@ -332,7 +321,11 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
>      g_free(dev->msix_cache);
>  
>      dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> -    return 0;
> +}
> +
> +void msix_uninit_simple(PCIDevice *dev)
> +{
> +    msix_uninit(dev, &dev->msix_mmio);
>  }
>  
>  void msix_save(PCIDevice *dev, QEMUFile *f)
> diff --git a/hw/msix.h b/hw/msix.h
> index dfc6087..56e7ba5 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -4,14 +4,13 @@
>  #include "qemu-common.h"
>  #include "pci.h"
>  
> -int msix_init(PCIDevice *pdev, unsigned short nentries,
> -              MemoryRegion *bar,
> -              unsigned bar_nr, unsigned bar_size);
> +int msix_init_simple(PCIDevice *dev, unsigned short nentries, unsigned bar_nr);
>  
>  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
>                         uint32_t old_val, int len);
>  
> -int msix_uninit(PCIDevice *d, MemoryRegion *bar);
> +void msix_uninit(PCIDevice *d, MemoryRegion *bar);
> +void msix_uninit_simple(PCIDevice *d);
>  
>  void msix_save(PCIDevice *dev, QEMUFile *f);
>  void msix_load(PCIDevice *dev, QEMUFile *f);
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 5004d7d..6fe2b5e 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -713,13 +713,10 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
>      pci_set_word(config + 0x2e, vdev->device_id);
>      config[0x3d] = 1;
>  
> -    memory_region_init(&proxy->msix_bar, "virtio-msix", 4096);
> -    if (vdev->nvectors && !msix_init(&proxy->pci_dev, vdev->nvectors,
> -                                     &proxy->msix_bar, 1, 0)) {
> -        pci_register_bar(&proxy->pci_dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
> -                         &proxy->msix_bar);
> -    } else
> +    if (vdev->nvectors &&
> +        msix_init_simple(&proxy->pci_dev, vdev->nvectors, 1)) {
>          vdev->nvectors = 0;
> +    }
>  
>      proxy->pci_dev.config_write = virtio_write_config;
>  
> @@ -766,12 +763,10 @@ static int virtio_blk_init_pci(PCIDevice *pci_dev)
>  static int virtio_exit_pci(PCIDevice *pci_dev)
>  {
>      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> -    int r;
>  
>      memory_region_destroy(&proxy->bar);
> -    r = msix_uninit(pci_dev, &proxy->msix_bar);
> -    memory_region_destroy(&proxy->msix_bar);
> -    return r;
> +    msix_uninit_simple(pci_dev);
> +    return 0;
>  }
>  
>  static int virtio_blk_exit_pci(PCIDevice *pci_dev)
> diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
> index 14c10f7..5af1c8c 100644
> --- a/hw/virtio-pci.h
> +++ b/hw/virtio-pci.h
> @@ -22,7 +22,6 @@ typedef struct {
>      PCIDevice pci_dev;
>      VirtIODevice *vdev;
>      MemoryRegion bar;
> -    MemoryRegion msix_bar;
>      uint32_t flags;
>      uint32_t class_code;
>      uint32_t nvectors;
> -- 
> 1.7.3.4
Jan Kiszka - Oct. 17, 2011, 11:27 a.m.
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 <jan.kiszka@siemens.com>
> 
> 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
Michael S. Tsirkin - Oct. 17, 2011, 2:28 p.m.
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 <jan.kiszka@siemens.com>
> > 
> > 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?

> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
Jan Kiszka - Oct. 17, 2011, 7:21 p.m.
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 <jan.kiszka@siemens.com>
>>>
>>> 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

Patch

diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index a402c98..d9dbd18 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -65,7 +65,6 @@  typedef struct IVShmemState {
      */
     MemoryRegion bar;
     MemoryRegion ivshmem;
-    MemoryRegion msix_bar;
     uint64_t ivshmem_size; /* size of shared memory region */
     int shm_fd; /* shared memory file descriptor */
 
@@ -539,10 +538,7 @@  static void ivshmem_setup_msi(IVShmemState *s)
 {
     /* allocate the MSI-X vectors */
 
-    memory_region_init(&s->msix_bar, "ivshmem-msix", 4096);
-    if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) {
-        pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
-                         &s->msix_bar);
+    if (!msix_init_simple(&s->dev, s->vectors, 1)) {
         IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
     } else {
         IVSHMEM_DPRINTF("msix initialization failed\n");
diff --git a/hw/msix.c b/hw/msix.c
index bccd8b1..258b9c1 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -244,17 +244,6 @@  static const MemoryRegionOps msix_mmio_ops = {
     },
 };
 
-static void msix_mmio_setup(PCIDevice *d, MemoryRegion *bar)
-{
-    uint8_t *config = d->config + d->msix_cap;
-    uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
-    uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
-    /* TODO: for assigned devices, we'll want to make it possible to map
-     * pending bits separately in case they are in a separate bar. */
-
-    memory_region_add_subregion(bar, offset, &d->msix_mmio);
-}
-
 static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
 {
     int vector;
@@ -272,11 +261,9 @@  static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
     }
 }
 
-/* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is
- * modified, it should be retrieved with msix_bar_size. */
-int msix_init(struct PCIDevice *dev, unsigned short nentries,
-              MemoryRegion *bar,
-              unsigned bar_nr, unsigned bar_size)
+/* Initialize the MSI-X structures in a single dedicated BAR
+ * and register it. */
+int msix_init_simple(PCIDevice *dev, unsigned short nentries, unsigned bar_nr)
 {
     int ret;
 
@@ -296,14 +283,16 @@  int msix_init(struct PCIDevice *dev, unsigned short nentries,
                           "msix", MSIX_PAGE_SIZE);
 
     dev->msix_entries_nr = nentries;
-    ret = msix_add_config(dev, nentries, bar_nr, bar_size);
+    ret = msix_add_config(dev, nentries, bar_nr, 0);
     if (ret)
         goto err_config;
 
     dev->msix_cache = g_malloc0(nentries * sizeof *dev->msix_cache);
 
     dev->cap_present |= QEMU_PCI_CAP_MSIX;
-    msix_mmio_setup(dev, bar);
+
+    pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
+                     &dev->msix_mmio);
     return 0;
 
 err_config:
@@ -315,10 +304,10 @@  err_config:
 }
 
 /* Clean up resources for the device. */
-int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
+void msix_uninit(PCIDevice *dev, MemoryRegion *bar)
 {
     if (!msix_present(dev)) {
-        return 0;
+        return;
     }
     pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
     dev->msix_cap = 0;
@@ -332,7 +321,11 @@  int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
     g_free(dev->msix_cache);
 
     dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
-    return 0;
+}
+
+void msix_uninit_simple(PCIDevice *dev)
+{
+    msix_uninit(dev, &dev->msix_mmio);
 }
 
 void msix_save(PCIDevice *dev, QEMUFile *f)
diff --git a/hw/msix.h b/hw/msix.h
index dfc6087..56e7ba5 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -4,14 +4,13 @@ 
 #include "qemu-common.h"
 #include "pci.h"
 
-int msix_init(PCIDevice *pdev, unsigned short nentries,
-              MemoryRegion *bar,
-              unsigned bar_nr, unsigned bar_size);
+int msix_init_simple(PCIDevice *dev, unsigned short nentries, unsigned bar_nr);
 
 void msix_write_config(PCIDevice *pci_dev, uint32_t address,
                        uint32_t old_val, int len);
 
-int msix_uninit(PCIDevice *d, MemoryRegion *bar);
+void msix_uninit(PCIDevice *d, MemoryRegion *bar);
+void msix_uninit_simple(PCIDevice *d);
 
 void msix_save(PCIDevice *dev, QEMUFile *f);
 void msix_load(PCIDevice *dev, QEMUFile *f);
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 5004d7d..6fe2b5e 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -713,13 +713,10 @@  void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
     pci_set_word(config + 0x2e, vdev->device_id);
     config[0x3d] = 1;
 
-    memory_region_init(&proxy->msix_bar, "virtio-msix", 4096);
-    if (vdev->nvectors && !msix_init(&proxy->pci_dev, vdev->nvectors,
-                                     &proxy->msix_bar, 1, 0)) {
-        pci_register_bar(&proxy->pci_dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
-                         &proxy->msix_bar);
-    } else
+    if (vdev->nvectors &&
+        msix_init_simple(&proxy->pci_dev, vdev->nvectors, 1)) {
         vdev->nvectors = 0;
+    }
 
     proxy->pci_dev.config_write = virtio_write_config;
 
@@ -766,12 +763,10 @@  static int virtio_blk_init_pci(PCIDevice *pci_dev)
 static int virtio_exit_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
-    int r;
 
     memory_region_destroy(&proxy->bar);
-    r = msix_uninit(pci_dev, &proxy->msix_bar);
-    memory_region_destroy(&proxy->msix_bar);
-    return r;
+    msix_uninit_simple(pci_dev);
+    return 0;
 }
 
 static int virtio_blk_exit_pci(PCIDevice *pci_dev)
diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index 14c10f7..5af1c8c 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -22,7 +22,6 @@  typedef struct {
     PCIDevice pci_dev;
     VirtIODevice *vdev;
     MemoryRegion bar;
-    MemoryRegion msix_bar;
     uint32_t flags;
     uint32_t class_code;
     uint32_t nvectors;