| Message ID | 20120614045118.11034.11599.stgit@bling.home |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote: > msi_init() takes over a BAR without really specifying or allowing > specification of how it does so. Instead, let's split it into > two interfaces, one fully specified, and one trivially easy. This > implements the latter. msix_init_exclusive_bar() takes over > allocating and filling a PCI BAR _exclusively_ for the use of MSIX. > When used, the matching msi_uninit_exclusive_bar() should be used > to tear it down. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > > hw/msix.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > hw/msix.h | 3 +++ > hw/pci.h | 2 ++ > 3 files changed, 48 insertions(+) > > diff --git a/hw/msix.c b/hw/msix.c > index b64f109..a4cdfb0 100644 > --- a/hw/msix.c > +++ b/hw/msix.c > @@ -299,6 +299,41 @@ err_config: > return ret; > } > > +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > + uint8_t bar_nr) > +{ > + int ret; > + char *name; > + > + /* > + * Migration compatibility dictates that this remains a 4k > + * BAR with the vector table in the lower half and PBA in > + * the upper half. > + */ > + if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) { > + return -EINVAL; > + } > + > + if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) { > + return -ENOMEM; > + } > + I think we should avoid spaces in names. Also let's use lower case. And BAR is confusing IMO: it's not a base address register, it's the region. So I would prefer simply: "%s-msix". > + memory_region_init(&dev->msix_exclusive_bar, name, 4096); > + > + free(name); > + > + ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096); > + if (ret) { > + memory_region_destroy(&dev->msix_exclusive_bar); > + return ret; > + } > + > + pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY, > + &dev->msix_exclusive_bar); > + > + return 0; > +} > + > static void msix_free_irq_entries(PCIDevice *dev) > { > int vector; > @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar) > return 0; > } > > +void msix_uninit_exclusive_bar(PCIDevice *dev) > +{ > + if (msix_present(dev)) { > + msix_uninit(dev, &dev->msix_exclusive_bar); > + memory_region_destroy(&dev->msix_exclusive_bar); > + } > +} > + > void msix_save(PCIDevice *dev, QEMUFile *f) > { > unsigned n = dev->msix_entries_nr; > diff --git a/hw/msix.h b/hw/msix.h > index e5a488d..bed6bfb 100644 > --- a/hw/msix.h > +++ b/hw/msix.h > @@ -7,11 +7,14 @@ > int msix_init(PCIDevice *pdev, unsigned short nentries, > MemoryRegion *bar, > unsigned bar_nr, unsigned bar_size); > +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > + uint8_t bar_nr); > > void msix_write_config(PCIDevice *pci_dev, uint32_t address, > uint32_t val, int len); > > int msix_uninit(PCIDevice *d, MemoryRegion *bar); > +void msix_uninit_exclusive_bar(PCIDevice *dev); > > unsigned int msix_nr_vectors_allocated(const PCIDevice *dev); > > diff --git a/hw/pci.h b/hw/pci.h > index 4c96268..d517a54 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -226,6 +226,8 @@ struct PCIDevice { > > /* Space to store MSIX table */ > uint8_t *msix_table_page; > + /* MemoryRegion container for msix exclusive BAR setup */ > + MemoryRegion msix_exclusive_bar; > /* MMIO index used to map MSIX table and pending bit entries. */ > MemoryRegion msix_mmio; > /* Reference-count for entries actually in use by driver. */
On Thu, 2012-06-14 at 13:29 +0300, Michael S. Tsirkin wrote: > On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote: > > msi_init() takes over a BAR without really specifying or allowing > > specification of how it does so. Instead, let's split it into > > two interfaces, one fully specified, and one trivially easy. This > > implements the latter. msix_init_exclusive_bar() takes over > > allocating and filling a PCI BAR _exclusively_ for the use of MSIX. > > When used, the matching msi_uninit_exclusive_bar() should be used > > to tear it down. > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > > > hw/msix.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > > hw/msix.h | 3 +++ > > hw/pci.h | 2 ++ > > 3 files changed, 48 insertions(+) > > > > diff --git a/hw/msix.c b/hw/msix.c > > index b64f109..a4cdfb0 100644 > > --- a/hw/msix.c > > +++ b/hw/msix.c > > @@ -299,6 +299,41 @@ err_config: > > return ret; > > } > > > > +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > > + uint8_t bar_nr) > > +{ > > + int ret; > > + char *name; > > + > > + /* > > + * Migration compatibility dictates that this remains a 4k > > + * BAR with the vector table in the lower half and PBA in > > + * the upper half. > > + */ > > + if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) { > > + return -EINVAL; > > + } > > + > > + if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) { > > + return -ENOMEM; > > + } > > + > > I think we should avoid spaces in names. > Also let's use lower case. And BAR is confusing IMO: > it's not a base address register, it's the region. It's a region backing a base address register. > So I would prefer simply: "%s-msix". Ok, fixed. Thanks, Alex > > + memory_region_init(&dev->msix_exclusive_bar, name, 4096); > > + > > + free(name); > > + > > + ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096); > > + if (ret) { > > + memory_region_destroy(&dev->msix_exclusive_bar); > > + return ret; > > + } > > + > > + pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY, > > + &dev->msix_exclusive_bar); > > + > > + return 0; > > +} > > + > > static void msix_free_irq_entries(PCIDevice *dev) > > { > > int vector; > > @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar) > > return 0; > > } > > > > +void msix_uninit_exclusive_bar(PCIDevice *dev) > > +{ > > + if (msix_present(dev)) { > > + msix_uninit(dev, &dev->msix_exclusive_bar); > > + memory_region_destroy(&dev->msix_exclusive_bar); > > + } > > +} > > + > > void msix_save(PCIDevice *dev, QEMUFile *f) > > { > > unsigned n = dev->msix_entries_nr; > > diff --git a/hw/msix.h b/hw/msix.h > > index e5a488d..bed6bfb 100644 > > --- a/hw/msix.h > > +++ b/hw/msix.h > > @@ -7,11 +7,14 @@ > > int msix_init(PCIDevice *pdev, unsigned short nentries, > > MemoryRegion *bar, > > unsigned bar_nr, unsigned bar_size); > > +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > > + uint8_t bar_nr); > > > > void msix_write_config(PCIDevice *pci_dev, uint32_t address, > > uint32_t val, int len); > > > > int msix_uninit(PCIDevice *d, MemoryRegion *bar); > > +void msix_uninit_exclusive_bar(PCIDevice *dev); > > > > unsigned int msix_nr_vectors_allocated(const PCIDevice *dev); > > > > diff --git a/hw/pci.h b/hw/pci.h > > index 4c96268..d517a54 100644 > > --- a/hw/pci.h > > +++ b/hw/pci.h > > @@ -226,6 +226,8 @@ struct PCIDevice { > > > > /* Space to store MSIX table */ > > uint8_t *msix_table_page; > > + /* MemoryRegion container for msix exclusive BAR setup */ > > + MemoryRegion msix_exclusive_bar; > > /* MMIO index used to map MSIX table and pending bit entries. */ > > MemoryRegion msix_mmio; > > /* Reference-count for entries actually in use by driver. */ >
On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote: > msi_init() takes over a BAR without really specifying or allowing > specification of how it does so. Instead, let's split it into > two interfaces, one fully specified, and one trivially easy. This > implements the latter. msix_init_exclusive_bar() takes over > allocating and filling a PCI BAR _exclusively_ for the use of MSIX. > When used, the matching msi_uninit_exclusive_bar() should be used > to tear it down. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > > hw/msix.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > hw/msix.h | 3 +++ > hw/pci.h | 2 ++ > 3 files changed, 48 insertions(+) > > diff --git a/hw/msix.c b/hw/msix.c > index b64f109..a4cdfb0 100644 > --- a/hw/msix.c > +++ b/hw/msix.c > @@ -299,6 +299,41 @@ err_config: > return ret; > } > > +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > + uint8_t bar_nr) > +{ > + int ret; > + char *name; > + > + /* > + * Migration compatibility dictates that this remains a 4k > + * BAR with the vector table in the lower half and PBA in > + * the upper half. > + */ > + if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) { > + return -EINVAL; > + } > + > + if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) { > + return -ENOMEM; > + } > + > + memory_region_init(&dev->msix_exclusive_bar, name, 4096); > + > + free(name); > + > + ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096); > + if (ret) { > + memory_region_destroy(&dev->msix_exclusive_bar); > + return ret; > + } > + > + pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY, > + &dev->msix_exclusive_bar); > + > + return 0; > +} > + > static void msix_free_irq_entries(PCIDevice *dev) > { > int vector; > @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar) > return 0; > } > > +void msix_uninit_exclusive_bar(PCIDevice *dev) > +{ > + if (msix_present(dev)) { > + msix_uninit(dev, &dev->msix_exclusive_bar); > + memory_region_destroy(&dev->msix_exclusive_bar); > + } > +} > + > void msix_save(PCIDevice *dev, QEMUFile *f) > { > unsigned n = dev->msix_entries_nr; > diff --git a/hw/msix.h b/hw/msix.h > index e5a488d..bed6bfb 100644 > --- a/hw/msix.h > +++ b/hw/msix.h > @@ -7,11 +7,14 @@ > int msix_init(PCIDevice *pdev, unsigned short nentries, > MemoryRegion *bar, > unsigned bar_nr, unsigned bar_size); > +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > + uint8_t bar_nr); > > void msix_write_config(PCIDevice *pci_dev, uint32_t address, > uint32_t val, int len); > > int msix_uninit(PCIDevice *d, MemoryRegion *bar); > +void msix_uninit_exclusive_bar(PCIDevice *dev); > > unsigned int msix_nr_vectors_allocated(const PCIDevice *dev); > > diff --git a/hw/pci.h b/hw/pci.h > index 4c96268..d517a54 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -226,6 +226,8 @@ struct PCIDevice { > > /* Space to store MSIX table */ > uint8_t *msix_table_page; > + /* MemoryRegion container for msix exclusive BAR setup */ > + MemoryRegion msix_exclusive_bar; Wrappers are good but this doesn't belong in PCIDevice IMO. E.g. when I debug and look at data structure I don't want to dig into code and go "oh this device uses an exclusive bar to it's here, that one uses a non exlcusive so the field is invalid, the real bar is over there. Keep the region in devices where it was, pass to msix_init_exclusive_bar by parameter. This way these are just helpers. > /* MMIO index used to map MSIX table and pending bit entries. */ > MemoryRegion msix_mmio; > /* Reference-count for entries actually in use by driver. */
On 2012-06-14 17:49, Michael S. Tsirkin wrote: > On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote: >> msi_init() takes over a BAR without really specifying or allowing >> specification of how it does so. Instead, let's split it into >> two interfaces, one fully specified, and one trivially easy. This >> implements the latter. msix_init_exclusive_bar() takes over >> allocating and filling a PCI BAR _exclusively_ for the use of MSIX. >> When used, the matching msi_uninit_exclusive_bar() should be used >> to tear it down. >> >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> >> --- >> >> hw/msix.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >> hw/msix.h | 3 +++ >> hw/pci.h | 2 ++ >> 3 files changed, 48 insertions(+) >> >> diff --git a/hw/msix.c b/hw/msix.c >> index b64f109..a4cdfb0 100644 >> --- a/hw/msix.c >> +++ b/hw/msix.c >> @@ -299,6 +299,41 @@ err_config: >> return ret; >> } >> >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, >> + uint8_t bar_nr) >> +{ >> + int ret; >> + char *name; >> + >> + /* >> + * Migration compatibility dictates that this remains a 4k >> + * BAR with the vector table in the lower half and PBA in >> + * the upper half. >> + */ >> + if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) { >> + return -EINVAL; >> + } >> + >> + if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) { >> + return -ENOMEM; >> + } >> + >> + memory_region_init(&dev->msix_exclusive_bar, name, 4096); >> + >> + free(name); >> + >> + ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096); >> + if (ret) { >> + memory_region_destroy(&dev->msix_exclusive_bar); >> + return ret; >> + } >> + >> + pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY, >> + &dev->msix_exclusive_bar); >> + >> + return 0; >> +} >> + >> static void msix_free_irq_entries(PCIDevice *dev) >> { >> int vector; >> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar) >> return 0; >> } >> >> +void msix_uninit_exclusive_bar(PCIDevice *dev) >> +{ >> + if (msix_present(dev)) { >> + msix_uninit(dev, &dev->msix_exclusive_bar); >> + memory_region_destroy(&dev->msix_exclusive_bar); >> + } >> +} >> + >> void msix_save(PCIDevice *dev, QEMUFile *f) >> { >> unsigned n = dev->msix_entries_nr; >> diff --git a/hw/msix.h b/hw/msix.h >> index e5a488d..bed6bfb 100644 >> --- a/hw/msix.h >> +++ b/hw/msix.h >> @@ -7,11 +7,14 @@ >> int msix_init(PCIDevice *pdev, unsigned short nentries, >> MemoryRegion *bar, >> unsigned bar_nr, unsigned bar_size); >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, >> + uint8_t bar_nr); >> >> void msix_write_config(PCIDevice *pci_dev, uint32_t address, >> uint32_t val, int len); >> >> int msix_uninit(PCIDevice *d, MemoryRegion *bar); >> +void msix_uninit_exclusive_bar(PCIDevice *dev); >> >> unsigned int msix_nr_vectors_allocated(const PCIDevice *dev); >> >> diff --git a/hw/pci.h b/hw/pci.h >> index 4c96268..d517a54 100644 >> --- a/hw/pci.h >> +++ b/hw/pci.h >> @@ -226,6 +226,8 @@ struct PCIDevice { >> >> /* Space to store MSIX table */ >> uint8_t *msix_table_page; >> + /* MemoryRegion container for msix exclusive BAR setup */ >> + MemoryRegion msix_exclusive_bar; > > Wrappers are good but this doesn't belong in PCIDevice IMO. > E.g. when I debug and look at data structure I don't want to dig into > code and go "oh this device uses an exclusive bar to it's here, that one > uses a non exlcusive so the field is invalid, the real bar is over > there. > > Keep the region in devices where it was, pass to msix_init_exclusive_bar > by parameter. I disagree. There are plenty of fields in PCIDevice that are only used under certain circumstances. This is just +1, and it makes the interface usage much easier. Jan
On Thu, Jun 14, 2012 at 05:55:18PM +0200, Jan Kiszka wrote: > On 2012-06-14 17:49, Michael S. Tsirkin wrote: > > On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote: > >> msi_init() takes over a BAR without really specifying or allowing > >> specification of how it does so. Instead, let's split it into > >> two interfaces, one fully specified, and one trivially easy. This > >> implements the latter. msix_init_exclusive_bar() takes over > >> allocating and filling a PCI BAR _exclusively_ for the use of MSIX. > >> When used, the matching msi_uninit_exclusive_bar() should be used > >> to tear it down. > >> > >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > >> --- > >> > >> hw/msix.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > >> hw/msix.h | 3 +++ > >> hw/pci.h | 2 ++ > >> 3 files changed, 48 insertions(+) > >> > >> diff --git a/hw/msix.c b/hw/msix.c > >> index b64f109..a4cdfb0 100644 > >> --- a/hw/msix.c > >> +++ b/hw/msix.c > >> @@ -299,6 +299,41 @@ err_config: > >> return ret; > >> } > >> > >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > >> + uint8_t bar_nr) > >> +{ > >> + int ret; > >> + char *name; > >> + > >> + /* > >> + * Migration compatibility dictates that this remains a 4k > >> + * BAR with the vector table in the lower half and PBA in > >> + * the upper half. > >> + */ > >> + if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) { > >> + return -EINVAL; > >> + } > >> + > >> + if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) { > >> + return -ENOMEM; > >> + } > >> + > >> + memory_region_init(&dev->msix_exclusive_bar, name, 4096); > >> + > >> + free(name); > >> + > >> + ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096); > >> + if (ret) { > >> + memory_region_destroy(&dev->msix_exclusive_bar); > >> + return ret; > >> + } > >> + > >> + pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY, > >> + &dev->msix_exclusive_bar); > >> + > >> + return 0; > >> +} > >> + > >> static void msix_free_irq_entries(PCIDevice *dev) > >> { > >> int vector; > >> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar) > >> return 0; > >> } > >> > >> +void msix_uninit_exclusive_bar(PCIDevice *dev) > >> +{ > >> + if (msix_present(dev)) { > >> + msix_uninit(dev, &dev->msix_exclusive_bar); > >> + memory_region_destroy(&dev->msix_exclusive_bar); > >> + } > >> +} > >> + > >> void msix_save(PCIDevice *dev, QEMUFile *f) > >> { > >> unsigned n = dev->msix_entries_nr; > >> diff --git a/hw/msix.h b/hw/msix.h > >> index e5a488d..bed6bfb 100644 > >> --- a/hw/msix.h > >> +++ b/hw/msix.h > >> @@ -7,11 +7,14 @@ > >> int msix_init(PCIDevice *pdev, unsigned short nentries, > >> MemoryRegion *bar, > >> unsigned bar_nr, unsigned bar_size); > >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > >> + uint8_t bar_nr); > >> > >> void msix_write_config(PCIDevice *pci_dev, uint32_t address, > >> uint32_t val, int len); > >> > >> int msix_uninit(PCIDevice *d, MemoryRegion *bar); > >> +void msix_uninit_exclusive_bar(PCIDevice *dev); > >> > >> unsigned int msix_nr_vectors_allocated(const PCIDevice *dev); > >> > >> diff --git a/hw/pci.h b/hw/pci.h > >> index 4c96268..d517a54 100644 > >> --- a/hw/pci.h > >> +++ b/hw/pci.h > >> @@ -226,6 +226,8 @@ struct PCIDevice { > >> > >> /* Space to store MSIX table */ > >> uint8_t *msix_table_page; > >> + /* MemoryRegion container for msix exclusive BAR setup */ > >> + MemoryRegion msix_exclusive_bar; > > > > Wrappers are good but this doesn't belong in PCIDevice IMO. > > E.g. when I debug and look at data structure I don't want to dig into > > code and go "oh this device uses an exclusive bar to it's here, that one > > uses a non exlcusive so the field is invalid, the real bar is over > > there. > > > > Keep the region in devices where it was, pass to msix_init_exclusive_bar > > by parameter. > > I disagree. There are plenty of fields in PCIDevice that are only used > under certain circumstances. This is just +1, and it makes the interface > usage much easier. > > Jan *Much* easier? It's one parameter less. > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux
On Thu, 2012-06-14 at 19:05 +0300, Michael S. Tsirkin wrote: > On Thu, Jun 14, 2012 at 05:55:18PM +0200, Jan Kiszka wrote: > > On 2012-06-14 17:49, Michael S. Tsirkin wrote: > > > On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote: > > >> msi_init() takes over a BAR without really specifying or allowing > > >> specification of how it does so. Instead, let's split it into > > >> two interfaces, one fully specified, and one trivially easy. This > > >> implements the latter. msix_init_exclusive_bar() takes over > > >> allocating and filling a PCI BAR _exclusively_ for the use of MSIX. > > >> When used, the matching msi_uninit_exclusive_bar() should be used > > >> to tear it down. > > >> > > >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > >> --- > > >> > > >> hw/msix.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > > >> hw/msix.h | 3 +++ > > >> hw/pci.h | 2 ++ > > >> 3 files changed, 48 insertions(+) > > >> > > >> diff --git a/hw/msix.c b/hw/msix.c > > >> index b64f109..a4cdfb0 100644 > > >> --- a/hw/msix.c > > >> +++ b/hw/msix.c > > >> @@ -299,6 +299,41 @@ err_config: > > >> return ret; > > >> } > > >> > > >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > > >> + uint8_t bar_nr) > > >> +{ > > >> + int ret; > > >> + char *name; > > >> + > > >> + /* > > >> + * Migration compatibility dictates that this remains a 4k > > >> + * BAR with the vector table in the lower half and PBA in > > >> + * the upper half. > > >> + */ > > >> + if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) { > > >> + return -EINVAL; > > >> + } > > >> + > > >> + if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) { > > >> + return -ENOMEM; > > >> + } > > >> + > > >> + memory_region_init(&dev->msix_exclusive_bar, name, 4096); > > >> + > > >> + free(name); > > >> + > > >> + ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096); > > >> + if (ret) { > > >> + memory_region_destroy(&dev->msix_exclusive_bar); > > >> + return ret; > > >> + } > > >> + > > >> + pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY, > > >> + &dev->msix_exclusive_bar); > > >> + > > >> + return 0; > > >> +} > > >> + > > >> static void msix_free_irq_entries(PCIDevice *dev) > > >> { > > >> int vector; > > >> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar) > > >> return 0; > > >> } > > >> > > >> +void msix_uninit_exclusive_bar(PCIDevice *dev) > > >> +{ > > >> + if (msix_present(dev)) { > > >> + msix_uninit(dev, &dev->msix_exclusive_bar); > > >> + memory_region_destroy(&dev->msix_exclusive_bar); > > >> + } > > >> +} > > >> + > > >> void msix_save(PCIDevice *dev, QEMUFile *f) > > >> { > > >> unsigned n = dev->msix_entries_nr; > > >> diff --git a/hw/msix.h b/hw/msix.h > > >> index e5a488d..bed6bfb 100644 > > >> --- a/hw/msix.h > > >> +++ b/hw/msix.h > > >> @@ -7,11 +7,14 @@ > > >> int msix_init(PCIDevice *pdev, unsigned short nentries, > > >> MemoryRegion *bar, > > >> unsigned bar_nr, unsigned bar_size); > > >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > > >> + uint8_t bar_nr); > > >> > > >> void msix_write_config(PCIDevice *pci_dev, uint32_t address, > > >> uint32_t val, int len); > > >> > > >> int msix_uninit(PCIDevice *d, MemoryRegion *bar); > > >> +void msix_uninit_exclusive_bar(PCIDevice *dev); > > >> > > >> unsigned int msix_nr_vectors_allocated(const PCIDevice *dev); > > >> > > >> diff --git a/hw/pci.h b/hw/pci.h > > >> index 4c96268..d517a54 100644 > > >> --- a/hw/pci.h > > >> +++ b/hw/pci.h > > >> @@ -226,6 +226,8 @@ struct PCIDevice { > > >> > > >> /* Space to store MSIX table */ > > >> uint8_t *msix_table_page; > > >> + /* MemoryRegion container for msix exclusive BAR setup */ > > >> + MemoryRegion msix_exclusive_bar; > > > > > > Wrappers are good but this doesn't belong in PCIDevice IMO. > > > E.g. when I debug and look at data structure I don't want to dig into > > > code and go "oh this device uses an exclusive bar to it's here, that one > > > uses a non exlcusive so the field is invalid, the real bar is over > > > there. > > > > > > Keep the region in devices where it was, pass to msix_init_exclusive_bar > > > by parameter. > > > > I disagree. There are plenty of fields in PCIDevice that are only used > > under certain circumstances. This is just +1, and it makes the interface > > usage much easier. > > > > Jan > > *Much* easier? It's one parameter less. I've been asked to implement pci_unregister_bar for two less parameters...
On 2012-06-14 18:05, Michael S. Tsirkin wrote: > On Thu, Jun 14, 2012 at 05:55:18PM +0200, Jan Kiszka wrote: >> On 2012-06-14 17:49, Michael S. Tsirkin wrote: >>> On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote: >>>> msi_init() takes over a BAR without really specifying or allowing >>>> specification of how it does so. Instead, let's split it into >>>> two interfaces, one fully specified, and one trivially easy. This >>>> implements the latter. msix_init_exclusive_bar() takes over >>>> allocating and filling a PCI BAR _exclusively_ for the use of MSIX. >>>> When used, the matching msi_uninit_exclusive_bar() should be used >>>> to tear it down. >>>> >>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> >>>> --- >>>> >>>> hw/msix.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >>>> hw/msix.h | 3 +++ >>>> hw/pci.h | 2 ++ >>>> 3 files changed, 48 insertions(+) >>>> >>>> diff --git a/hw/msix.c b/hw/msix.c >>>> index b64f109..a4cdfb0 100644 >>>> --- a/hw/msix.c >>>> +++ b/hw/msix.c >>>> @@ -299,6 +299,41 @@ err_config: >>>> return ret; >>>> } >>>> >>>> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, >>>> + uint8_t bar_nr) >>>> +{ >>>> + int ret; >>>> + char *name; >>>> + >>>> + /* >>>> + * Migration compatibility dictates that this remains a 4k >>>> + * BAR with the vector table in the lower half and PBA in >>>> + * the upper half. >>>> + */ >>>> + if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) { >>>> + return -EINVAL; >>>> + } >>>> + >>>> + if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) { >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + memory_region_init(&dev->msix_exclusive_bar, name, 4096); >>>> + >>>> + free(name); >>>> + >>>> + ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096); >>>> + if (ret) { >>>> + memory_region_destroy(&dev->msix_exclusive_bar); >>>> + return ret; >>>> + } >>>> + >>>> + pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY, >>>> + &dev->msix_exclusive_bar); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static void msix_free_irq_entries(PCIDevice *dev) >>>> { >>>> int vector; >>>> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar) >>>> return 0; >>>> } >>>> >>>> +void msix_uninit_exclusive_bar(PCIDevice *dev) >>>> +{ >>>> + if (msix_present(dev)) { >>>> + msix_uninit(dev, &dev->msix_exclusive_bar); >>>> + memory_region_destroy(&dev->msix_exclusive_bar); >>>> + } >>>> +} >>>> + >>>> void msix_save(PCIDevice *dev, QEMUFile *f) >>>> { >>>> unsigned n = dev->msix_entries_nr; >>>> diff --git a/hw/msix.h b/hw/msix.h >>>> index e5a488d..bed6bfb 100644 >>>> --- a/hw/msix.h >>>> +++ b/hw/msix.h >>>> @@ -7,11 +7,14 @@ >>>> int msix_init(PCIDevice *pdev, unsigned short nentries, >>>> MemoryRegion *bar, >>>> unsigned bar_nr, unsigned bar_size); >>>> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, >>>> + uint8_t bar_nr); >>>> >>>> void msix_write_config(PCIDevice *pci_dev, uint32_t address, >>>> uint32_t val, int len); >>>> >>>> int msix_uninit(PCIDevice *d, MemoryRegion *bar); >>>> +void msix_uninit_exclusive_bar(PCIDevice *dev); >>>> >>>> unsigned int msix_nr_vectors_allocated(const PCIDevice *dev); >>>> >>>> diff --git a/hw/pci.h b/hw/pci.h >>>> index 4c96268..d517a54 100644 >>>> --- a/hw/pci.h >>>> +++ b/hw/pci.h >>>> @@ -226,6 +226,8 @@ struct PCIDevice { >>>> >>>> /* Space to store MSIX table */ >>>> uint8_t *msix_table_page; >>>> + /* MemoryRegion container for msix exclusive BAR setup */ >>>> + MemoryRegion msix_exclusive_bar; >>> >>> Wrappers are good but this doesn't belong in PCIDevice IMO. >>> E.g. when I debug and look at data structure I don't want to dig into >>> code and go "oh this device uses an exclusive bar to it's here, that one >>> uses a non exlcusive so the field is invalid, the real bar is over >>> there. >>> >>> Keep the region in devices where it was, pass to msix_init_exclusive_bar >>> by parameter. >> >> I disagree. There are plenty of fields in PCIDevice that are only used >> under certain circumstances. This is just +1, and it makes the interface >> usage much easier. >> >> Jan > > *Much* easier? It's one parameter less. ...and one field to add to every user of msix_init_exclusive_bar while those users have no business with it. The container is a pure internal field, so it belongs to the infrastructure. Jan
On Thu, Jun 14, 2012 at 10:10:40AM -0600, Alex Williamson wrote: > On Thu, 2012-06-14 at 19:05 +0300, Michael S. Tsirkin wrote: > > On Thu, Jun 14, 2012 at 05:55:18PM +0200, Jan Kiszka wrote: > > > On 2012-06-14 17:49, Michael S. Tsirkin wrote: > > > > On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote: > > > >> msi_init() takes over a BAR without really specifying or allowing > > > >> specification of how it does so. Instead, let's split it into > > > >> two interfaces, one fully specified, and one trivially easy. This > > > >> implements the latter. msix_init_exclusive_bar() takes over > > > >> allocating and filling a PCI BAR _exclusively_ for the use of MSIX. > > > >> When used, the matching msi_uninit_exclusive_bar() should be used > > > >> to tear it down. > > > >> > > > >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > >> --- > > > >> > > > >> hw/msix.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > > > >> hw/msix.h | 3 +++ > > > >> hw/pci.h | 2 ++ > > > >> 3 files changed, 48 insertions(+) > > > >> > > > >> diff --git a/hw/msix.c b/hw/msix.c > > > >> index b64f109..a4cdfb0 100644 > > > >> --- a/hw/msix.c > > > >> +++ b/hw/msix.c > > > >> @@ -299,6 +299,41 @@ err_config: > > > >> return ret; > > > >> } > > > >> > > > >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > > > >> + uint8_t bar_nr) > > > >> +{ > > > >> + int ret; > > > >> + char *name; > > > >> + > > > >> + /* > > > >> + * Migration compatibility dictates that this remains a 4k > > > >> + * BAR with the vector table in the lower half and PBA in > > > >> + * the upper half. > > > >> + */ > > > >> + if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) { > > > >> + return -EINVAL; > > > >> + } > > > >> + > > > >> + if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) { > > > >> + return -ENOMEM; > > > >> + } > > > >> + > > > >> + memory_region_init(&dev->msix_exclusive_bar, name, 4096); > > > >> + > > > >> + free(name); > > > >> + > > > >> + ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096); > > > >> + if (ret) { > > > >> + memory_region_destroy(&dev->msix_exclusive_bar); > > > >> + return ret; > > > >> + } > > > >> + > > > >> + pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY, > > > >> + &dev->msix_exclusive_bar); > > > >> + > > > >> + return 0; > > > >> +} > > > >> + > > > >> static void msix_free_irq_entries(PCIDevice *dev) > > > >> { > > > >> int vector; > > > >> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar) > > > >> return 0; > > > >> } > > > >> > > > >> +void msix_uninit_exclusive_bar(PCIDevice *dev) > > > >> +{ > > > >> + if (msix_present(dev)) { > > > >> + msix_uninit(dev, &dev->msix_exclusive_bar); > > > >> + memory_region_destroy(&dev->msix_exclusive_bar); > > > >> + } > > > >> +} > > > >> + > > > >> void msix_save(PCIDevice *dev, QEMUFile *f) > > > >> { > > > >> unsigned n = dev->msix_entries_nr; > > > >> diff --git a/hw/msix.h b/hw/msix.h > > > >> index e5a488d..bed6bfb 100644 > > > >> --- a/hw/msix.h > > > >> +++ b/hw/msix.h > > > >> @@ -7,11 +7,14 @@ > > > >> int msix_init(PCIDevice *pdev, unsigned short nentries, > > > >> MemoryRegion *bar, > > > >> unsigned bar_nr, unsigned bar_size); > > > >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > > > >> + uint8_t bar_nr); > > > >> > > > >> void msix_write_config(PCIDevice *pci_dev, uint32_t address, > > > >> uint32_t val, int len); > > > >> > > > >> int msix_uninit(PCIDevice *d, MemoryRegion *bar); > > > >> +void msix_uninit_exclusive_bar(PCIDevice *dev); > > > >> > > > >> unsigned int msix_nr_vectors_allocated(const PCIDevice *dev); > > > >> > > > >> diff --git a/hw/pci.h b/hw/pci.h > > > >> index 4c96268..d517a54 100644 > > > >> --- a/hw/pci.h > > > >> +++ b/hw/pci.h > > > >> @@ -226,6 +226,8 @@ struct PCIDevice { > > > >> > > > >> /* Space to store MSIX table */ > > > >> uint8_t *msix_table_page; > > > >> + /* MemoryRegion container for msix exclusive BAR setup */ > > > >> + MemoryRegion msix_exclusive_bar; > > > > > > > > Wrappers are good but this doesn't belong in PCIDevice IMO. > > > > E.g. when I debug and look at data structure I don't want to dig into > > > > code and go "oh this device uses an exclusive bar to it's here, that one > > > > uses a non exlcusive so the field is invalid, the real bar is over > > > > there. > > > > > > > > Keep the region in devices where it was, pass to msix_init_exclusive_bar > > > > by parameter. > > > > > > I disagree. There are plenty of fields in PCIDevice that are only used > > > under certain circumstances. This is just +1, and it makes the interface > > > usage much easier. > > > > > > Jan > > > > *Much* easier? It's one parameter less. > > I've been asked to implement pci_unregister_bar for two less > parameters... Yes but for a function that has too many already, and you were adding more, to limit the damage I suggested this. More than this, many parameters have same type so if you get confused compiler won't help you. If you find a way to not add parameters to msix_init I will not ask you to reduce it's numer of parameters. This one has 1 or two, not a big deal at all.
On Thu, Jun 14, 2012 at 06:11:18PM +0200, Jan Kiszka wrote: > On 2012-06-14 18:05, Michael S. Tsirkin wrote: > > On Thu, Jun 14, 2012 at 05:55:18PM +0200, Jan Kiszka wrote: > >> On 2012-06-14 17:49, Michael S. Tsirkin wrote: > >>> On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote: > >>>> msi_init() takes over a BAR without really specifying or allowing > >>>> specification of how it does so. Instead, let's split it into > >>>> two interfaces, one fully specified, and one trivially easy. This > >>>> implements the latter. msix_init_exclusive_bar() takes over > >>>> allocating and filling a PCI BAR _exclusively_ for the use of MSIX. > >>>> When used, the matching msi_uninit_exclusive_bar() should be used > >>>> to tear it down. > >>>> > >>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > >>>> --- > >>>> > >>>> hw/msix.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > >>>> hw/msix.h | 3 +++ > >>>> hw/pci.h | 2 ++ > >>>> 3 files changed, 48 insertions(+) > >>>> > >>>> diff --git a/hw/msix.c b/hw/msix.c > >>>> index b64f109..a4cdfb0 100644 > >>>> --- a/hw/msix.c > >>>> +++ b/hw/msix.c > >>>> @@ -299,6 +299,41 @@ err_config: > >>>> return ret; > >>>> } > >>>> > >>>> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > >>>> + uint8_t bar_nr) > >>>> +{ > >>>> + int ret; > >>>> + char *name; > >>>> + > >>>> + /* > >>>> + * Migration compatibility dictates that this remains a 4k > >>>> + * BAR with the vector table in the lower half and PBA in > >>>> + * the upper half. > >>>> + */ > >>>> + if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) { > >>>> + return -EINVAL; > >>>> + } > >>>> + > >>>> + if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) { > >>>> + return -ENOMEM; > >>>> + } > >>>> + > >>>> + memory_region_init(&dev->msix_exclusive_bar, name, 4096); > >>>> + > >>>> + free(name); > >>>> + > >>>> + ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096); > >>>> + if (ret) { > >>>> + memory_region_destroy(&dev->msix_exclusive_bar); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY, > >>>> + &dev->msix_exclusive_bar); > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> static void msix_free_irq_entries(PCIDevice *dev) > >>>> { > >>>> int vector; > >>>> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar) > >>>> return 0; > >>>> } > >>>> > >>>> +void msix_uninit_exclusive_bar(PCIDevice *dev) > >>>> +{ > >>>> + if (msix_present(dev)) { > >>>> + msix_uninit(dev, &dev->msix_exclusive_bar); > >>>> + memory_region_destroy(&dev->msix_exclusive_bar); > >>>> + } > >>>> +} > >>>> + > >>>> void msix_save(PCIDevice *dev, QEMUFile *f) > >>>> { > >>>> unsigned n = dev->msix_entries_nr; > >>>> diff --git a/hw/msix.h b/hw/msix.h > >>>> index e5a488d..bed6bfb 100644 > >>>> --- a/hw/msix.h > >>>> +++ b/hw/msix.h > >>>> @@ -7,11 +7,14 @@ > >>>> int msix_init(PCIDevice *pdev, unsigned short nentries, > >>>> MemoryRegion *bar, > >>>> unsigned bar_nr, unsigned bar_size); > >>>> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > >>>> + uint8_t bar_nr); > >>>> > >>>> void msix_write_config(PCIDevice *pci_dev, uint32_t address, > >>>> uint32_t val, int len); > >>>> > >>>> int msix_uninit(PCIDevice *d, MemoryRegion *bar); > >>>> +void msix_uninit_exclusive_bar(PCIDevice *dev); > >>>> > >>>> unsigned int msix_nr_vectors_allocated(const PCIDevice *dev); > >>>> > >>>> diff --git a/hw/pci.h b/hw/pci.h > >>>> index 4c96268..d517a54 100644 > >>>> --- a/hw/pci.h > >>>> +++ b/hw/pci.h > >>>> @@ -226,6 +226,8 @@ struct PCIDevice { > >>>> > >>>> /* Space to store MSIX table */ > >>>> uint8_t *msix_table_page; > >>>> + /* MemoryRegion container for msix exclusive BAR setup */ > >>>> + MemoryRegion msix_exclusive_bar; > >>> > >>> Wrappers are good but this doesn't belong in PCIDevice IMO. > >>> E.g. when I debug and look at data structure I don't want to dig into > >>> code and go "oh this device uses an exclusive bar to it's here, that one > >>> uses a non exlcusive so the field is invalid, the real bar is over > >>> there. > >>> > >>> Keep the region in devices where it was, pass to msix_init_exclusive_bar > >>> by parameter. > >> > >> I disagree. There are plenty of fields in PCIDevice that are only used > >> under certain circumstances. This is just +1, and it makes the interface > >> usage much easier. > >> > >> Jan > > > > *Much* easier? It's one parameter less. > > ...and one field to add to every user of msix_init_exclusive_bar while > those users have no business with it. The container is a pure internal > field, so it belongs to the infrastructure. > > Jan True. I wish I had a better idea. Will ponder this a bit. > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux
diff --git a/hw/msix.c b/hw/msix.c index b64f109..a4cdfb0 100644 --- a/hw/msix.c +++ b/hw/msix.c @@ -299,6 +299,41 @@ err_config: return ret; } +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, + uint8_t bar_nr) +{ + int ret; + char *name; + + /* + * Migration compatibility dictates that this remains a 4k + * BAR with the vector table in the lower half and PBA in + * the upper half. + */ + if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) { + return -EINVAL; + } + + if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) { + return -ENOMEM; + } + + memory_region_init(&dev->msix_exclusive_bar, name, 4096); + + free(name); + + ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096); + if (ret) { + memory_region_destroy(&dev->msix_exclusive_bar); + return ret; + } + + pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY, + &dev->msix_exclusive_bar); + + return 0; +} + static void msix_free_irq_entries(PCIDevice *dev) { int vector; @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar) return 0; } +void msix_uninit_exclusive_bar(PCIDevice *dev) +{ + if (msix_present(dev)) { + msix_uninit(dev, &dev->msix_exclusive_bar); + memory_region_destroy(&dev->msix_exclusive_bar); + } +} + void msix_save(PCIDevice *dev, QEMUFile *f) { unsigned n = dev->msix_entries_nr; diff --git a/hw/msix.h b/hw/msix.h index e5a488d..bed6bfb 100644 --- a/hw/msix.h +++ b/hw/msix.h @@ -7,11 +7,14 @@ int msix_init(PCIDevice *pdev, unsigned short nentries, MemoryRegion *bar, unsigned bar_nr, unsigned bar_size); +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, + uint8_t bar_nr); void msix_write_config(PCIDevice *pci_dev, uint32_t address, uint32_t val, int len); int msix_uninit(PCIDevice *d, MemoryRegion *bar); +void msix_uninit_exclusive_bar(PCIDevice *dev); unsigned int msix_nr_vectors_allocated(const PCIDevice *dev); diff --git a/hw/pci.h b/hw/pci.h index 4c96268..d517a54 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -226,6 +226,8 @@ struct PCIDevice { /* Space to store MSIX table */ uint8_t *msix_table_page; + /* MemoryRegion container for msix exclusive BAR setup */ + MemoryRegion msix_exclusive_bar; /* MMIO index used to map MSIX table and pending bit entries. */ MemoryRegion msix_mmio; /* Reference-count for entries actually in use by driver. */
msi_init() takes over a BAR without really specifying or allowing specification of how it does so. Instead, let's split it into two interfaces, one fully specified, and one trivially easy. This implements the latter. msix_init_exclusive_bar() takes over allocating and filling a PCI BAR _exclusively_ for the use of MSIX. When used, the matching msi_uninit_exclusive_bar() should be used to tear it down. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- hw/msix.c | 43 +++++++++++++++++++++++++++++++++++++++++++ hw/msix.h | 3 +++ hw/pci.h | 2 ++ 3 files changed, 48 insertions(+)