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.william...@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.
--
MST
