Re: [Qemu-devel] [PATCH v2 1/6] msix: Add simple BAR allocation MSIX setup functions

[Michael S. Tsirkin]

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 <address@hidden>
> ---
> 
>  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. */