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

Alex Williamson
2010-11-01T15:56:08Z

On Thu, 2010-10-28 at 17:00 +0200, Avi Kivity wrote:
> On 10/23/2010 06:55 PM, Alex Williamson wrote:
> > 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,
> >
>
> Ok; applied. Please add some warning if the condition happens so the
> breakage is at least not silent.

Looks like this is already handled before we even get here. msix_init()
checks nentries against the max supported in a single page and returns
-EINVAL if over. So I think we're safely covered until we add support
for more pages. Thanks,

Alex