| Message ID | c6418267e763286248166985b25b1dc0a357fc4c.1318843694.git.jan.kiszka@siemens.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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;
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(-)