Re: [Qemu-devel] [PATCH v2 5/6] msix: Allow full specification of MSIX layout

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 2012-06-14 at 08:17 +0200, Jan Kiszka wrote:
> On 2012-06-14 06:51, Alex Williamson wrote:
>   
> > -/* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is
> > - * modified, it should be retrieved with msix_bar_size. */
> > +/* Add MSI-X capability to the config space for the device. */
> > +static int msix_add_config(struct PCIDevice *dev, unsigned short nentries,
> > +                           uint8_t table_bar, unsigned table_offset,
> > +                           uint8_t pba_bar, unsigned pba_offset, uint8_t pos)
> 
> Why not fold msix_add_config into msix_init and move sanity checks to
> the beginning, i.e. before resource allocations? Then you also do not
> need to call msix_uninit from msix_init. Likely a matter of taste, so
> not a must-have.

Yep, that works nicely.  fixed.

> > +{
> > +    int config_offset;
> > +    uint8_t *config;
> > +
> > +    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    config_offset = pci_add_capability(dev, PCI_CAP_ID_MSIX,
> > +                                       pos, MSIX_CAP_LENGTH);
> > +    if (config_offset < 0) {
> > +        return config_offset;
> > +    }
> > +
> > +    config = dev->config + config_offset;
> > +
> > +    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > +    pci_set_long(config + PCI_MSIX_TABLE, table_offset | table_bar);
> > +    pci_set_long(config + PCI_MSIX_PBA, pba_offset | pba_bar);
> > +
> > +    dev->msix_cap = config_offset;
> > +
> > +    /* Make flags bit writable. */
> > +    dev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > +                                                       MSIX_MASKALL_MASK;
> > +
> > +    dev->msix_function_masked = true;
> > +
> > +    return 0;
> > +}
> > +
> > +/* Clean up resources for the device. */
> > +void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
> > +{
> > +    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
> 
> msix_present()

Fixed.

> > +        return;
> > +    }
> > +
> > +    pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> > +    dev->msix_cap = 0;
> > +    dev->msix_entries_nr = 0;
> > +
> > +    memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
> > +    memory_region_destroy(&dev->msix_pba_mmio);
> > +    g_free(dev->msix_pba);
> > +    dev->msix_pba = NULL;
> > +
> > +    memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
> > +    memory_region_destroy(&dev->msix_table_mmio);
> > +    g_free(dev->msix_table);
> > +    dev->msix_table = NULL;
> > +
> > +    g_free(dev->msix_entry_used);
> > +    dev->msix_entry_used = NULL;
> > +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> > +}
> > +
> > +/* Initialize the MSI-X structures */
> >  int msix_init(struct PCIDevice *dev, unsigned short nentries,
> > -              MemoryRegion *bar,
> > -              unsigned bar_nr, unsigned bar_size)
> > +              MemoryRegion *table_bar, uint8_t table_bar_nr,
> > +              unsigned table_offset, MemoryRegion *pba_bar,
> > +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos)
> >  {
> >      int ret;
> >      unsigned table_size, pba_size;
> > @@ -281,43 +279,41 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> >      if (!msi_supported) {
> >          return -ENOTSUP;
> >      }
> > -    if (nentries > MSIX_MAX_ENTRIES)
> > -        return -EINVAL;
> >  
> >      table_size = nentries * PCI_MSIX_ENTRY_SIZE;
> >      pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
> >  
> > -    dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
> > -                                        sizeof *dev->msix_entry_used);
> > +    /* Sanity test: table & pba don't overlap, fit within BARs, min aligned */
> > +    if ((table_bar_nr == pba_bar_nr &&
> > +         ranges_overlap(table_offset, table_size, pba_offset, pba_size)) ||
> > +        table_offset + table_size > memory_region_size(table_bar) ||
> > +        pba_offset + pba_size > memory_region_size(pba_bar) ||
> > +        (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
> > +        return -EINVAL;
> > +    }
> >  
> >      dev->msix_table = g_malloc0(table_size);
> >      dev->msix_pba = g_malloc0(pba_size);
> > +    dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
> > +    dev->msix_entries_nr = nentries;
> > +    dev->cap_present |= QEMU_PCI_CAP_MSIX;
> > +
> >      msix_mask_all(dev, nentries);
> >  
> >      memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev,
> >                            "msix", table_size);
> > +    memory_region_add_subregion(table_bar, table_offset, &dev->msix_table_mmio);
> > +
> >      memory_region_init_io(&dev->msix_pba_mmio, &msix_pba_mmio_ops, dev,
> >                            "msix-pba", pba_size);
> > +    memory_region_add_subregion(pba_bar, pba_offset, &dev->msix_pba_mmio);
> >  
> > -    dev->msix_entries_nr = nentries;
> > -    ret = msix_add_config(dev, nentries, bar_nr, bar_size);
> > -    if (ret)
> > -        goto err_config;
> > -
> > -    dev->cap_present |= QEMU_PCI_CAP_MSIX;
> > -    msix_mmio_setup(dev, bar);
> > -    return 0;
> > +    ret = msix_add_config(dev, nentries, table_bar_nr, table_offset,
> > +                          pba_bar_nr, pba_offset, cap_pos);
> > +    if (ret) {
> > +        msix_uninit(dev, table_bar, pba_bar);
> > +    }
> >  
> > -err_config:
> > -    dev->msix_entries_nr = 0;
> > -    memory_region_destroy(&dev->msix_pba_mmio);
> > -    g_free(dev->msix_pba);
> > -    dev->msix_pba = NULL;
> > -    memory_region_destroy(&dev->msix_table_mmio);
> > -    g_free(dev->msix_table);
> > -    dev->msix_table = NULL;
> > -    g_free(dev->msix_entry_used);
> > -    dev->msix_entry_used = NULL;
> >      return ret;
> >  }
> >  
> > @@ -344,7 +340,8 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >  
> >      free(name);
> >  
> > -    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
> > +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 0,
> > +                    &dev->msix_exclusive_bar, bar_nr, 2048, 0);
> 
> Doesn't this have to be msix_table_mmi and msix_pba_mmio?

Nope, msix_exclusive_bar is the container, msix_table_mmio and
msix_pba_mmio are the subregions for their respective types.

> >      if (ret) {
> >          memory_region_destroy(&dev->msix_exclusive_bar);
> >          return ret;
> > @@ -356,6 +353,14 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >      return 0;
> >  }
> >  
> > +void msix_uninit_exclusive_bar(PCIDevice *dev)
> > +{
> > +    if (msix_present(dev)) {
> > +        msix_uninit(dev, &dev->msix_exclusive_bar, &dev->msix_exclusive_bar);
> 
> Same here.

Same as above.

> > +        memory_region_destroy(&dev->msix_exclusive_bar);
> > +    }
> > +}
> > +
> >  static void msix_free_irq_entries(PCIDevice *dev)
> >  {
> >      int vector;
> > @@ -366,38 +371,6 @@ static void msix_free_irq_entries(PCIDevice *dev)
> >      }
> >  }
> >  
> > -/* Clean up resources for the device. */
> > -int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> > -{
> > -    if (!msix_present(dev)) {
> > -        return 0;
> > -    }
> > -    pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> > -    dev->msix_cap = 0;
> > -    msix_free_irq_entries(dev);
> > -    dev->msix_entries_nr = 0;
> > -    memory_region_del_subregion(bar, &dev->msix_pba_mmio);
> > -    memory_region_destroy(&dev->msix_pba_mmio);
> > -    g_free(dev->msix_pba);
> > -    dev->msix_pba = NULL;
> > -    memory_region_del_subregion(bar, &dev->msix_table_mmio);
> > -    memory_region_destroy(&dev->msix_table_mmio);
> > -    g_free(dev->msix_table);
> > -    dev->msix_table = NULL;
> > -    g_free(dev->msix_entry_used);
> > -    dev->msix_entry_used = NULL;
> > -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> > -    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 bed6bfb..14b1a2e 100644
> > --- a/hw/msix.h
> > +++ b/hw/msix.h
> > @@ -4,16 +4,18 @@
> >  #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(PCIDevice *dev, unsigned short nentries,
> > +              MemoryRegion *table_bar, uint8_t table_bar_nr,
> > +              unsigned table_offset, MemoryRegion *pba_bar,
> > +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
> >  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(PCIDevice *dev, MemoryRegion *table_bar,
> > +                 MemoryRegion *pba_bar);
> >  void msix_uninit_exclusive_bar(PCIDevice *dev);
> >  
> >  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
> > 
> 
> Interfaces look good to me, the logic as well - except for the few remarks.

Thanks!

Alex