[Qemu-devel] [PATCH 1/2] pc: add machine type for 0.12

[Qemu-devel] [PATCH 1/2] pc: add machine type for 0.12

[Gerd Hoffmann]

Add a new machine type for qemu 0.12.

Also fixup the 0.11 machine type: msi for virtio-blk-pci was enabled
after the 0.11 release, so turn it off in the 0.11 machine type.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pc.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 7c791c4..fdaa52c 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1279,7 +1279,7 @@ void cmos_set_s3_resume(void)
 }
 
 static QEMUMachine pc_machine = {
-    .name = "pc-0.11",
+    .name = "pc-0.12",
     .alias = "pc",
     .desc = "Standard PC",
     .init = pc_init_pci,
@@ -1287,6 +1287,21 @@ static QEMUMachine pc_machine = {
     .is_default = 1,
 };
 
+static QEMUMachine pc_machine_v0_11 = {
+    .name = "pc-0.11",
+    .desc = "Standard PC, qemu 0.11",
+    .init = pc_init_pci,
+    .max_cpus = 255,
+    .compat_props = (CompatProperty[]) {
+        {
+            .driver   = "virtio-blk-pci",
+            .property = "vectors",
+            .value    = stringify(0),
+        },
+        { /* end of list */ }
+    }
+};
+
 static QEMUMachine pc_machine_v0_10 = {
     .name = "pc-0.10",
     .desc = "Standard PC, qemu 0.10",
@@ -1324,6 +1339,7 @@ static QEMUMachine isapc_machine = {
 static void pc_machine_init(void)
 {
     qemu_register_machine(&pc_machine);
+    qemu_register_machine(&pc_machine_v0_11);
     qemu_register_machine(&pc_machine_v0_10);
     qemu_register_machine(&isapc_machine);
 }
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/2] virtio: enable msi-x for console+balloon

[Gerd Hoffmann]

Enable MSI-X for virtio-console-pci and virtio-balloon-pci.
Add entries to the compatibility machine types so MSI-X will
be disabled for pc-0.10 and pc-0.11.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pc.c         |   16 ++++++++++++++++
 hw/virtio-pci.c |   11 +++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index fdaa52c..cb78923 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1297,6 +1297,14 @@ static QEMUMachine pc_machine_v0_11 = {
             .driver   = "virtio-blk-pci",
             .property = "vectors",
             .value    = stringify(0),
+        },{
+            .driver   = "virtio-balloon-pci",
+            .property = "vectors",
+            .value    = stringify(0),
+        },{
+            .driver   = "virtio-console-pci",
+            .property = "vectors",
+            .value    = stringify(0),
         },
         { /* end of list */ }
     }
@@ -1324,6 +1332,14 @@ static QEMUMachine pc_machine_v0_10 = {
             .driver   = "virtio-blk-pci",
             .property = "vectors",
             .value    = stringify(0),
+        },{
+            .driver   = "virtio-balloon-pci",
+            .property = "vectors",
+            .value    = stringify(0),
+        },{
+            .driver   = "virtio-console-pci",
+            .property = "vectors",
+            .value    = stringify(0),
         },
         { /* end of list */ }
     },
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index aebcf9d..cb8ab21 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -483,10 +483,13 @@ static int virtio_console_init_pci(PCIDevice *pci_dev)
     if (!vdev) {
         return -1;
     }
+    vdev->nvectors = proxy->nvectors;
     virtio_init_pci(proxy, vdev,
                     PCI_VENDOR_ID_REDHAT_QUMRANET,
                     PCI_DEVICE_ID_VIRTIO_CONSOLE,
                     proxy->class_code, 0x00);
+    /* make the actual value visible */
+    proxy->nvectors = vdev->nvectors;
     return 0;
 }
 
@@ -531,11 +534,14 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
     VirtIODevice *vdev;
 
     vdev = virtio_balloon_init(&pci_dev->qdev);
+    vdev->nvectors = proxy->nvectors;
     virtio_init_pci(proxy, vdev,
                     PCI_VENDOR_ID_REDHAT_QUMRANET,
                     PCI_DEVICE_ID_VIRTIO_BALLOON,
                     PCI_CLASS_MEMORY_RAM,
                     0x00);
+    /* make the actual value visible */
+    proxy->nvectors = vdev->nvectors;
     return 0;
 }
 
@@ -569,6 +575,7 @@ static PCIDeviceInfo virtio_info[] = {
         .init      = virtio_console_init_pci,
         .exit      = virtio_exit_pci,
         .qdev.props = (Property[]) {
+            DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
             DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
             DEFINE_PROP_END_OF_LIST(),
         },
@@ -579,6 +586,10 @@ static PCIDeviceInfo virtio_info[] = {
         .init      = virtio_balloon_init_pci,
         .exit      = virtio_exit_pci,
         .qdev.reset = virtio_pci_reset,
+        .qdev.props = (Property[]) {
+            DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
+            DEFINE_PROP_END_OF_LIST(),
+        },
     },{
         /* end of list */
     }
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH 2/2] virtio: enable msi-x for console+balloon

[Michael S. Tsirkin]

Sorry, I missed this the first time this was posted, and I see this in
staging now. Gerd, could you please explain the motivation for this
patch?

I assumed console/baloon interrupts are not performance critical, so
would we not be better off using a shared interrupt for these,
reserving MSI vectors for where performance matters?


On Wed, Nov 25, 2009 at 09:16:48AM +0100, Gerd Hoffmann wrote:
> Enable MSI-X for virtio-console-pci and virtio-balloon-pci.
> Add entries to the compatibility machine types so MSI-X will
> be disabled for pc-0.10 and pc-0.11.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pc.c         |   16 ++++++++++++++++
>  hw/virtio-pci.c |   11 +++++++++++
>  2 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index fdaa52c..cb78923 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1297,6 +1297,14 @@ static QEMUMachine pc_machine_v0_11 = {
>              .driver   = "virtio-blk-pci",
>              .property = "vectors",
>              .value    = stringify(0),
> +        },{
> +            .driver   = "virtio-balloon-pci",
> +            .property = "vectors",
> +            .value    = stringify(0),
> +        },{
> +            .driver   = "virtio-console-pci",
> +            .property = "vectors",
> +            .value    = stringify(0),
>          },
>          { /* end of list */ }
>      }
> @@ -1324,6 +1332,14 @@ static QEMUMachine pc_machine_v0_10 = {
>              .driver   = "virtio-blk-pci",
>              .property = "vectors",
>              .value    = stringify(0),
> +        },{
> +            .driver   = "virtio-balloon-pci",
> +            .property = "vectors",
> +            .value    = stringify(0),
> +        },{
> +            .driver   = "virtio-console-pci",
> +            .property = "vectors",
> +            .value    = stringify(0),
>          },
>          { /* end of list */ }
>      },
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index aebcf9d..cb8ab21 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -483,10 +483,13 @@ static int virtio_console_init_pci(PCIDevice *pci_dev)
>      if (!vdev) {
>          return -1;
>      }
> +    vdev->nvectors = proxy->nvectors;
>      virtio_init_pci(proxy, vdev,
>                      PCI_VENDOR_ID_REDHAT_QUMRANET,
>                      PCI_DEVICE_ID_VIRTIO_CONSOLE,
>                      proxy->class_code, 0x00);
> +    /* make the actual value visible */
> +    proxy->nvectors = vdev->nvectors;
>      return 0;
>  }
>  
> @@ -531,11 +534,14 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
>      VirtIODevice *vdev;
>  
>      vdev = virtio_balloon_init(&pci_dev->qdev);
> +    vdev->nvectors = proxy->nvectors;
>      virtio_init_pci(proxy, vdev,
>                      PCI_VENDOR_ID_REDHAT_QUMRANET,
>                      PCI_DEVICE_ID_VIRTIO_BALLOON,
>                      PCI_CLASS_MEMORY_RAM,
>                      0x00);
> +    /* make the actual value visible */
> +    proxy->nvectors = vdev->nvectors;
>      return 0;
>  }
>  
> @@ -569,6 +575,7 @@ static PCIDeviceInfo virtio_info[] = {
>          .init      = virtio_console_init_pci,
>          .exit      = virtio_exit_pci,
>          .qdev.props = (Property[]) {
> +            DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>              DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
>              DEFINE_PROP_END_OF_LIST(),
>          },
> @@ -579,6 +586,10 @@ static PCIDeviceInfo virtio_info[] = {
>          .init      = virtio_balloon_init_pci,
>          .exit      = virtio_exit_pci,
>          .qdev.reset = virtio_pci_reset,
> +        .qdev.props = (Property[]) {
> +            DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
> +            DEFINE_PROP_END_OF_LIST(),
> +        },
>      },{
>          /* end of list */
>      }
> -- 
> 1.6.2.5
> 
> 

[Qemu-devel] Re: [PATCH 2/2] virtio: enable msi-x for console+balloon

[Gerd Hoffmann]

On 12/07/09 10:37, Michael S. Tsirkin wrote:
>
> Sorry, I missed this the first time this was posted, and I see this in
> staging now. Gerd, could you please explain the motivation for this
> patch?
>
> I assumed console/baloon interrupts are not performance critical, so
> would we not be better off using a shared interrupt for these,
> reserving MSI vectors for where performance matters?

The motivation was to move them away from the ioapic, to reduce irq 
sharing of *other* devices which are connected to the ioapic too (i.e. 
usb, e1000, lsi, ...)

I'm aware that these are not performance-critical.  I've even tried to 
use 'vectors=1' because of that.  I expected that would make them use 
MSI-X, but a single IRQ line only.  Didn't work though.  Intentional?

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 2/2] virtio: enable msi-x for console+balloon

[Michael S. Tsirkin]

On Mon, Dec 07, 2009 at 11:32:36AM +0100, Gerd Hoffmann wrote:
> On 12/07/09 10:37, Michael S. Tsirkin wrote:
>>
>> Sorry, I missed this the first time this was posted, and I see this in
>> staging now. Gerd, could you please explain the motivation for this
>> patch?
>>
>> I assumed console/baloon interrupts are not performance critical, so
>> would we not be better off using a shared interrupt for these,
>> reserving MSI vectors for where performance matters?
>
> The motivation was to move them away from the ioapic, to reduce irq  
> sharing of *other* devices which are connected to the ioapic too (i.e.  
> usb, e1000, lsi, ...)

Let's convert these to MSI instead?
This will likely pay off long term,
e.g. with nested virtualization.

>
> I'm aware that these are not performance-critical.  I've even tried to  
> use 'vectors=1' because of that.  I expected that would make them use  
> MSI-X, but a single IRQ line only.  Didn't work though.  Intentional?

So it's even worse, we are using up 2 vectors per device? Ugh ...

vectors=1 currently will make guest fall back on
regular interrupts, because IRQ field is 
undefined when MSI is used, so there is
no way to distinguish between vq interrupt
and config change. This last thing is important
because it allows fastpath injection of MSI
interrupts directly from kernel without
notifying qemu to update IRQ field.

Maybe we can define something special for a single
vector, but this is not a use case I considered
so will need guest changes ...

> cheers,
>   Gerd

[Qemu-devel] Re: [PATCH 2/2] virtio: enable msi-x for console+balloon

[Gerd Hoffmann]

On 12/07/09 11:59, Michael S. Tsirkin wrote:
>> The motivation was to move them away from the ioapic, to reduce irq
>> sharing of *other* devices which are connected to the ioapic too (i.e.
>> usb, e1000, lsi, ...)
>
> Let's convert these to MSI instead?
> This will likely pay off long term,
> e.g. with nested virtualization.

Works only of the real hardware we emulate can do MSI-X too, otherwise 
guests simply wouldn't use it.  I think the only case where this could 
work out is e1000, newer revisions can do MSI-X.  Maybe also the 
upcoming megasas emulation Hannes is working on.

>> I'm aware that these are not performance-critical.  I've even tried to
>> use 'vectors=1' because of that.  I expected that would make them use
>> MSI-X, but a single IRQ line only.  Didn't work though.  Intentional?
>
> So it's even worse, we are using up 2 vectors per device? Ugh ...

Yes.  Three for balloon, but that can easily changed to two.

> no way to distinguish between vq interrupt
> and config change. This last thing is important
> because it allows fastpath injection of MSI
> interrupts directly from kernel without
> notifying qemu to update IRQ field.

Ah, *that* is the reason for the separate config interrupt.  Does the 
in-kernel injection matter for balloon+console?  I'd expect only 
virtio-net needs that when it is configured with vhost?

cheers
   Gerd

[Qemu-devel] Re: [PATCH 2/2] virtio: enable msi-x for console+balloon

[Michael S. Tsirkin]

On Mon, Dec 07, 2009 at 12:24:02PM +0100, Gerd Hoffmann wrote:
> On 12/07/09 11:59, Michael S. Tsirkin wrote:
>>> The motivation was to move them away from the ioapic, to reduce irq
>>> sharing of *other* devices which are connected to the ioapic too (i.e.
>>> usb, e1000, lsi, ...)
>>
>> Let's convert these to MSI instead?
>> This will likely pay off long term,
>> e.g. with nested virtualization.
>
> Works only of the real hardware we emulate can do MSI-X too, otherwise  
> guests simply wouldn't use it.

Sure. Naturally, improving IRQ routing so that e.g. lsi
and usb do not share an interrupt would also be a good idea
regardless.
Also - why not simply use virtio? I assume you are talking about
guests with virtio support otherwise MSI support in virtio
won't matter for them.

> I think the only case where this could  
> work out is e1000, newer revisions can do MSI-X.  Maybe also the  
> upcoming megasas emulation Hannes is working on.

Hmm. I would expect high-end storage to support MSI-X as well.
Could you point me to linux drivers for devices we emulate
so that I can check?

>>> I'm aware that these are not performance-critical.  I've even tried to
>>> use 'vectors=1' because of that.  I expected that would make them use
>>> MSI-X, but a single IRQ line only.  Didn't work though.  Intentional?
>>
>> So it's even worse, we are using up 2 vectors per device? Ugh ...
>
> Yes.  Three for balloon, but that can easily changed to two.

Yes, please, make this change for 0.12.

>> no way to distinguish between vq interrupt
>> and config change. This last thing is important
>> because it allows fastpath injection of MSI
>> interrupts directly from kernel without
>> notifying qemu to update IRQ field.
>
> Ah, *that* is the reason for the separate config interrupt.  Does the  
> in-kernel injection matter for balloon+console?

No, but changing this will need updating guests.
And if we do update guests anyway, I would suggest that
we put IRQ field in guest memory, with an atomic set,
and guest would get it with test and clear, so that
we get a generic interface and not something
specific for console/baloon.
Naturally, this needs a feature bit, and it won't happen
in 0.12/2.6.33 timeframe.


> I'd expect only  
> virtio-net needs that when it is configured with vhost?
>
> cheers
>   Gerd

Any other device will need the same if it has
a kernel backend.

-- 
MST

[Qemu-devel] Re: [PATCH 2/2] virtio: enable msi-x for console+balloon

[Gerd Hoffmann]

On 12/07/09 14:03, Michael S. Tsirkin wrote:
> Also - why not simply use virtio? I assume you are talking about
> guests with virtio support otherwise MSI support in virtio
> won't matter for them.

Point.  Guests with virtio-console+balloon most likely can use virtio 
for storage+net too, so nothing performance-critical is left in ioapic 
irq space and IRQ sharing for the leftovers (just usb I think) shouldn't 
be a big issue.

>> Yes.  Three for balloon, but that can easily changed to two.
>
> Yes, please, make this change for 0.12.

We can also simply drop this ...

> No, but changing this will need updating guests.

... or postphone until we have the "one MSI-X vector" support working.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 2/2] virtio: enable msi-x for console+balloon

[Michael S. Tsirkin]

On Mon, Dec 07, 2009 at 02:16:33PM +0100, Gerd Hoffmann wrote:
> On 12/07/09 14:03, Michael S. Tsirkin wrote:
>> Also - why not simply use virtio? I assume you are talking about
>> guests with virtio support otherwise MSI support in virtio
>> won't matter for them.
>
> Point.  Guests with virtio-console+balloon most likely can use virtio  
> for storage+net too, so nothing performance-critical is left in ioapic  
> irq space and IRQ sharing for the leftovers (just usb I think) shouldn't  
> be a big issue.
>
>>> Yes.  Three for balloon, but that can easily changed to two.
>>
>> Yes, please, make this change for 0.12.
>
> We can also simply drop this ...
>
>> No, but changing this will need updating guests.
>
> ... or postphone until we have the "one MSI-X vector" support working.

OK.
Anthony, could you drop this from staging for now please?

Gerd, any idea whether "one MSI-X vector" feature is worth pursuing?

-- 
MST

[Qemu-devel] Re: [PATCH 2/2] virtio: enable msi-x for console+balloon

[Gerd Hoffmann]

On 12/07/09 15:13, Michael S. Tsirkin wrote:
> Gerd, any idea whether "one MSI-X vector" feature is worth pursuing?

I'd rate it low priority.  First because virtio-enabled guests can use 
virtio-net+blk.  Also because with the upcoming p35 support we'll get a 
more modern pc emulation including a ioapic with all 24 IRQ lines being 
wired up, which will help reducing IRQ sharing too.

MSI support (no -X) could be more intresting (for emulated devices) as 
it is older and thus support is more common.  My T60 for example has no 
device with MSI-X support but 9 with MSI support.  Linux turns on MSI 
for 7 of them (4 PCIe ports, e1000, ahci, iwl3945).

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 2/2] virtio: enable msi-x for console+balloon

[Michael S. Tsirkin]

On Mon, Dec 07, 2009 at 03:30:09PM +0100, Gerd Hoffmann wrote:
> On 12/07/09 15:13, Michael S. Tsirkin wrote:
>> Gerd, any idea whether "one MSI-X vector" feature is worth pursuing?
>
> I'd rate it low priority.  First because virtio-enabled guests can use  
> virtio-net+blk.  Also because with the upcoming p35 support we'll get a  
> more modern pc emulation including a ioapic with all 24 IRQ lines being  
> wired up, which will help reducing IRQ sharing too.
>
> MSI support (no -X) could be more intresting (for emulated devices) as  
> it is older and thus support is more common.  My T60 for example has no  
> device with MSI-X support but 9 with MSI support.  Linux turns on MSI  
> for 7 of them (4 PCIe ports, e1000, ahci, iwl3945).
>
> cheers,
>   Gerd

Yes, but what matters is guest support I think.
On windows it seems that both msix and msi support
were added simulataneously.

It won't be hard to add MSI support, but let's determine
when is it needed.

-- 
MST