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.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;
> > + }
> > +
>
> 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. */
>
