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

[Michael S. Tsirkin]

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?

> ---
>  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