diff mbox

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

Message ID 512E0E31.5080207@redhat.com
State New
Headers show

Commit Message

Gal Hammer Feb. 27, 2013, 1:46 p.m. UTC 
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(-)

Comments

Paolo Bonzini Feb. 27, 2013, 1:47 p.m. UTC | #1 
Il 27/02/2013 14:46, Gal Hammer ha scritto:
> 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.
> 
> I've added the BC property. I hope it is in the right format.

Yes, it's okay though usually we send patches with git-send-email.  It's
up to mst whether to take the attached patch or not. :)

Anyhow,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks!

Paolo
Michael S. Tsirkin Feb. 27, 2013, 2:03 p.m. UTC | #2 
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?
diff mbox

Patch

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),