[Qemu-devel] [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

[Qemu-devel] [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

[Yan Vugenfirer]

[-- Attachment #1: Type: text/plain, Size: 316 bytes --]

Hello,

In some cases bus driver can deassert "bus master" bit in PCI command
register. The driver will no longer be able to update related registers in
the device. Eventually it will cause QEMU to exit in "virtqueue_num_heads"
function.

Attached path that fixes the described issue.

Best regards,
Yan Vugenfirer.

[-- Attachment #2: 0001-VirtIO-Fix-QEMU-crash-during-Windows-PNP-tests.patch --]
[-- Type: application/octet-stream, Size: 1789 bytes --]

>From 3fdafbdfad676ec8479dc073cff70bf356868bfe Mon Sep 17 00:00:00 2001
From: Yan Vugenfirer <yvugenfi@redhat.com>
Date: Tue, 8 Sep 2009 10:08:14 -0400
Subject: [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

---
 hw/virtio-pci.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index f812ab7..f7a51ff 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num,
 static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
                                 uint32_t val, int len)
 {
+    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
+
+    if (PCI_COMMAND == address) {
+        if (!(val & PCI_COMMAND_MASTER)) {
+            proxy->vdev->status &= !VIRTIO_CONFIG_S_DRIVER_OK;
+        }
+    }
+
     pci_default_write_config(pci_dev, address, val, len);
-    msix_write_config(pci_dev, address, val, len);
+    if(proxy->vdev->nvectors)
+        msix_write_config(pci_dev, address, val, len);
 }
 
 static const VirtIOBindings virtio_pci_bindings = {
@@ -407,11 +416,12 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
                          msix_bar_size(&proxy->pci_dev),
                          PCI_ADDRESS_SPACE_MEM,
                          msix_mmio_map);
-        proxy->pci_dev.config_write = virtio_write_config;
         proxy->pci_dev.unregister = msix_uninit;
     } else
         vdev->nvectors = 0;
 
+    proxy->pci_dev.config_write = virtio_write_config;
+
     size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) + vdev->config_len;
     if (size & (size-1))
         size = 1 << qemu_fls(size);
-- 
1.5.5.6

Re: [Qemu-devel] [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

[Anthony Liguori]

Yan Vugenfirer wrote:
> Hello,
>
> In some cases bus driver can deassert "bus master" bit in PCI command
> register. The driver will no longer be able to update related registers in
> the device. Eventually it will cause QEMU to exit in "virtqueue_num_heads"
> function.
>
> Attached path that fixes the described issue.
>
> Best regards,
> Yan Vugenfirer.
>   
Needs a Signed-off-by.

Regards,

Anthony Liguori

RE: [Qemu-devel] [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

[Yan Vugenfirer]

[-- Attachment #1: Type: text/plain, Size: 124 bytes --]

> From: Anthony Liguori
> 
> Needs a Signed-off-by.
> 
> Regards,
> 
> Anthony Liguori
 
[YV] Resending with signed-off-by.

[-- Attachment #2: 0001-VirtIO-Fix-QEMU-crash-during-Windows-PNP-tests.patch --]
[-- Type: application/octet-stream, Size: 1844 bytes --]

>From d7b9ccc5c1a6a28367a550a5e55a40d482623ec7 Mon Sep 17 00:00:00 2001
From: Yan Vugenfirer <yvugenfi@redhat.com>
Date: Tue, 8 Sep 2009 10:08:14 -0400
Subject: [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com>

---
 hw/virtio-pci.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index f812ab7..a0a22c4 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num,
 static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
                                 uint32_t val, int len)
 {
+    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
+
+    if (PCI_COMMAND == address) {
+        if (!(val & PCI_COMMAND_MASTER)) {
+            proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
+        }
+    }
+
     pci_default_write_config(pci_dev, address, val, len);
-    msix_write_config(pci_dev, address, val, len);
+    if(proxy->vdev->nvectors)
+        msix_write_config(pci_dev, address, val, len);
 }
 
 static const VirtIOBindings virtio_pci_bindings = {
@@ -407,11 +416,12 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
                          msix_bar_size(&proxy->pci_dev),
                          PCI_ADDRESS_SPACE_MEM,
                          msix_mmio_map);
-        proxy->pci_dev.config_write = virtio_write_config;
         proxy->pci_dev.unregister = msix_uninit;
     } else
         vdev->nvectors = 0;
 
+    proxy->pci_dev.config_write = virtio_write_config;
+
     size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) + vdev->config_len;
     if (size & (size-1))
         size = 1 << qemu_fls(size);
-- 
1.5.5.6

Re: [Qemu-devel] [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

[Anthony Liguori]

Yan Vugenfirer wrote:
>> From: Anthony Liguori
>>
>> Needs a Signed-off-by.
>>
>> Regards,
>>
>> Anthony Liguori
>>     
>  
> [YV] Resending with signed-off-by.
>   

Please send as a top level patch (and not as an attachment).

Regards,

Anthony Liguori

[Qemu-devel] [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

[Yan Vugenfirer]

Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com>

---
 hw/virtio-pci.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index f812ab7..a0a22c4 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num,
 static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
                                 uint32_t val, int len)
 {
+    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
+
+    if (PCI_COMMAND == address) {
+        if (!(val & PCI_COMMAND_MASTER)) {
+            proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
+        }
+    }
+
     pci_default_write_config(pci_dev, address, val, len);
-    msix_write_config(pci_dev, address, val, len);
+    if(proxy->vdev->nvectors)
+        msix_write_config(pci_dev, address, val, len);
 }
 
 static const VirtIOBindings virtio_pci_bindings = {
@@ -407,11 +416,12 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
                          msix_bar_size(&proxy->pci_dev),
                          PCI_ADDRESS_SPACE_MEM,
                          msix_mmio_map);
-        proxy->pci_dev.config_write = virtio_write_config;
         proxy->pci_dev.unregister = msix_uninit;
     } else
         vdev->nvectors = 0;
 
+    proxy->pci_dev.config_write = virtio_write_config;
+
     size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) + vdev->config_len;
     if (size & (size-1))
         size = 1 << qemu_fls(size);
-- 
1.5.5.6

Re: [Qemu-devel] [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

[Alexander Graf]

On 14.09.2009, at 15:31, Yan Vugenfirer wrote:

> Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com>
> 
> ---
> hw/virtio-pci.c |   14 ++++++++++++--
> 1 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index f812ab7..a0a22c4 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num,
> static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
>                                 uint32_t val, int len)
> {
> +    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> +
> +    if (PCI_COMMAND == address) {
> +        if (!(val & PCI_COMMAND_MASTER)) {
> +            proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;

This part breaks PCI hotplug with Linux guests.

What happens is the following:

(qemu) pci_add auto nic model=virtio,vlan=0
  - Virtio dev 1 -> write config (0)
  - Virtio dev 1 -> write config (0x3)
  - Virtio dev 1 -> set status explicitly to OK

(qemu) pci_add auto storage file=/tmp/image.raw,if=virtio
  - Virtio dev 1 -> write config (0x3)
  -> Unset DRIVER_OK bit
  -> network device becomes unresponsive 
  - Virtio dev 2 -> write config (0)
  - Virtio dev 2 -> write config (0x3)
  - Virtio dev 2 -> set status explicitly to OK


Please either revert this patch or provide a proper fix.

Alex

Re: [Qemu-devel] [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

[Gleb Natapov]

On Mon, Mar 01, 2010 at 12:14:43PM +0100, Alexander Graf wrote:
> 
> On 14.09.2009, at 15:31, Yan Vugenfirer wrote:
> 
> > Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com>
> > 
> > ---
> > hw/virtio-pci.c |   14 ++++++++++++--
> > 1 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > index f812ab7..a0a22c4 100644
> > --- a/hw/virtio-pci.c
> > +++ b/hw/virtio-pci.c
> > @@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num,
> > static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
> >                                 uint32_t val, int len)
> > {
> > +    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> > +
> > +    if (PCI_COMMAND == address) {
> > +        if (!(val & PCI_COMMAND_MASTER)) {
> > +            proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
> 
> This part breaks PCI hotplug with Linux guests.
> 
> What happens is the following:
Looks like something is broken even without this patch.

> 
> (qemu) pci_add auto nic model=virtio,vlan=0
>   - Virtio dev 1 -> write config (0)
>   - Virtio dev 1 -> write config (0x3)
>   - Virtio dev 1 -> set status explicitly to OK
Why Linux doesn't enable bus mastering on this device?

> 
> (qemu) pci_add auto storage file=/tmp/image.raw,if=virtio
>   - Virtio dev 1 -> write config (0x3)
Why Linux touches unrelated device's config space?

>   -> Unset DRIVER_OK bit
>   -> network device becomes unresponsive 
>   - Virtio dev 2 -> write config (0)
>   - Virtio dev 2 -> write config (0x3)
>   - Virtio dev 2 -> set status explicitly to OK
> 
> 
> Please either revert this patch or provide a proper fix.
> 
> Alex

--
			Gleb.

Re: [Qemu-devel] [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

[Michael S. Tsirkin]

On Mon, Mar 01, 2010 at 01:26:37PM +0200, Gleb Natapov wrote:
> On Mon, Mar 01, 2010 at 12:14:43PM +0100, Alexander Graf wrote:
> > 
> > On 14.09.2009, at 15:31, Yan Vugenfirer wrote:
> > 
> > > Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com>
> > > 
> > > ---
> > > hw/virtio-pci.c |   14 ++++++++++++--
> > > 1 files changed, 12 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > > index f812ab7..a0a22c4 100644
> > > --- a/hw/virtio-pci.c
> > > +++ b/hw/virtio-pci.c
> > > @@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num,
> > > static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
> > >                                 uint32_t val, int len)
> > > {
> > > +    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> > > +
> > > +    if (PCI_COMMAND == address) {
> > > +        if (!(val & PCI_COMMAND_MASTER)) {
> > > +            proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
> > 
> > This part breaks PCI hotplug with Linux guests.
> > 
> > What happens is the following:
> Looks like something is broken even without this patch.
> 
> > 
> > (qemu) pci_add auto nic model=virtio,vlan=0
> >   - Virtio dev 1 -> write config (0)
> >   - Virtio dev 1 -> write config (0x3)
> >   - Virtio dev 1 -> set status explicitly to OK
> Why Linux doesn't enable bus mastering on this device?

I fixed this, and Rusty said he has applied the patch,
but it seems the patch got lost later:

http://lkml.org/lkml/2009/11/29/73

Alexander, could you please check whether applying
this patch fixes hotplug for you?
If yes I will queue it up for 2.6.34.


> > 
> > (qemu) pci_add auto storage file=/tmp/image.raw,if=virtio
> >   - Virtio dev 1 -> write config (0x3)
> Why Linux touches unrelated device's config space?
> 
> >   -> Unset DRIVER_OK bit
> >   -> network device becomes unresponsive 
> >   - Virtio dev 2 -> write config (0)
> >   - Virtio dev 2 -> write config (0x3)
> >   - Virtio dev 2 -> set status explicitly to OK
> > 
> > 
> > Please either revert this patch or provide a proper fix.
> > 
> > Alex
> 
> --
> 			Gleb.

Re: [Qemu-devel] [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

[Alexander Graf]

On 02.03.2010, at 11:09, Michael S. Tsirkin wrote:

> On Mon, Mar 01, 2010 at 01:26:37PM +0200, Gleb Natapov wrote:
>> On Mon, Mar 01, 2010 at 12:14:43PM +0100, Alexander Graf wrote:
>>> 
>>> On 14.09.2009, at 15:31, Yan Vugenfirer wrote:
>>> 
>>>> Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com>
>>>> 
>>>> ---
>>>> hw/virtio-pci.c |   14 ++++++++++++--
>>>> 1 files changed, 12 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>>> index f812ab7..a0a22c4 100644
>>>> --- a/hw/virtio-pci.c
>>>> +++ b/hw/virtio-pci.c
>>>> @@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num,
>>>> static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
>>>>                                uint32_t val, int len)
>>>> {
>>>> +    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>>>> +
>>>> +    if (PCI_COMMAND == address) {
>>>> +        if (!(val & PCI_COMMAND_MASTER)) {
>>>> +            proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
>>> 
>>> This part breaks PCI hotplug with Linux guests.
>>> 
>>> What happens is the following:
>> Looks like something is broken even without this patch.
>> 
>>> 
>>> (qemu) pci_add auto nic model=virtio,vlan=0
>>>  - Virtio dev 1 -> write config (0)
>>>  - Virtio dev 1 -> write config (0x3)
>>>  - Virtio dev 1 -> set status explicitly to OK
>> Why Linux doesn't enable bus mastering on this device?
> 
> I fixed this, and Rusty said he has applied the patch,
> but it seems the patch got lost later:
> 
> http://lkml.org/lkml/2009/11/29/73
> 
> Alexander, could you please check whether applying
> this patch fixes hotplug for you?
> If yes I will queue it up for 2.6.34.

So we're looking at a guest bug?

It's still a qemu regression then. This configuration used to work with 0.10. There should at least be a feature bit from the guest saying "I'm good with bus mastering", so we can enable the check only for those.


Alex

Re: [Qemu-devel] [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

[Michael S. Tsirkin]

On Tue, Mar 02, 2010 at 12:03:15PM +0100, Alexander Graf wrote:
> 
> On 02.03.2010, at 11:09, Michael S. Tsirkin wrote:
> 
> > On Mon, Mar 01, 2010 at 01:26:37PM +0200, Gleb Natapov wrote:
> >> On Mon, Mar 01, 2010 at 12:14:43PM +0100, Alexander Graf wrote:
> >>> 
> >>> On 14.09.2009, at 15:31, Yan Vugenfirer wrote:
> >>> 
> >>>> Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com>
> >>>> 
> >>>> ---
> >>>> hw/virtio-pci.c |   14 ++++++++++++--
> >>>> 1 files changed, 12 insertions(+), 2 deletions(-)
> >>>> 
> >>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> >>>> index f812ab7..a0a22c4 100644
> >>>> --- a/hw/virtio-pci.c
> >>>> +++ b/hw/virtio-pci.c
> >>>> @@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num,
> >>>> static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
> >>>>                                uint32_t val, int len)
> >>>> {
> >>>> +    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> >>>> +
> >>>> +    if (PCI_COMMAND == address) {
> >>>> +        if (!(val & PCI_COMMAND_MASTER)) {
> >>>> +            proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
> >>> 
> >>> This part breaks PCI hotplug with Linux guests.
> >>> 
> >>> What happens is the following:
> >> Looks like something is broken even without this patch.
> >> 
> >>> 
> >>> (qemu) pci_add auto nic model=virtio,vlan=0
> >>>  - Virtio dev 1 -> write config (0)
> >>>  - Virtio dev 1 -> write config (0x3)
> >>>  - Virtio dev 1 -> set status explicitly to OK
> >> Why Linux doesn't enable bus mastering on this device?
> > 
> > I fixed this, and Rusty said he has applied the patch,
> > but it seems the patch got lost later:
> > 
> > http://lkml.org/lkml/2009/11/29/73
> > 
> > Alexander, could you please check whether applying
> > this patch fixes hotplug for you?
> > If yes I will queue it up for 2.6.34.
> 
> So we're looking at a guest bug?

Donnu. Does it work with the patch applied?

> It's still a qemu regression then. This configuration used to work
> with 0.10. There should at least be a feature bit from the guest
> saying "I'm good with bus mastering", so we can enable the check only
> for those.
> Alex

Re: [Qemu-devel] [PATCH] VirtIO: Fix QEMU crash during Windows PNP tests

[Alexander Graf]

On 02.03.2010, at 12:05, Michael S. Tsirkin wrote:

> On Tue, Mar 02, 2010 at 12:03:15PM +0100, Alexander Graf wrote:
>> 
>> On 02.03.2010, at 11:09, Michael S. Tsirkin wrote:
>> 
>>> On Mon, Mar 01, 2010 at 01:26:37PM +0200, Gleb Natapov wrote:
>>>> On Mon, Mar 01, 2010 at 12:14:43PM +0100, Alexander Graf wrote:
>>>>> 
>>>>> On 14.09.2009, at 15:31, Yan Vugenfirer wrote:
>>>>> 
>>>>>> Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com>
>>>>>> 
>>>>>> ---
>>>>>> hw/virtio-pci.c |   14 ++++++++++++--
>>>>>> 1 files changed, 12 insertions(+), 2 deletions(-)
>>>>>> 
>>>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>>>>> index f812ab7..a0a22c4 100644
>>>>>> --- a/hw/virtio-pci.c
>>>>>> +++ b/hw/virtio-pci.c
>>>>>> @@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num,
>>>>>> static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
>>>>>>                               uint32_t val, int len)
>>>>>> {
>>>>>> +    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>>>>>> +
>>>>>> +    if (PCI_COMMAND == address) {
>>>>>> +        if (!(val & PCI_COMMAND_MASTER)) {
>>>>>> +            proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
>>>>> 
>>>>> This part breaks PCI hotplug with Linux guests.
>>>>> 
>>>>> What happens is the following:
>>>> Looks like something is broken even without this patch.
>>>> 
>>>>> 
>>>>> (qemu) pci_add auto nic model=virtio,vlan=0
>>>>> - Virtio dev 1 -> write config (0)
>>>>> - Virtio dev 1 -> write config (0x3)
>>>>> - Virtio dev 1 -> set status explicitly to OK
>>>> Why Linux doesn't enable bus mastering on this device?
>>> 
>>> I fixed this, and Rusty said he has applied the patch,
>>> but it seems the patch got lost later:
>>> 
>>> http://lkml.org/lkml/2009/11/29/73
>>> 
>>> Alexander, could you please check whether applying
>>> this patch fixes hotplug for you?
>>> If yes I will queue it up for 2.6.34.
>> 
>> So we're looking at a guest bug?
> 
> Donnu. Does it work with the patch applied?

Yes, it works with the patch applied. It's still missing a capability though :-).


Alex