Patchwork How many msi-x vectors should be allocated for the virtio-serial device?

login
register
mail settings
Submitter Gal Hammer
Date Feb. 27, 2013, 2:51 p.m.
Message ID <512E1D54.5040607@redhat.com>
Download mbox | patch
Permalink /patch/223623/
State New
Headers show

Comments

Gal Hammer - Feb. 27, 2013, 2:51 p.m.
On 27/02/2013 16:03, Michael S. Tsirkin wrote:
> On Wed, Feb 27, 2013 at 03:46:25PM +0200, Gal Hammer wrote:
>> On 27/02/2013 15:28, Paolo Bonzini wrote:
>>> Il 27/02/2013 14:24, Gal Hammer ha scritto:
>>>> On 27/02/2013 14:35, Amit Shah wrote:
>>>>> On (Mon) 04 Feb 2013 [11:59:02], Michael S. Tsirkin wrote:
>>>>>> On Mon, Feb 04, 2013 at 10:42:32AM +0100, Paolo Bonzini wrote:
>>>>>>> Il 03/02/2013 13:11, Michael S. Tsirkin ha scritto:
>>>>>>>>>>    static Property virtio_serial_properties[] = {
>>>>>>>>>>        DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
>>>>>>>>>> VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
>>>>>>>>>> -    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
>>>>>>>>>> DEV_NVECTORS_UNSPECIFIED),
>>>>>>>>>> +    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>>>>>>>>>>        DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
>>>>>>>>>>        DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
>>>>>>>>>>        DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy,
>>>>>>>>>> serial.max_virtserial_ports, 31),
>>>>>>>>>>
>>>>>>>>>> plus the backwards-compatibility stuff.
>>>>>>>>>>
>>>>>>>>>> Paolo
>>>>>>>> Makes sense, but the logic in virtio_serial_init_pci is
>>>>>>>> then dead code and should go away.
>>>>>>>>
>>>>>>>
>>>>>>> It won't be dead code for the backwards-compatible machine types (that
>>>>>>> use DEV_NVECTORS_UNSPECIFIED).
>>>>>>>
>>>>>>> Paolo
>>>>>>
>>>>>> Good point. Ack. Want to post this with proper signature etc?
>>>>>
>>>>> Gal, do you want to submit a patch for this?
>>>>>
>>>>>          Amit
>>>>>
>>>>
>>>> Attached.
>>>
>>> You need to add backwards-compatibility properties in hw/pc.h.
>>>
>>> Paolo
>>>
>>
>> I've added the BC property. I hope it is in the right format.
>>
>> Thanks,
>>
>>      Gal.
>>
>
>> >From db49a36ecc4de5744415357691140fb68df4fae2 Mon Sep 17 00:00:00 2001
>> From: Gal Hammer <ghammer@redhat.com>
>> Date: Wed, 27 Feb 2013 15:15:31 +0200
>> Subject: [PATCH] Set virtio-serial device to have a default of 2 MSI vectors.
>>
>> The virtio-serial device is expected to use 2 MSI vectors, one for
>> control queue and a second shared for all queues.
>>
>> Signed-off-by: Gal Hammer <ghammer@redhat.com>
>> ---
>>   hw/pc.h         | 4 ++++
>>   hw/virtio-pci.c | 5 ++++-
>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/pc.h b/hw/pc.h
>> index da1b102..d63d411 100644
>> --- a/hw/pc.h
>> +++ b/hw/pc.h
>> @@ -216,6 +216,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
>>               .driver   = "virtio-blk-pci",\
>>               .property = "discard_granularity",\
>>               .value    = stringify(0),\
>> +	},{\
>> +            .driver   = "virtio-serial-pci",\
>> +            .property = "vectors",\
>> +            .value    = stringify(DEV_NVECTORS_UNSPECIFIED),\
>>   	}
>>
>>   #endif
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index a869f53..ba56ab2 100644
>> --- a/hw/virtio-pci.c
>> +++ b/hw/virtio-pci.c
>> @@ -975,6 +975,9 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
>>       if (!vdev) {
>>           return -1;
>>       }
>> +
>> +    /* backwards-compatibility with machines that were created with
>> +       DEV_NVECTORS_UNSPECIFIED */
>>       vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
>>                                           ? proxy->serial.max_virtserial_ports + 1
>>                                           : proxy->nvectors;
>> @@ -1155,7 +1158,7 @@ static const TypeInfo virtio_net_info = {
>>
>>   static Property virtio_serial_properties[] = {
>>       DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
>> -    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, DEV_NVECTORS_UNSPECIFIED),
>> +    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>>       DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
>>       DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
>>       DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
>> --
>> 1.8.1.2
>>
>
>
> Looks good but just making sure - you did test with compat machine type,
> yes?
>

After a small fix (attached), I've executed:

$ x86_64-softmmu/qemu-system-x86_64 -device virtio-serial-pci
vectors = 2

$ x86_64-softmmu/qemu-system-x86_64 -device virtio-serial-pci -M pc-0.14
vectors = 32

Is this test is enough?

     Gal.
From fddf3136cc2cd0372c52a09424715b15c6ad131f Mon Sep 17 00:00:00 2001
From: Gal Hammer <ghammer@redhat.com>
Date: Wed, 27 Feb 2013 15:15:31 +0200
Subject: [PATCH] Set virtio-serial device to have a default of 2 MSI vectors.

The virtio-serial device is expected to use 2 MSI vectors, one for
control queue and a second shared for all queues.

Signed-off-by: Gal Hammer <ghammer@redhat.com>
---
 hw/pc.h         | 5 +++++
 hw/virtio-pci.c | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)
Michael S. Tsirkin - Feb. 27, 2013, 3:15 p.m.
On Wed, Feb 27, 2013 at 04:51:00PM +0200, Gal Hammer wrote:
> On 27/02/2013 16:03, Michael S. Tsirkin wrote:
> >On Wed, Feb 27, 2013 at 03:46:25PM +0200, Gal Hammer wrote:
> >>On 27/02/2013 15:28, Paolo Bonzini wrote:
> >>>Il 27/02/2013 14:24, Gal Hammer ha scritto:
> >>>>On 27/02/2013 14:35, Amit Shah wrote:
> >>>>>On (Mon) 04 Feb 2013 [11:59:02], Michael S. Tsirkin wrote:
> >>>>>>On Mon, Feb 04, 2013 at 10:42:32AM +0100, Paolo Bonzini wrote:
> >>>>>>>Il 03/02/2013 13:11, Michael S. Tsirkin ha scritto:
> >>>>>>>>>>   static Property virtio_serial_properties[] = {
> >>>>>>>>>>       DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
> >>>>>>>>>>VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
> >>>>>>>>>>-    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
> >>>>>>>>>>DEV_NVECTORS_UNSPECIFIED),
> >>>>>>>>>>+    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
> >>>>>>>>>>       DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
> >>>>>>>>>>       DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> >>>>>>>>>>       DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy,
> >>>>>>>>>>serial.max_virtserial_ports, 31),
> >>>>>>>>>>
> >>>>>>>>>>plus the backwards-compatibility stuff.
> >>>>>>>>>>
> >>>>>>>>>>Paolo
> >>>>>>>>Makes sense, but the logic in virtio_serial_init_pci is
> >>>>>>>>then dead code and should go away.
> >>>>>>>>
> >>>>>>>
> >>>>>>>It won't be dead code for the backwards-compatible machine types (that
> >>>>>>>use DEV_NVECTORS_UNSPECIFIED).
> >>>>>>>
> >>>>>>>Paolo
> >>>>>>
> >>>>>>Good point. Ack. Want to post this with proper signature etc?
> >>>>>
> >>>>>Gal, do you want to submit a patch for this?
> >>>>>
> >>>>>         Amit
> >>>>>
> >>>>
> >>>>Attached.
> >>>
> >>>You need to add backwards-compatibility properties in hw/pc.h.
> >>>
> >>>Paolo
> >>>
> >>
> >>I've added the BC property. I hope it is in the right format.
> >>
> >>Thanks,
> >>
> >>     Gal.
> >>
> >
> >>>From db49a36ecc4de5744415357691140fb68df4fae2 Mon Sep 17 00:00:00 2001
> >>From: Gal Hammer <ghammer@redhat.com>
> >>Date: Wed, 27 Feb 2013 15:15:31 +0200
> >>Subject: [PATCH] Set virtio-serial device to have a default of 2 MSI vectors.
> >>
> >>The virtio-serial device is expected to use 2 MSI vectors, one for
> >>control queue and a second shared for all queues.
> >>
> >>Signed-off-by: Gal Hammer <ghammer@redhat.com>
> >>---
> >>  hw/pc.h         | 4 ++++
> >>  hw/virtio-pci.c | 5 ++++-
> >>  2 files changed, 8 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/hw/pc.h b/hw/pc.h
> >>index da1b102..d63d411 100644
> >>--- a/hw/pc.h
> >>+++ b/hw/pc.h
> >>@@ -216,6 +216,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
> >>              .driver   = "virtio-blk-pci",\
> >>              .property = "discard_granularity",\
> >>              .value    = stringify(0),\
> >>+	},{\
> >>+            .driver   = "virtio-serial-pci",\
> >>+            .property = "vectors",\
> >>+            .value    = stringify(DEV_NVECTORS_UNSPECIFIED),\
> >>  	}
> >>
> >>  #endif
> >>diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> >>index a869f53..ba56ab2 100644
> >>--- a/hw/virtio-pci.c
> >>+++ b/hw/virtio-pci.c
> >>@@ -975,6 +975,9 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
> >>      if (!vdev) {
> >>          return -1;
> >>      }
> >>+
> >>+    /* backwards-compatibility with machines that were created with
> >>+       DEV_NVECTORS_UNSPECIFIED */
> >>      vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
> >>                                          ? proxy->serial.max_virtserial_ports + 1
> >>                                          : proxy->nvectors;
> >>@@ -1155,7 +1158,7 @@ static const TypeInfo virtio_net_info = {
> >>
> >>  static Property virtio_serial_properties[] = {
> >>      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
> >>-    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, DEV_NVECTORS_UNSPECIFIED),
> >>+    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
> >>      DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
> >>      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> >>      DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
> >>--
> >>1.8.1.2
> >>
> >
> >
> >Looks good but just making sure - you did test with compat machine type,
> >yes?
> >
> 
> After a small fix (attached), I've executed:
> 
> $ x86_64-softmmu/qemu-system-x86_64 -device virtio-serial-pci
> vectors = 2
> 
> $ x86_64-softmmu/qemu-system-x86_64 -device virtio-serial-pci -M pc-0.14
> vectors = 32
> 
> Is this test is enough?
> 
>     Gal.
> 

You'll want to test 1.4 I guess?


> >From fddf3136cc2cd0372c52a09424715b15c6ad131f Mon Sep 17 00:00:00 2001
> From: Gal Hammer <ghammer@redhat.com>
> Date: Wed, 27 Feb 2013 15:15:31 +0200
> Subject: [PATCH] Set virtio-serial device to have a default of 2 MSI vectors.
> 
> The virtio-serial device is expected to use 2 MSI vectors, one for
> control queue and a second shared for all queues.
> 
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---
>  hw/pc.h         | 5 +++++
>  hw/virtio-pci.c | 5 ++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pc.h b/hw/pc.h
> index da1b102..f2c1b1c 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -216,6 +216,11 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
>              .driver   = "virtio-blk-pci",\
>              .property = "discard_granularity",\
>              .value    = stringify(0),\
> +	},{\
> +            .driver   = "virtio-serial-pci",\
> +            .property = "vectors",\
> +            /* DEV_NVECTORS_UNSPECIFIED as a uint32_t string */\
> +            .value    = stringify(0xFFFFFFFF),\
>  	}
>  
>  #endif
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index a869f53..ba56ab2 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -975,6 +975,9 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
>      if (!vdev) {
>          return -1;
>      }
> +
> +    /* backwards-compatibility with machines that were created with
> +       DEV_NVECTORS_UNSPECIFIED */
>      vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
>                                          ? proxy->serial.max_virtserial_ports + 1
>                                          : proxy->nvectors;
> @@ -1155,7 +1158,7 @@ static const TypeInfo virtio_net_info = {
>  
>  static Property virtio_serial_properties[] = {
>      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
> -    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, DEV_NVECTORS_UNSPECIFIED),
> +    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>      DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
>      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
>      DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
> -- 
> 1.8.1.2
>
Michael S. Tsirkin - Feb. 27, 2013, 3:27 p.m.
> >From fddf3136cc2cd0372c52a09424715b15c6ad131f Mon Sep 17 00:00:00 2001
> From: Gal Hammer <ghammer@redhat.com>
> Date: Wed, 27 Feb 2013 15:15:31 +0200
> Subject: [PATCH] Set virtio-serial device to have a default of 2 MSI vectors.
> 
> The virtio-serial device is expected to use 2 MSI vectors, one for
> control queue and a second shared for all queues.
> 
> Signed-off-by: Gal Hammer <ghammer@redhat.com>

Thanks, applied.

> ---
>  hw/pc.h         | 5 +++++
>  hw/virtio-pci.c | 5 ++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pc.h b/hw/pc.h
> index da1b102..f2c1b1c 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -216,6 +216,11 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
>              .driver   = "virtio-blk-pci",\
>              .property = "discard_granularity",\
>              .value    = stringify(0),\
> +	},{\
> +            .driver   = "virtio-serial-pci",\
> +            .property = "vectors",\
> +            /* DEV_NVECTORS_UNSPECIFIED as a uint32_t string */\
> +            .value    = stringify(0xFFFFFFFF),\
>  	}
>  
>  #endif
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index a869f53..ba56ab2 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -975,6 +975,9 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
>      if (!vdev) {
>          return -1;
>      }
> +
> +    /* backwards-compatibility with machines that were created with
> +       DEV_NVECTORS_UNSPECIFIED */
>      vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
>                                          ? proxy->serial.max_virtserial_ports + 1
>                                          : proxy->nvectors;
> @@ -1155,7 +1158,7 @@ static const TypeInfo virtio_net_info = {
>  
>  static Property virtio_serial_properties[] = {
>      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
> -    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, DEV_NVECTORS_UNSPECIFIED),
> +    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>      DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
>      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
>      DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
> -- 
> 1.8.1.2
>

Patch

diff --git a/hw/pc.h b/hw/pc.h
index da1b102..f2c1b1c 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -216,6 +216,11 @@  int e820_add_entry(uint64_t, uint64_t, uint32_t);
             .driver   = "virtio-blk-pci",\
             .property = "discard_granularity",\
             .value    = stringify(0),\
+	},{\
+            .driver   = "virtio-serial-pci",\
+            .property = "vectors",\
+            /* DEV_NVECTORS_UNSPECIFIED as a uint32_t string */\
+            .value    = stringify(0xFFFFFFFF),\
 	}
 
 #endif
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index a869f53..ba56ab2 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -975,6 +975,9 @@  static int virtio_serial_init_pci(PCIDevice *pci_dev)
     if (!vdev) {
         return -1;
     }
+
+    /* backwards-compatibility with machines that were created with
+       DEV_NVECTORS_UNSPECIFIED */
     vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
                                         ? proxy->serial.max_virtserial_ports + 1
                                         : proxy->nvectors;
@@ -1155,7 +1158,7 @@  static const TypeInfo virtio_net_info = {
 
 static Property virtio_serial_properties[] = {
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
-    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, DEV_NVECTORS_UNSPECIFIED),
+    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),