[Qemu-devel] [PATCH] virtio-serial: Allow one MSI-X vector per virtqueue

[Qemu-devel] [PATCH] virtio-serial: Allow one MSI-X vector per virtqueue

[zanghongyong]

From: Hongyong Zang <zanghongyong@huawei.com>

In pci_enable_msix(), the guest's virtio-serial driver tries to set msi-x
with one vector per queue. But it fails and eventually all virtio-serial
ports share one MSI-X vector. Because every virtio-serial port has *two*
virtqueues, virtio-serial needs (port+1)*2 vectors other than (port+1).

This patch allows every virtqueue to have its own MSI-X vector.
(When the MSI-X vectors needed are more than MSIX_MAX_ENTRIES defined in
qemu: msix.c, all the queues still share one MSI-X vector as before.)

Signed-off-by: Hongyong Zang <zanghongyong@huawei.com>
---
 hw/virtio-pci.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 77b75bc..2c9c6fb 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -718,8 +718,11 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
         return -1;
     }
     vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
-                                        ? proxy->serial.max_virtserial_ports + 1
+                                        ? (proxy->serial.max_virtserial_ports + 1) * 2
                                         : proxy->nvectors;
+    /*msix.c: #define MSIX_MAX_ENTRIES 32*/
+    if (vdev->nvectors > 32)
+        vdev->nvectors = 32;
     virtio_init_pci(proxy, vdev);
     proxy->nvectors = vdev->nvectors;
     return 0;
-- 
1.7.1

Re: [Qemu-devel] [PATCH] virtio-serial: Allow one MSI-X vector per virtqueue

[Amit Shah]

On (Fri) 16 Dec 2011 [09:14:26], zanghongyong@huawei.com wrote:
> From: Hongyong Zang <zanghongyong@huawei.com>
> 
> In pci_enable_msix(), the guest's virtio-serial driver tries to set msi-x
> with one vector per queue. But it fails and eventually all virtio-serial
> ports share one MSI-X vector. Because every virtio-serial port has *two*
> virtqueues, virtio-serial needs (port+1)*2 vectors other than (port+1).

Ouch, good catch.

One comment below:

> This patch allows every virtqueue to have its own MSI-X vector.
> (When the MSI-X vectors needed are more than MSIX_MAX_ENTRIES defined in
> qemu: msix.c, all the queues still share one MSI-X vector as before.)
> 
> Signed-off-by: Hongyong Zang <zanghongyong@huawei.com>
> ---
>  hw/virtio-pci.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 77b75bc..2c9c6fb 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -718,8 +718,11 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
>          return -1;
>      }
>      vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
> -                                        ? proxy->serial.max_virtserial_ports + 1
> +                                        ? (proxy->serial.max_virtserial_ports + 1) * 2
>                                          : proxy->nvectors;
> +    /*msix.c: #define MSIX_MAX_ENTRIES 32*/
> +    if (vdev->nvectors > 32)
> +        vdev->nvectors = 32;

This change isn't needed: if the proxy->nvectors value exceeds the max
allowed, virtio_init_pci() will end up using a shared vector instead
of separate ones.

Thanks,

		Amit

Re: [Qemu-devel] [PATCH] virtio-serial: Allow one MSI-X vector per virtqueue

[Zang Hongyong]

于 2011/12/16,星期五 17:39, Amit Shah 写道:
> On (Fri) 16 Dec 2011 [09:14:26], zanghongyong@huawei.com wrote:
>> From: Hongyong Zang<zanghongyong@huawei.com>
>>
>> In pci_enable_msix(), the guest's virtio-serial driver tries to set msi-x
>> with one vector per queue. But it fails and eventually all virtio-serial
>> ports share one MSI-X vector. Because every virtio-serial port has *two*
>> virtqueues, virtio-serial needs (port+1)*2 vectors other than (port+1).
> Ouch, good catch.
>
> One comment below:
>
>> This patch allows every virtqueue to have its own MSI-X vector.
>> (When the MSI-X vectors needed are more than MSIX_MAX_ENTRIES defined in
>> qemu: msix.c, all the queues still share one MSI-X vector as before.)
>>
>> Signed-off-by: Hongyong Zang<zanghongyong@huawei.com>
>> ---
>>   hw/virtio-pci.c |    5 ++++-
>>   1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index 77b75bc..2c9c6fb 100644
>> --- a/hw/virtio-pci.c
>> +++ b/hw/virtio-pci.c
>> @@ -718,8 +718,11 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
>>           return -1;
>>       }
>>       vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
>> -                                        ? proxy->serial.max_virtserial_ports + 1
>> +                                        ? (proxy->serial.max_virtserial_ports + 1) * 2
>>                                           : proxy->nvectors;
>> +    /*msix.c: #define MSIX_MAX_ENTRIES 32*/
>> +    if (vdev->nvectors>  32)
>> +        vdev->nvectors = 32;
> This change isn't needed: if the proxy->nvectors value exceeds the max
> allowed, virtio_init_pci() will end up using a shared vector instead
> of separate ones.
>
> Thanks,
>
> 		Amit
>
> .
>
Hi Amit,
If the nvectors exceeds the max, msix_init() will return -EINVAL in QEMU,
and the front-end driver in Guest will use regular interrupt instead of 
MSI-X.

Hongyong

Re: [Qemu-devel] [PATCH] virtio-serial: Allow one MSI-X vector per virtqueue

[Amit Shah]

On (Mon) 19 Dec 2011 [14:09:43], Zang Hongyong wrote:
> 于 2011/12/16,星期五 17:39, Amit Shah 写道:
> >On (Fri) 16 Dec 2011 [09:14:26], zanghongyong@huawei.com wrote:
> >>From: Hongyong Zang<zanghongyong@huawei.com>
> >>
> >>In pci_enable_msix(), the guest's virtio-serial driver tries to set msi-x
> >>with one vector per queue. But it fails and eventually all virtio-serial
> >>ports share one MSI-X vector. Because every virtio-serial port has *two*
> >>virtqueues, virtio-serial needs (port+1)*2 vectors other than (port+1).
> >Ouch, good catch.
> >
> >One comment below:
> >
> >>This patch allows every virtqueue to have its own MSI-X vector.
> >>(When the MSI-X vectors needed are more than MSIX_MAX_ENTRIES defined in
> >>qemu: msix.c, all the queues still share one MSI-X vector as before.)
> >>
> >>Signed-off-by: Hongyong Zang<zanghongyong@huawei.com>
> >>---
> >>  hw/virtio-pci.c |    5 ++++-
> >>  1 files changed, 4 insertions(+), 1 deletions(-)
> >>
> >>diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> >>index 77b75bc..2c9c6fb 100644
> >>--- a/hw/virtio-pci.c
> >>+++ b/hw/virtio-pci.c
> >>@@ -718,8 +718,11 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
> >>          return -1;
> >>      }
> >>      vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
> >>-                                        ? proxy->serial.max_virtserial_ports + 1
> >>+                                        ? (proxy->serial.max_virtserial_ports + 1) * 2
> >>                                          : proxy->nvectors;
> >>+    /*msix.c: #define MSIX_MAX_ENTRIES 32*/
> >>+    if (vdev->nvectors>  32)
> >>+        vdev->nvectors = 32;
> >This change isn't needed: if the proxy->nvectors value exceeds the max
> >allowed, virtio_init_pci() will end up using a shared vector instead
> >of separate ones.
> >
> Hi Amit,
> If the nvectors exceeds the max, msix_init() will return -EINVAL in QEMU,
> and the front-end driver in Guest will use regular interrupt instead
> of MSI-X.

In that case, I believe msix_init() should be changed to attempt to
share interrupts instead of drivers doing this by themselves.

		Amit

Re: [Qemu-devel] [PATCH] virtio-serial: Allow one MSI-X vector per virtqueue

[Zang Hongyong]

于 2011/12/19,星期一 15:26, Amit Shah 写道:
> On (Mon) 19 Dec 2011 [14:09:43], Zang Hongyong wrote:
>> 于 2011/12/16,星期五 17:39, Amit Shah 写道:
>>> On (Fri) 16 Dec 2011 [09:14:26], zanghongyong@huawei.com wrote:
>>>> From: Hongyong Zang<zanghongyong@huawei.com>
>>>>
>>>> In pci_enable_msix(), the guest's virtio-serial driver tries to set msi-x
>>>> with one vector per queue. But it fails and eventually all virtio-serial
>>>> ports share one MSI-X vector. Because every virtio-serial port has *two*
>>>> virtqueues, virtio-serial needs (port+1)*2 vectors other than (port+1).
>>> Ouch, good catch.
>>>
>>> One comment below:
>>>
>>>> This patch allows every virtqueue to have its own MSI-X vector.
>>>> (When the MSI-X vectors needed are more than MSIX_MAX_ENTRIES defined in
>>>> qemu: msix.c, all the queues still share one MSI-X vector as before.)
>>>>
>>>> Signed-off-by: Hongyong Zang<zanghongyong@huawei.com>
>>>> ---
>>>>   hw/virtio-pci.c |    5 ++++-
>>>>   1 files changed, 4 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>>> index 77b75bc..2c9c6fb 100644
>>>> --- a/hw/virtio-pci.c
>>>> +++ b/hw/virtio-pci.c
>>>> @@ -718,8 +718,11 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
>>>>           return -1;
>>>>       }
>>>>       vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
>>>> -                                        ? proxy->serial.max_virtserial_ports + 1
>>>> +                                        ? (proxy->serial.max_virtserial_ports + 1) * 2
>>>>                                           : proxy->nvectors;
>>>> +    /*msix.c: #define MSIX_MAX_ENTRIES 32*/
>>>> +    if (vdev->nvectors>   32)
>>>> +        vdev->nvectors = 32;
>>> This change isn't needed: if the proxy->nvectors value exceeds the max
>>> allowed, virtio_init_pci() will end up using a shared vector instead
>>> of separate ones.
>>>
>> Hi Amit,
>> If the nvectors exceeds the max, msix_init() will return -EINVAL in QEMU,
>> and the front-end driver in Guest will use regular interrupt instead
>> of MSI-X.
> In that case, I believe msix_init() should be changed to attempt to
> share interrupts instead of drivers doing this by themselves.
>
> 		Amit
>
> .
>
Yup, if the vectors exceeds the max, msix_init() should try to share one 
vector or use the max vectors directly.

Or we may change the MSIX_MAX_ENTRIES defined in misx.c:
It allocs one page for the msix table(reserve second half of the page 
for pending bits), and one table entry only needs four DWORDs in PCI 
SPEC, so I think the max entries could be 128 instead of 32.