[Qemu-devel] [PATCH 0/2] msix: couple fixes

[Qemu-devel] [PATCH 0/2] msix: couple fixes

[Alex Williamson]

I've been porting the qemu vfio driver to use the common msix
infrastructure and hit a couple issues.  The first patch addresses
the problem that a device may already have MSI-X capability setup
and non-adjustable BARs, so msix_init should be able to use the
existing capability instead of adding it's own.

I'm surprised we need the second patch, as I'm not sure how things
work today without it.  We allocate msix_irq_entries within a
KVM_CAP_IRQCHIP #ifdef.  This should be pulled in from kvm.h, if we
have CONFIG_KVM, but that's pulled in from config-target.h, which
we don't include.  We can add config.h to msix.c, but that only
works if the msix object is in Makefile.target instead of
Makefile.objs.  Feel free to fix this differently, but I think
something is wrong there.  Thanks,

Alex

---

Alex Williamson (2):
      msix: Pull in config.h for CONFIG_KVM
      msix: Allow msix_init on a device with existing MSI-X capability


 Makefile.objs   |    2 --
 Makefile.target |    1 +
 hw/msix.c       |   68 ++++++++++++++++++++++++++++++++-----------------------
 3 files changed, 40 insertions(+), 31 deletions(-)

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

[Alex Williamson]

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),
                                  d->msix_mmio_index);
 }
 

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

[Michael S. Tsirkin]

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.

>                                   d->msix_mmio_index);
>  }
>  

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

[Alex Williamson]

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

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

[Avi Kivity]

  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.

-- 
error compiling committee.c: too many arguments to function

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

[Alex Williamson]

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

[Qemu-devel] [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM

[Alex Williamson]

We need to pull in config.h or else kvm.h doesn't pull in
linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
defined.  This requires moving the object over to Makefile.target
or else we can't find config-target.h

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 Makefile.objs   |    2 --
 Makefile.target |    1 +
 hw/msix.c       |    1 +
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index ca2d2d0..c097d25 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -201,8 +201,6 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
 # PCI watchdog devices
 hw-obj-y += wdt_i6300esb.o
 
-hw-obj-y += msix.o
-
 # PCI network cards
 hw-obj-y += ne2000.o
 hw-obj-y += eepro100.o
diff --git a/Makefile.target b/Makefile.target
index 347ad6b..63da13b 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -185,6 +185,7 @@ obj-y += rwhandler.o
 obj-$(CONFIG_KVM) += kvm.o kvm-all.o
 obj-$(CONFIG_NO_KVM) += kvm-stub.o
 obj-y += memory.o
+obj-y += msix.o
 
 LIBS+=-lz
 
diff --git a/hw/msix.c b/hw/msix.c
index 4122395..23256c9 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -11,6 +11,7 @@
  * the COPYING file in the top-level directory.
  */
 
+#include "config.h"
 #include "hw.h"
 #include "msix.h"
 #include "pci.h"

[Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM

[Alex Williamson]

On Fri, 2010-10-22 at 14:40 -0600, Alex Williamson wrote:
> We need to pull in config.h or else kvm.h doesn't pull in
> linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> defined.  This requires moving the object over to Makefile.target
> or else we can't find config-target.h
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  Makefile.objs   |    2 --
>  Makefile.target |    1 +
>  hw/msix.c       |    1 +
>  3 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index ca2d2d0..c097d25 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -201,8 +201,6 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
>  # PCI watchdog devices
>  hw-obj-y += wdt_i6300esb.o
>  
> -hw-obj-y += msix.o
> -
>  # PCI network cards
>  hw-obj-y += ne2000.o
>  hw-obj-y += eepro100.o
> diff --git a/Makefile.target b/Makefile.target
> index 347ad6b..63da13b 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -185,6 +185,7 @@ obj-y += rwhandler.o
>  obj-$(CONFIG_KVM) += kvm.o kvm-all.o
>  obj-$(CONFIG_NO_KVM) += kvm-stub.o
>  obj-y += memory.o
> +obj-y += msix.o

Oops, memory.c isn't upstream, I'll push it down in my patch queue and
send a new one.

Alex

>  LIBS+=-lz
>  
> diff --git a/hw/msix.c b/hw/msix.c
> index 4122395..23256c9 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -11,6 +11,7 @@
>   * the COPYING file in the top-level directory.
>   */
>  
> +#include "config.h"
>  #include "hw.h"
>  #include "msix.h"
>  #include "pci.h"
> 

[Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM

[Paolo Bonzini]

On 10/23/2010 03:50 AM, Alex Williamson wrote:
> Oops, memory.c isn't upstream, I'll push it down in my patch queue and
> send a new one.

Neither is kvm_set_irq actually. :)  This patch is only needed for qemu-kvm.

BTW, maybe the better solution would be to move the kvm_*_irq* functions 
from qemu-kvm.c to kvm-all.c, add stubs to kvm-stub.c, and get rid of 
the #ifdef completely in msix.c

Paolo

[Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM

[Michael S. Tsirkin]

On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> We need to pull in config.h or else kvm.h doesn't pull in
> linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> defined.  This requires moving the object over to Makefile.target
> or else we can't find config-target.h
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Why? We just moved it from .target to .objs, see
889e30cc18e21f2091b77267dca8096d7dd34f8b.

> ---
> 
>  Makefile.objs   |    2 --
>  Makefile.target |    1 +
>  hw/msix.c       |    1 +
>  3 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index ca2d2d0..c097d25 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -201,8 +201,6 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
>  # PCI watchdog devices
>  hw-obj-y += wdt_i6300esb.o
>  
> -hw-obj-y += msix.o
> -
>  # PCI network cards
>  hw-obj-y += ne2000.o
>  hw-obj-y += eepro100.o
> diff --git a/Makefile.target b/Makefile.target
> index 347ad6b..63da13b 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -185,6 +185,7 @@ obj-y += rwhandler.o
>  obj-$(CONFIG_KVM) += kvm.o kvm-all.o
>  obj-$(CONFIG_NO_KVM) += kvm-stub.o
>  obj-y += memory.o
> +obj-y += msix.o
>  
>  LIBS+=-lz
>  
> diff --git a/hw/msix.c b/hw/msix.c
> index 4122395..23256c9 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -11,6 +11,7 @@
>   * the COPYING file in the top-level directory.
>   */
>  
> +#include "config.h"
>  #include "hw.h"
>  #include "msix.h"
>  #include "pci.h"

[Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM

[Alex Williamson]

On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
> On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> > We need to pull in config.h or else kvm.h doesn't pull in
> > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> > defined.  This requires moving the object over to Makefile.target
> > or else we can't find config-target.h
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Why? We just moved it from .target to .objs, see
> 889e30cc18e21f2091b77267dca8096d7dd34f8b.

Maybe that's why it used to work.  When building in the qemu-kvm.git
tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
allocated.  Then when I call msix_vector_use, I get a seg fault.
Something is broken there.  Thanks,

Alex

> > ---
> > 
> >  Makefile.objs   |    2 --
> >  Makefile.target |    1 +
> >  hw/msix.c       |    1 +
> >  3 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Makefile.objs b/Makefile.objs
> > index ca2d2d0..c097d25 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -201,8 +201,6 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
> >  # PCI watchdog devices
> >  hw-obj-y += wdt_i6300esb.o
> >  
> > -hw-obj-y += msix.o
> > -
> >  # PCI network cards
> >  hw-obj-y += ne2000.o
> >  hw-obj-y += eepro100.o
> > diff --git a/Makefile.target b/Makefile.target
> > index 347ad6b..63da13b 100644
> > --- a/Makefile.target
> > +++ b/Makefile.target
> > @@ -185,6 +185,7 @@ obj-y += rwhandler.o
> >  obj-$(CONFIG_KVM) += kvm.o kvm-all.o
> >  obj-$(CONFIG_NO_KVM) += kvm-stub.o
> >  obj-y += memory.o
> > +obj-y += msix.o
> >  
> >  LIBS+=-lz
> >  
> > diff --git a/hw/msix.c b/hw/msix.c
> > index 4122395..23256c9 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -11,6 +11,7 @@
> >   * the COPYING file in the top-level directory.
> >   */
> >  
> > +#include "config.h"
> >  #include "hw.h"
> >  #include "msix.h"
> >  #include "pci.h"

[Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM

[Michael S. Tsirkin]

On Sat, Oct 23, 2010 at 10:52:43AM -0600, Alex Williamson wrote:
> On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
> > On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> > > We need to pull in config.h or else kvm.h doesn't pull in
> > > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> > > defined.  This requires moving the object over to Makefile.target
> > > or else we can't find config-target.h
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > 
> > Why? We just moved it from .target to .objs, see
> > 889e30cc18e21f2091b77267dca8096d7dd34f8b.
> 
> Maybe that's why it used to work.  When building in the qemu-kvm.git
> tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
> KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
> allocated.  Then when I call msix_vector_use, I get a seg fault.
> Something is broken there.  Thanks,
> 
> Alex

This is hopefully fixed in the latest bits.
bd8b215bce453706c3951460cc7e6627ccb90314

-- 
MST

[Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM

[Alex Williamson]

On Sat, 2010-10-23 at 19:29 +0200, Michael S. Tsirkin wrote:
> On Sat, Oct 23, 2010 at 10:52:43AM -0600, Alex Williamson wrote:
> > On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
> > > On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> > > > We need to pull in config.h or else kvm.h doesn't pull in
> > > > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> > > > defined.  This requires moving the object over to Makefile.target
> > > > or else we can't find config-target.h
> > > > 
> > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > 
> > > Why? We just moved it from .target to .objs, see
> > > 889e30cc18e21f2091b77267dca8096d7dd34f8b.
> > 
> > Maybe that's why it used to work.  When building in the qemu-kvm.git
> > tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
> > KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
> > allocated.  Then when I call msix_vector_use, I get a seg fault.
> > Something is broken there.  Thanks,
> > 
> > Alex
> 
> This is hopefully fixed in the latest bits.
> bd8b215bce453706c3951460cc7e6627ccb90314

Nope, my tree includes that.  It's not kvm_set_irq, it's kvm_msix_add,
which dereferences msix_irq_entries, which is only allocated in
msix_init if KVM_CAP_IRQCHIP is defined, which it's not.  Maybe you also
meant to remove the ifdef from msix_init?  I also note there's another
in msix_notify.  Thanks,

Alex

[Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM

[Michael S. Tsirkin]

On Sat, Oct 23, 2010 at 12:42:44PM -0600, Alex Williamson wrote:
> On Sat, 2010-10-23 at 19:29 +0200, Michael S. Tsirkin wrote:
> > On Sat, Oct 23, 2010 at 10:52:43AM -0600, Alex Williamson wrote:
> > > On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
> > > > On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> > > > > We need to pull in config.h or else kvm.h doesn't pull in
> > > > > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> > > > > defined.  This requires moving the object over to Makefile.target
> > > > > or else we can't find config-target.h
> > > > > 
> > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > > 
> > > > Why? We just moved it from .target to .objs, see
> > > > 889e30cc18e21f2091b77267dca8096d7dd34f8b.
> > > 
> > > Maybe that's why it used to work.  When building in the qemu-kvm.git
> > > tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
> > > KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
> > > allocated.  Then when I call msix_vector_use, I get a seg fault.
> > > Something is broken there.  Thanks,
> > > 
> > > Alex
> > 
> > This is hopefully fixed in the latest bits.
> > bd8b215bce453706c3951460cc7e6627ccb90314
> 
> Nope, my tree includes that.  It's not kvm_set_irq, it's kvm_msix_add,
> which dereferences msix_irq_entries, which is only allocated in
> msix_init if KVM_CAP_IRQCHIP is defined, which it's not.  Maybe you also
> meant to remove the ifdef from msix_init?  I also note there's another
> in msix_notify.  Thanks,
> 
> Alex

Not sure what's wrong:
$git grep KVM_CAP_IRQCHIP origin/master -- hw/msix.c
$

http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=blob;f=hw/msix.c;hb=HEAD

also does not show any ifdefs.

-- 
MST

[Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM

[Alex Williamson]

On Sat, Oct 23, 2010 at 2:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sat, Oct 23, 2010 at 12:42:44PM -0600, Alex Williamson wrote:
>> On Sat, 2010-10-23 at 19:29 +0200, Michael S. Tsirkin wrote:
>> > On Sat, Oct 23, 2010 at 10:52:43AM -0600, Alex Williamson wrote:
>> > > On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
>> > > > On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
>> > > > > We need to pull in config.h or else kvm.h doesn't pull in
>> > > > > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
>> > > > > defined.  This requires moving the object over to Makefile.target
>> > > > > or else we can't find config-target.h
>> > > > >
>> > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>> > > >
>> > > > Why? We just moved it from .target to .objs, see
>> > > > 889e30cc18e21f2091b77267dca8096d7dd34f8b.
>> > >
>> > > Maybe that's why it used to work.  When building in the qemu-kvm.git
>> > > tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
>> > > KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
>> > > allocated.  Then when I call msix_vector_use, I get a seg fault.
>> > > Something is broken there.  Thanks,
>> > >
>> > > Alex
>> >
>> > This is hopefully fixed in the latest bits.
>> > bd8b215bce453706c3951460cc7e6627ccb90314
>>
>> Nope, my tree includes that.  It's not kvm_set_irq, it's kvm_msix_add,
>> which dereferences msix_irq_entries, which is only allocated in
>> msix_init if KVM_CAP_IRQCHIP is defined, which it's not.  Maybe you also
>> meant to remove the ifdef from msix_init?  I also note there's another
>> in msix_notify.  Thanks,
>>
>> Alex
>
> Not sure what's wrong:
> $git grep KVM_CAP_IRQCHIP origin/master -- hw/msix.c
> $
>
> http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=blob;f=hw/msix.c;hb=HEAD
>
> also does not show any ifdefs.

Hmm, somehow my tree missed 763a04a920f1098e57ad6b46c91c3e531adc961d
Ignore this patch and I'll refresh again.  Sorry for the noise.

Alex