[Qemu-devel] [PATCH] virtio-pci: report an error when disable msix

[Qemu-devel] [PATCH] virtio-pci: report an error when disable msix

[Amos Kong]

QEMU remains 4k memory for PCI BAR, each msix entry takes 16 bytes.
If user assigns more than 128 vectors, msix resource isn't enough,
so msix will be disabled.

This patch addes a note when fail to init exclusive bars for msix.

 qemu -device virtio-net-pci,netdev=h1,vectors=129,mq=on \
      -netdev tap,id=h1,queues=8

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/virtio/virtio-pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..ea5dcdf 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -976,6 +976,8 @@ static void virtio_pci_device_plugged(DeviceState *d)
 
     if (proxy->nvectors &&
         msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) {
+        error_report("%s: unable to init exclusive bars for msix, disable msix",
+                     __func__);
         proxy->nvectors = 0;
     }
 
-- 
1.9.0

Re: [Qemu-devel] [PATCH] virtio-pci: report an error when disable msix

[Michael S. Tsirkin]

On Thu, May 22, 2014 at 05:02:17PM +0800, Amos Kong wrote:
> QEMU remains 4k memory for PCI BAR, each msix entry takes 16 bytes.
> If user assigns more than 128 vectors, msix resource isn't enough,
> so msix will be disabled.
> 
> This patch addes a note when fail to init exclusive bars for msix.
> 
>  qemu -device virtio-net-pci,netdev=h1,vectors=129,mq=on \
>       -netdev tap,id=h1,queues=8
> 
> Signed-off-by: Amos Kong <akong@redhat.com>

OK I guess, but how about removing the limit instead?

> ---
>  hw/virtio/virtio-pci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ce97514..ea5dcdf 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -976,6 +976,8 @@ static void virtio_pci_device_plugged(DeviceState *d)
>  
>      if (proxy->nvectors &&
>          msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) {
> +        error_report("%s: unable to init exclusive bars for msix, disable msix",
> +                     __func__);
>          proxy->nvectors = 0;
>      }
>  
> -- 
> 1.9.0

Re: [Qemu-devel] [PATCH] virtio-pci: report an error when disable msix

[Amos Kong]

CC: Alex

On Thu, May 22, 2014 at 12:11:47PM +0300, Michael S. Tsirkin wrote:
> On Thu, May 22, 2014 at 05:02:17PM +0800, Amos Kong wrote:
> > QEMU remains 4k memory for PCI BAR, each msix entry takes 16 bytes.
> > If user assigns more than 128 vectors, msix resource isn't enough,
> > so msix will be disabled.
> > 
> > This patch addes a note when fail to init exclusive bars for msix.
> > 
> >  qemu -device virtio-net-pci,netdev=h1,vectors=129,mq=on \
> >       -netdev tap,id=h1,queues=8
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> 
> OK I guess, but how about removing the limit instead?

The limit of nentries had been 2048, it was changed to 128 by commit 53f94925.

If we remove this limit checking, it also can't pass sanity test,
ranges_overlap() fails.

    /* 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)) ||
         ... ) {
        return -EINVAL;
    }

Related commits:
==============================================================
|commit 53f949254ad2435bfd45cb0dee96f246a0bdd7e3
|Author: Alex Williamson <alex.williamson@redhat.com>
|Date:   Thu Jun 14 12:15:51 2012 -0600
|
|    msix: Add simple BAR allocation MSIX setup functions
|
|diff --git a/hw/msix.c b/hw/msix.c
|index b64f109..bafea94 100644
|--- a/hw/msix.c
|+++ b/hw/msix.c
|@@ -299,6 +299,45 @@ err_config:
|@@ -299,6 +299,45 @@ 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.  Do not use these elsewhere!
|+     */

^^^^^^^^^^^^^^^^^^^^^ the limit comes from here

|+#define MSIX_EXCLUSIVE_BAR_SIZE 4096
|+#define MSIX_EXCLUSIVE_BAR_PBA_OFFSET (MSIX_EXCLUSIVE_BAR_SIZE / 2)
|+
|+    if (nentries * PCI_MSIX_ENTRY_SIZE >
|MSIX_EXCLUSIVE_BAR_PBA_OFFSET) {
|+        return -EINVAL;
|+    }

> > ---
> >  hw/virtio/virtio-pci.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index ce97514..ea5dcdf 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -976,6 +976,8 @@ static void virtio_pci_device_plugged(DeviceState *d)
> >  
> >      if (proxy->nvectors &&
> >          msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) {
> > +        error_report("%s: unable to init exclusive bars for msix, disable msix",
> > +                     __func__);
> >          proxy->nvectors = 0;
> >      }
> >  
> > -- 
> > 1.9.0

-- 
			Amos.

Re: [Qemu-devel] [PATCH] virtio-pci: report an error when disable msix

[Peter Maydell]

On 22 May 2014 10:02, Amos Kong <akong@redhat.com> wrote:
> QEMU remains 4k memory for PCI BAR, each msix entry takes 16 bytes.
> If user assigns more than 128 vectors, msix resource isn't enough,
> so msix will be disabled.
>
> This patch addes a note when fail to init exclusive bars for msix.
>
>  qemu -device virtio-net-pci,netdev=h1,vectors=129,mq=on \
>       -netdev tap,id=h1,queues=8
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  hw/virtio/virtio-pci.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ce97514..ea5dcdf 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -976,6 +976,8 @@ static void virtio_pci_device_plugged(DeviceState *d)
>
>      if (proxy->nvectors &&
>          msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) {
> +        error_report("%s: unable to init exclusive bars for msix, disable msix",
> +                     __func__);

I think you probably mean "disabling".

>          proxy->nvectors = 0;
>      }

thanks
-- PMM