Patchwork VirtIO: Fix QEMU crash during Windows PNP tests

login
register
mail settings
Submitter Yan Vugenfirer
Date Sept. 14, 2009, 1:31 p.m.
Message ID <1252935086-8856-1-git-send-email-yvugenfi@redhat.com>
Download mbox | patch
Permalink /patch/33582/
State Superseded
Headers show

Comments

Yan Vugenfirer - Sept. 14, 2009, 1:31 p.m.
Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com>

---
 hw/virtio-pci.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)
Alexander Graf - March 1, 2010, 11:14 a.m.
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
Gleb Natapov - March 1, 2010, 11:26 a.m.
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.
Michael S. Tsirkin - March 2, 2010, 10:09 a.m.
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.
Alexander Graf - March 2, 2010, 11:03 a.m.
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
Michael S. Tsirkin - March 2, 2010, 11:05 a.m.
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
Alexander Graf - March 2, 2010, 11:43 a.m.
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

Patch

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