| Message ID | 1323998066-2396-1-git-send-email-zanghongyong@huawei.com |
|---|---|
| State | New |
| Headers | show |
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
于 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
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
于 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.
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;