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.kis...@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
