[Qemu-devel] Re: [PATCH 1/2] msix: Allow msix_init on a device with existing MSI-X capability

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

On Sat, 2010-10-23 at 18:18 +0200, Michael S. Tsirkin wrote:
> On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote:
> > To enable common msix support to be used with pass through devices,
> > don't attempt to change the BAR if the device already has an
> > MSI-X capability.  This also means we want to pay closer attention
> > to the size when we map the msix table page, as it isn't necessarily
> > covering the entire end of the BAR.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> >  hw/msix.c |   67 +++++++++++++++++++++++++++++++++++--------------------------
> >  1 files changed, 38 insertions(+), 29 deletions(-)
> > 
> > diff --git a/hw/msix.c b/hw/msix.c
> > index 43efbd2..4122395 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> >  {
> >      int config_offset;
> >      uint8_t *config;
> > -    uint32_t new_size;
> >  
> > -    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> > -        return -EINVAL;
> > -    if (bar_size > 0x80000000)
> > -        return -ENOSPC;
> > -
> > -    /* Add space for MSI-X structures */
> > -    if (!bar_size) {
> > -        new_size = MSIX_PAGE_SIZE;
> > -    } else if (bar_size < MSIX_PAGE_SIZE) {
> > -        bar_size = MSIX_PAGE_SIZE;
> > -        new_size = MSIX_PAGE_SIZE * 2;
> > -    } else {
> > -        new_size = bar_size * 2;
> > -    }
> > -
> > -    pdev->msix_bar_size = new_size;
> > -    config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> > -    if (config_offset < 0)
> > -        return config_offset;
> > -    config = pdev->config + config_offset;
> > -
> > -    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > -    /* Table on top of BAR */
> > -    pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > -    /* Pending bits on top of that */
> > -    pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > -                 bar_nr);
> > +    pdev->msix_bar_size = bar_size;
> > +
> > +    config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX);
> > +
> > +    if (!config_offset) {
> > +        uint32_t new_size;
> > +
> > +        if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> > +            return -EINVAL;
> > +        if (bar_size > 0x80000000)
> > +            return -ENOSPC;
> > +
> > +        /* Add space for MSI-X structures */
> > +        if (!bar_size) {
> > +            new_size = MSIX_PAGE_SIZE;
> > +        } else if (bar_size < MSIX_PAGE_SIZE) {
> > +            bar_size = MSIX_PAGE_SIZE;
> > +            new_size = MSIX_PAGE_SIZE * 2;
> > +        } else {
> > +            new_size = bar_size * 2;
> > +        }
> > +
> > +        pdev->msix_bar_size = new_size;
> > +        config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> > +                                           MSIX_CAP_LENGTH);
> > +        if (config_offset < 0)
> > +            return config_offset;
> > +        config = pdev->config + config_offset;
> > +
> > +        pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > +        /* Table on top of BAR */
> > +        pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > +        /* Pending bits on top of that */
> > +        pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > +                     bar_nr);
> > +    }
> >      pdev->msix_cap = config_offset;
> >      /* Make flags bit writeable. */
> >      pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num,
> >          return;
> >      if (size <= offset)
> >          return;
> > -    cpu_register_physical_memory(addr + offset, size - offset,
> > +    cpu_register_physical_memory(addr + offset,
> > +                                 MIN(size - offset, MSIX_PAGE_SIZE),
> 
> This is wrong I think, the table might not fit in a single page.
> You would need to read table size out of from device config.

That's true, but I was hoping to save that for later since we don't seem
to be running into that problem yet.  Current device assignment code
assumes a single page, and I haven't heard of anyone with a vector table
that exceeds that yet.  Thanks,

Alex