[Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Michael S. Tsirkin]

commit cc943c36faa192cd4b32af8fe5edb31894017d35
    pci: Use bus master address space for delivering MSI/MSI-X messages
breaks virtio-net for rhel6.[56] x86 guests because they don't
enable bus mastering for virtio PCI devices

Old guests forgot to enable bus mastering, enable it
automatically on DRIVER_OK.

Note: we should either back out the original patch from
stable or apply this one on top.

Cc: qemu-stable@nongnu.org
Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio-pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ddb5da1..af937d2 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
             !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
             proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
+            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
+                                      true);
         }
         break;
     case VIRTIO_MSI_CONFIG_VECTOR:
-- 
MST

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Jan Kiszka]

On 2014-09-08 18:05, Michael S. Tsirkin wrote:
> commit cc943c36faa192cd4b32af8fe5edb31894017d35
>     pci: Use bus master address space for delivering MSI/MSI-X messages
> breaks virtio-net for rhel6.[56] x86 guests because they don't
> enable bus mastering for virtio PCI devices
> 
> Old guests forgot to enable bus mastering, enable it
> automatically on DRIVER_OK.
> 
> Note: we should either back out the original patch from
> stable or apply this one on top.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

I didn't signed off as I didn't write this patch ;). But you can add my
reviewed-by if you like.

Jan

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio/virtio-pci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ddb5da1..af937d2 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> +                                      true);
>          }
>          break;
>      case VIRTIO_MSI_CONFIG_VECTOR:
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Michael S. Tsirkin]

On Mon, Sep 08, 2014 at 06:26:51PM +0200, Jan Kiszka wrote:
> On 2014-09-08 18:05, Michael S. Tsirkin wrote:
> > commit cc943c36faa192cd4b32af8fe5edb31894017d35
> >     pci: Use bus master address space for delivering MSI/MSI-X messages
> > breaks virtio-net for rhel6.[56] x86 guests because they don't
> > enable bus mastering for virtio PCI devices
> > 
> > Old guests forgot to enable bus mastering, enable it
> > automatically on DRIVER_OK.
> > 
> > Note: we should either back out the original patch from
> > stable or apply this one on top.
> > 
> > Cc: qemu-stable@nongnu.org
> > Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> I didn't signed off as I didn't write this patch ;). But you can add my
> reviewed-by if you like.
> 
> Jan

Should have been Cc :)

> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/virtio/virtio-pci.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index ddb5da1..af937d2 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> >              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> >              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> > +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > +                                      true);
> >          }
> >          break;
> >      case VIRTIO_MSI_CONFIG_VECTOR:
> > 
> 
> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SES-DE
> Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Michael Roth]

Quoting Michael S. Tsirkin (2014-09-08 11:05:02)
> commit cc943c36faa192cd4b32af8fe5edb31894017d35
>     pci: Use bus master address space for delivering MSI/MSI-X messages
> breaks virtio-net for rhel6.[56] x86 guests because they don't
> enable bus mastering for virtio PCI devices
> 
> Old guests forgot to enable bus mastering, enable it
> automatically on DRIVER_OK.
> 
> Note: we should either back out the original patch from
> stable or apply this one on top.

I've already backed out the other patch, and it looks we'll have to stick
with that for now with the release being tomorrow. Will queue these back
up for 2.1.2 though.

> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio/virtio-pci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ddb5da1..af937d2 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> +                                      true);
>          }
>          break;
>      case VIRTIO_MSI_CONFIG_VECTOR:
> -- 
> MST

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Michael S. Tsirkin]

On Tue, Sep 09, 2014 at 09:13:38AM -0500, Michael Roth wrote:
> Quoting Michael S. Tsirkin (2014-09-08 11:05:02)
> > commit cc943c36faa192cd4b32af8fe5edb31894017d35
> >     pci: Use bus master address space for delivering MSI/MSI-X messages
> > breaks virtio-net for rhel6.[56] x86 guests because they don't
> > enable bus mastering for virtio PCI devices
> > 
> > Old guests forgot to enable bus mastering, enable it
> > automatically on DRIVER_OK.
> > 
> > Note: we should either back out the original patch from
> > stable or apply this one on top.
> 
> I've already backed out the other patch, and it looks we'll have to stick
> with that for now with the release being tomorrow.

That's fine.

> Will queue these back
> up for 2.1.2 though.
> > 
> > Cc: qemu-stable@nongnu.org
> > Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/virtio/virtio-pci.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index ddb5da1..af937d2 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> >              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> >              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> > +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > +                                      true);
> >          }
> >          break;
> >      case VIRTIO_MSI_CONFIG_VECTOR:
> > -- 
> > MST

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Greg Kurz]

On Mon, 8 Sep 2014 19:05:02 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> commit cc943c36faa192cd4b32af8fe5edb31894017d35
>     pci: Use bus master address space for delivering MSI/MSI-X messages
> breaks virtio-net for rhel6.[56] x86 guests because they don't
> enable bus mastering for virtio PCI devices
> 
> Old guests forgot to enable bus mastering, enable it
> automatically on DRIVER_OK.
> 
> Note: we should either back out the original patch from
> stable or apply this one on top.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio/virtio-pci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ddb5da1..af937d2 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> +                                      true);
>          }
>          break;
>      case VIRTIO_MSI_CONFIG_VECTOR:

Cc'ing Alexey for some SLOF and early boot of the ppc64 kernel expertise.

Michael,

This was enough to fix virtio-net in the rhel6.5 x86 guest case. Unfortunately,
this fails for rhel6.5 ppc64 because it is never called... I did some debugging:
it looks like the guest kernel calls the OF quisece call to flush pending DMA
and disables bus master on the virtio-blk device (PCI_COMMAND == 0x3). The
guest then continues to boot and hangs... It appears that waiting for the
guest to issue VIRTIO_CONFIG_S_DRIVER_OK is not enough. Since we need this for
MSI to work, I tried the following and it fixes the issue:

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index af937d2..3d72aa8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -111,9 +111,14 @@ static void virtio_pci_notify(DeviceState *d, uint16_t vector)
 {
     VirtIOPCIProxy *proxy = to_virtio_pci_proxy_fast(d);
 
-    if (msix_enabled(&proxy->pci_dev))
+    if (msix_enabled(&proxy->pci_dev)) {
+        if (!(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
+            proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
+            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
+                                      true);
+        }
         msix_notify(&proxy->pci_dev, vector);
-    else {
+    } else {
         VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
         pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
     }

If this is acceptable, I'll make it a helper and squash it into your patch.

Thoughts ?

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Alexey Kardashevskiy]

On 09/10/2014 08:35 AM, Greg Kurz wrote:
> On Mon, 8 Sep 2014 19:05:02 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>> commit cc943c36faa192cd4b32af8fe5edb31894017d35
>>     pci: Use bus master address space for delivering MSI/MSI-X messages
>> breaks virtio-net for rhel6.[56] x86 guests because they don't
>> enable bus mastering for virtio PCI devices
>>
>> Old guests forgot to enable bus mastering, enable it
>> automatically on DRIVER_OK.
>>
>> Note: we should either back out the original patch from
>> stable or apply this one on top.
>>
>> Cc: qemu-stable@nongnu.org
>> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  hw/virtio/virtio-pci.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index ddb5da1..af937d2 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
>>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
>> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
>> +                                      true);
>>          }
>>          break;
>>      case VIRTIO_MSI_CONFIG_VECTOR:
> 
> Cc'ing Alexey for some SLOF and early boot of the ppc64 kernel expertise.

Cc'ing Nikunj - he is SLOF expert.
I cannot comment - I have no idea what VIRTIO_PCI_FLAG_BUS_MASTER_BUG does.

> 
> Michael,
> 
> This was enough to fix virtio-net in the rhel6.5 x86 guest case. Unfortunately,
> this fails for rhel6.5 ppc64 because it is never called... I did some debugging:
> it looks like the guest kernel calls the OF quisece call to flush pending DMA
> and disables bus master on the virtio-blk device (PCI_COMMAND == 0x3). The
> guest then continues to boot and hangs... It appears that waiting for the
> guest to issue VIRTIO_CONFIG_S_DRIVER_OK is not enough. Since we need this for
> MSI to work, I tried the following and it fixes the issue:
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index af937d2..3d72aa8 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -111,9 +111,14 @@ static void virtio_pci_notify(DeviceState *d, uint16_t vector)
>  {
>      VirtIOPCIProxy *proxy = to_virtio_pci_proxy_fast(d);
>  
> -    if (msix_enabled(&proxy->pci_dev))
> +    if (msix_enabled(&proxy->pci_dev)) {
> +        if (!(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> +            proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> +                                      true);
> +        }
>          msix_notify(&proxy->pci_dev, vector);
> -    else {
> +    } else {
>          VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>          pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
>      }
> 
> If this is acceptable, I'll make it a helper and squash it into your patch.
> 
> Thoughts ?
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Nikunj A Dadhania]

Greg Kurz <gkurz@linux.vnet.ibm.com> writes:

> On Mon, 8 Sep 2014 19:05:02 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> commit cc943c36faa192cd4b32af8fe5edb31894017d35
>>     pci: Use bus master address space for delivering MSI/MSI-X messages
>> breaks virtio-net for rhel6.[56] x86 guests because they don't
>> enable bus mastering for virtio PCI devices
>> 
>> Old guests forgot to enable bus mastering, enable it
>> automatically on DRIVER_OK.
>> 
>> Note: we should either back out the original patch from
>> stable or apply this one on top.
>> 
>> Cc: qemu-stable@nongnu.org
>> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  hw/virtio/virtio-pci.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index ddb5da1..af937d2 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
>>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
>> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
>> +                                      true);
>>          }
>>          break;
>>      case VIRTIO_MSI_CONFIG_VECTOR:
>
> Cc'ing Alexey for some SLOF and early boot of the ppc64 kernel expertise.
>
> Michael,
>
> This was enough to fix virtio-net in the rhel6.5 x86 guest case. Unfortunately,
> this fails for rhel6.5 ppc64 because it is never called... 

> I did some debugging: it looks like the guest kernel calls the OF
> quisece call to flush pending DMA and disables bus master on the
> virtio-blk device (PCI_COMMAND == 0x3).

Getting confused, above you are talking about virtio-net and here it is
virtio-blk.

Anyways, the routines still remains same for both of them.  From SLOF
during init we set DRIVER_OK, and after using the device during the
quiesce, called from linux kernel VIRTIO_CONFIG_S_FAILED is set and then
a VIRTIO_DEVICE_RESET is done.

> The guest then continues to boot and hangs... It appears that waiting
> for the guest to issue VIRTIO_CONFIG_S_DRIVER_OK is not enough. Since
> we need this for MSI to work, I tried the following and it fixes the
> issue:
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index af937d2..3d72aa8 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -111,9 +111,14 @@ static void virtio_pci_notify(DeviceState *d, uint16_t vector)
>  {
>      VirtIOPCIProxy *proxy = to_virtio_pci_proxy_fast(d);
>  
> -    if (msix_enabled(&proxy->pci_dev))
> +    if (msix_enabled(&proxy->pci_dev)) {
> +        if (!(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> +            proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> +                                      true);
> +        }
>          msix_notify(&proxy->pci_dev, vector);
> -    else {
> +    } else {
>          VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>          pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
>      }
>
> If this is acceptable, I'll make it a helper and squash it into your patch.
>
> Thoughts ?
>
> -- 
> Gregory Kurz                                     kurzgreg@fr.ibm.com
>                                                  gkurz@linux.vnet.ibm.com
> Software Engineer @ IBM/Meiosys                  http://www.ibm.com
> Tel +33 (0)562 165 496
>
> "Anarchy is about taking complete responsibility for yourself."
>         Alan Moore.

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Greg Kurz]

On Wed, 10 Sep 2014 13:44:49 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
> Greg Kurz <gkurz@linux.vnet.ibm.com> writes:
> 
> > On Mon, 8 Sep 2014 19:05:02 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >> commit cc943c36faa192cd4b32af8fe5edb31894017d35
> >>     pci: Use bus master address space for delivering MSI/MSI-X messages
> >> breaks virtio-net for rhel6.[56] x86 guests because they don't
> >> enable bus mastering for virtio PCI devices
> >> 
> >> Old guests forgot to enable bus mastering, enable it
> >> automatically on DRIVER_OK.
> >> 
> >> Note: we should either back out the original patch from
> >> stable or apply this one on top.
> >> 
> >> Cc: qemu-stable@nongnu.org
> >> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> ---
> >>  hw/virtio/virtio-pci.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> index ddb5da1..af937d2 100644
> >> --- a/hw/virtio/virtio-pci.c
> >> +++ b/hw/virtio/virtio-pci.c
> >> @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> >>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> >>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> >> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> >> +                                      true);
> >>          }
> >>          break;
> >>      case VIRTIO_MSI_CONFIG_VECTOR:
> >
> > Cc'ing Alexey for some SLOF and early boot of the ppc64 kernel expertise.
> >
> > Michael,
> >
> > This was enough to fix virtio-net in the rhel6.5 x86 guest case. Unfortunately,
> > this fails for rhel6.5 ppc64 because it is never called... 
> 
> > I did some debugging: it looks like the guest kernel calls the OF
> > quisece call to flush pending DMA and disables bus master on the
> > virtio-blk device (PCI_COMMAND == 0x3).
> 
> Getting confused, above you are talking about virtio-net and here it is
> virtio-blk.
> 

I tried running rhel6.5 (old kernel that doesn't enable bus mastering on
virtio PCI devices), with a virtio-blk based disk and a virtio-net based
NIC for both x86_64 and ppc64. Results are as follow:
- x86_64: boots well but fails to activate network
- ppc64: does not boot because the virtio-blk notification doesn't
         reach the guest

> Anyways, the routines still remains same for both of them.  From SLOF
> during init we set DRIVER_OK, and after using the device during the
> quiesce, called from linux kernel VIRTIO_CONFIG_S_FAILED is set and then
> a VIRTIO_DEVICE_RESET is done.
> 

I chose to debug by attaching gdb to qemu-system-ppc64 itself. It appears
that SLOF seems to be enabling bus master during init but at some point bus
master gets disabled... unfortunately my SLOF knowledge is limited and I
don't know how exactly what's happening in the guest between SLOF and the
kernel.

> > The guest then continues to boot and hangs... It appears that waiting
> > for the guest to issue VIRTIO_CONFIG_S_DRIVER_OK is not enough. Since
> > we need this for MSI to work, I tried the following and it fixes the
> > issue:
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index af937d2..3d72aa8 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -111,9 +111,14 @@ static void virtio_pci_notify(DeviceState *d, uint16_t vector)
> >  {
> >      VirtIOPCIProxy *proxy = to_virtio_pci_proxy_fast(d);
> >  
> > -    if (msix_enabled(&proxy->pci_dev))
> > +    if (msix_enabled(&proxy->pci_dev)) {
> > +        if (!(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> > +            proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> > +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > +                                      true);
> > +        }
> >          msix_notify(&proxy->pci_dev, vector);
> > -    else {
> > +    } else {
> >          VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> >          pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
> >      }
> >
> > If this is acceptable, I'll make it a helper and squash it into your patch.
> >
> > Thoughts ?
> >
> > -- 
> > Gregory Kurz                                     kurzgreg@fr.ibm.com
> >                                                  gkurz@linux.vnet.ibm.com
> > Software Engineer @ IBM/Meiosys                  http://www.ibm.com
> > Tel +33 (0)562 165 496
> >
> > "Anarchy is about taking complete responsibility for yourself."
> >         Alan Moore.



-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Greg Kurz]

On Wed, 10 Sep 2014 12:32:30 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Sep 10, 2014 at 01:44:49PM +0530, Nikunj A Dadhania wrote:
> > Greg Kurz <gkurz@linux.vnet.ibm.com> writes:
> > 
> > > On Mon, 8 Sep 2014 19:05:02 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > >> commit cc943c36faa192cd4b32af8fe5edb31894017d35
> > >>     pci: Use bus master address space for delivering MSI/MSI-X messages
> > >> breaks virtio-net for rhel6.[56] x86 guests because they don't
> > >> enable bus mastering for virtio PCI devices
> > >> 
> > >> Old guests forgot to enable bus mastering, enable it
> > >> automatically on DRIVER_OK.
> > >> 
> > >> Note: we should either back out the original patch from
> > >> stable or apply this one on top.
> > >> 
> > >> Cc: qemu-stable@nongnu.org
> > >> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >> ---
> > >>  hw/virtio/virtio-pci.c | 2 ++
> > >>  1 file changed, 2 insertions(+)
> > >> 
> > >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > >> index ddb5da1..af937d2 100644
> > >> --- a/hw/virtio/virtio-pci.c
> > >> +++ b/hw/virtio/virtio-pci.c
> > >> @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> > >>          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > >>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> > >>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> > >> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > >> +                                      true);
> > >>          }
> > >>          break;
> > >>      case VIRTIO_MSI_CONFIG_VECTOR:
> > >
> > > Cc'ing Alexey for some SLOF and early boot of the ppc64 kernel expertise.
> > >
> > > Michael,
> > >
> > > This was enough to fix virtio-net in the rhel6.5 x86 guest case. Unfortunately,
> > > this fails for rhel6.5 ppc64 because it is never called... 
> > 
> > > I did some debugging: it looks like the guest kernel calls the OF
> > > quisece call to flush pending DMA and disables bus master on the
> > > virtio-blk device (PCI_COMMAND == 0x3).
> > 
> > Getting confused, above you are talking about virtio-net and here it is
> > virtio-blk.
> > 
> > Anyways, the routines still remains same for both of them.  From SLOF
> > during init we set DRIVER_OK, and after using the device during the
> > quiesce, called from linux kernel VIRTIO_CONFIG_S_FAILED is set and then
> > a VIRTIO_DEVICE_RESET is done.
> 
> BTW, you really should start enabling bus mastering, avoid relying
> on the work-around we have for broken guests.
> 

FWIW during my debug session, I see that SLOF enables bus mastering...
unfortunately, it gets disabled at some point after the guest kernel
is started (around the ppc64 prom_init() call).

> > > The guest then continues to boot and hangs... It appears that waiting
> > > for the guest to issue VIRTIO_CONFIG_S_DRIVER_OK is not enough. Since
> > > we need this for MSI to work, I tried the following and it fixes the
> > > issue:
> > >
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index af937d2..3d72aa8 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -111,9 +111,14 @@ static void virtio_pci_notify(DeviceState *d, uint16_t vector)
> > >  {
> > >      VirtIOPCIProxy *proxy = to_virtio_pci_proxy_fast(d);
> > >  
> > > -    if (msix_enabled(&proxy->pci_dev))
> > > +    if (msix_enabled(&proxy->pci_dev)) {
> > > +        if (!(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> > > +            proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> > > +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > > +                                      true);
> > > +        }
> > >          msix_notify(&proxy->pci_dev, vector);
> > > -    else {
> > > +    } else {
> > >          VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> > >          pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
> > >      }
> > >
> > > If this is acceptable, I'll make it a helper and squash it into your patch.
> > >
> > > Thoughts ?
> > >
> > > -- 
> > > Gregory Kurz                                     kurzgreg@fr.ibm.com
> > >                                                  gkurz@linux.vnet.ibm.com
> > > Software Engineer @ IBM/Meiosys                  http://www.ibm.com
> > > Tel +33 (0)562 165 496
> > >
> > > "Anarchy is about taking complete responsibility for yourself."
> > >         Alan Moore.
> 



-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Nikunj A Dadhania]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Sep 10, 2014 at 01:44:49PM +0530, Nikunj A Dadhania wrote:
>> Greg Kurz <gkurz@linux.vnet.ibm.com> writes:
>> 
>> > On Mon, 8 Sep 2014 19:05:02 +0300
>> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >
>> >> commit cc943c36faa192cd4b32af8fe5edb31894017d35
>> >>     pci: Use bus master address space for delivering MSI/MSI-X messages
>> >> breaks virtio-net for rhel6.[56] x86 guests because they don't
>> >> enable bus mastering for virtio PCI devices
>> >> 
>> >> Old guests forgot to enable bus mastering, enable it
>> >> automatically on DRIVER_OK.
>> >> 
>> >> Note: we should either back out the original patch from
>> >> stable or apply this one on top.
>> >> 
>> >> Cc: qemu-stable@nongnu.org
>> >> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> >> ---
>> >>  hw/virtio/virtio-pci.c | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >> 
>> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> >> index ddb5da1..af937d2 100644
>> >> --- a/hw/virtio/virtio-pci.c
>> >> +++ b/hw/virtio/virtio-pci.c
>> >> @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>> >>          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>> >>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
>> >>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
>> >> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
>> >> +                                      true);
>> >>          }
>> >>          break;
>> >>      case VIRTIO_MSI_CONFIG_VECTOR:
>> >
>> > Cc'ing Alexey for some SLOF and early boot of the ppc64 kernel expertise.
>> >
>> > Michael,
>> >
>> > This was enough to fix virtio-net in the rhel6.5 x86 guest case. Unfortunately,
>> > this fails for rhel6.5 ppc64 because it is never called... 
>> 
>> > I did some debugging: it looks like the guest kernel calls the OF
>> > quisece call to flush pending DMA and disables bus master on the
>> > virtio-blk device (PCI_COMMAND == 0x3).
>> 
>> Getting confused, above you are talking about virtio-net and here it is
>> virtio-blk.
>> 
>> Anyways, the routines still remains same for both of them.  From SLOF
>> during init we set DRIVER_OK, and after using the device during the
>> quiesce, called from linux kernel VIRTIO_CONFIG_S_FAILED is set and then
>> a VIRTIO_DEVICE_RESET is done.
>
> BTW, you really should start enabling bus mastering, avoid relying
> on the work-around we have for broken guests.

In SLOF, we do enable PCI MASTER during device scanning and then later
disable it.

Regards
Nikunj

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Nikunj A Dadhania]

Greg Kurz <gkurz@linux.vnet.ibm.com> writes:

>> > > I did some debugging: it looks like the guest kernel calls the OF
>> > > quisece call to flush pending DMA and disables bus master on the
>> > > virtio-blk device (PCI_COMMAND == 0x3).
>> > 
>> > Getting confused, above you are talking about virtio-net and here it is
>> > virtio-blk.
>> > 
>> > Anyways, the routines still remains same for both of them.  From SLOF
>> > during init we set DRIVER_OK, and after using the device during the
>> > quiesce, called from linux kernel VIRTIO_CONFIG_S_FAILED is set and then
>> > a VIRTIO_DEVICE_RESET is done.
>> 
>> BTW, you really should start enabling bus mastering, avoid relying
>> on the work-around we have for broken guests.
>> 
>
> FWIW during my debug session, I see that SLOF enables bus mastering...
> unfortunately, it gets disabled at some point after the guest kernel
> is started (around the ppc64 prom_init() call).

Is it before quiesce call?

Regards
Nikunj

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Michael S. Tsirkin]

On Wed, Sep 10, 2014 at 12:35:32AM +0200, Greg Kurz wrote:
> On Mon, 8 Sep 2014 19:05:02 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > commit cc943c36faa192cd4b32af8fe5edb31894017d35
> >     pci: Use bus master address space for delivering MSI/MSI-X messages
> > breaks virtio-net for rhel6.[56] x86 guests because they don't
> > enable bus mastering for virtio PCI devices
> > 
> > Old guests forgot to enable bus mastering, enable it
> > automatically on DRIVER_OK.
> > 
> > Note: we should either back out the original patch from
> > stable or apply this one on top.
> > 
> > Cc: qemu-stable@nongnu.org
> > Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/virtio/virtio-pci.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index ddb5da1..af937d2 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> >              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> >              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> > +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > +                                      true);
> >          }
> >          break;
> >      case VIRTIO_MSI_CONFIG_VECTOR:
> 
> Cc'ing Alexey for some SLOF and early boot of the ppc64 kernel expertise.
> 
> Michael,
> 
> This was enough to fix virtio-net in the rhel6.5 x86 guest case. Unfortunately,
> this fails for rhel6.5 ppc64 because it is never called... I did some debugging:
> it looks like the guest kernel calls the OF quisece call to flush pending DMA
> and disables bus master on the virtio-blk device (PCI_COMMAND == 0x3). The
> guest then continues to boot and hangs... It appears that waiting for the
> guest to issue VIRTIO_CONFIG_S_DRIVER_OK is not enough.

Got it. Writing to PCI_COMMAND disabled bus mastering again.
This should do it, on top - can you confirm please?

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index af937d2..6b7ac39 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -475,10 +475,14 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
     pci_default_write_config(pci_dev, address, val, len);
 
     if (range_covers_byte(address, len, PCI_COMMAND) &&
-        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER) &&
-        !(proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG)) {
-        virtio_pci_stop_ioeventfd(proxy);
-        virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
+        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
+        if (proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG) {
+            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
+                                      true);
+        } else {
+            virtio_pci_stop_ioeventfd(proxy);
+            virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
+        }
     }
 }
 

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Michael S. Tsirkin]

On Wed, Sep 10, 2014 at 01:44:49PM +0530, Nikunj A Dadhania wrote:
> Greg Kurz <gkurz@linux.vnet.ibm.com> writes:
> 
> > On Mon, 8 Sep 2014 19:05:02 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >> commit cc943c36faa192cd4b32af8fe5edb31894017d35
> >>     pci: Use bus master address space for delivering MSI/MSI-X messages
> >> breaks virtio-net for rhel6.[56] x86 guests because they don't
> >> enable bus mastering for virtio PCI devices
> >> 
> >> Old guests forgot to enable bus mastering, enable it
> >> automatically on DRIVER_OK.
> >> 
> >> Note: we should either back out the original patch from
> >> stable or apply this one on top.
> >> 
> >> Cc: qemu-stable@nongnu.org
> >> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> ---
> >>  hw/virtio/virtio-pci.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> index ddb5da1..af937d2 100644
> >> --- a/hw/virtio/virtio-pci.c
> >> +++ b/hw/virtio/virtio-pci.c
> >> @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> >>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> >>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> >> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> >> +                                      true);
> >>          }
> >>          break;
> >>      case VIRTIO_MSI_CONFIG_VECTOR:
> >
> > Cc'ing Alexey for some SLOF and early boot of the ppc64 kernel expertise.
> >
> > Michael,
> >
> > This was enough to fix virtio-net in the rhel6.5 x86 guest case. Unfortunately,
> > this fails for rhel6.5 ppc64 because it is never called... 
> 
> > I did some debugging: it looks like the guest kernel calls the OF
> > quisece call to flush pending DMA and disables bus master on the
> > virtio-blk device (PCI_COMMAND == 0x3).
> 
> Getting confused, above you are talking about virtio-net and here it is
> virtio-blk.
> 
> Anyways, the routines still remains same for both of them.  From SLOF
> during init we set DRIVER_OK, and after using the device during the
> quiesce, called from linux kernel VIRTIO_CONFIG_S_FAILED is set and then
> a VIRTIO_DEVICE_RESET is done.

BTW, you really should start enabling bus mastering, avoid relying
on the work-around we have for broken guests.

> > The guest then continues to boot and hangs... It appears that waiting
> > for the guest to issue VIRTIO_CONFIG_S_DRIVER_OK is not enough. Since
> > we need this for MSI to work, I tried the following and it fixes the
> > issue:
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index af937d2..3d72aa8 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -111,9 +111,14 @@ static void virtio_pci_notify(DeviceState *d, uint16_t vector)
> >  {
> >      VirtIOPCIProxy *proxy = to_virtio_pci_proxy_fast(d);
> >  
> > -    if (msix_enabled(&proxy->pci_dev))
> > +    if (msix_enabled(&proxy->pci_dev)) {
> > +        if (!(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> > +            proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> > +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > +                                      true);
> > +        }
> >          msix_notify(&proxy->pci_dev, vector);
> > -    else {
> > +    } else {
> >          VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> >          pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
> >      }
> >
> > If this is acceptable, I'll make it a helper and squash it into your patch.
> >
> > Thoughts ?
> >
> > -- 
> > Gregory Kurz                                     kurzgreg@fr.ibm.com
> >                                                  gkurz@linux.vnet.ibm.com
> > Software Engineer @ IBM/Meiosys                  http://www.ibm.com
> > Tel +33 (0)562 165 496
> >
> > "Anarchy is about taking complete responsibility for yourself."
> >         Alan Moore.

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Michael S. Tsirkin]

On Wed, Sep 10, 2014 at 02:45:51PM +0530, Nikunj A Dadhania wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, Sep 10, 2014 at 01:44:49PM +0530, Nikunj A Dadhania wrote:
> >> Greg Kurz <gkurz@linux.vnet.ibm.com> writes:
> >> 
> >> > On Mon, 8 Sep 2014 19:05:02 +0300
> >> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >
> >> >> commit cc943c36faa192cd4b32af8fe5edb31894017d35
> >> >>     pci: Use bus master address space for delivering MSI/MSI-X messages
> >> >> breaks virtio-net for rhel6.[56] x86 guests because they don't
> >> >> enable bus mastering for virtio PCI devices
> >> >> 
> >> >> Old guests forgot to enable bus mastering, enable it
> >> >> automatically on DRIVER_OK.
> >> >> 
> >> >> Note: we should either back out the original patch from
> >> >> stable or apply this one on top.
> >> >> 
> >> >> Cc: qemu-stable@nongnu.org
> >> >> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> >> ---
> >> >>  hw/virtio/virtio-pci.c | 2 ++
> >> >>  1 file changed, 2 insertions(+)
> >> >> 
> >> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> >> index ddb5da1..af937d2 100644
> >> >> --- a/hw/virtio/virtio-pci.c
> >> >> +++ b/hw/virtio/virtio-pci.c
> >> >> @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >> >>          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> >> >>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> >> >>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> >> >> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> >> >> +                                      true);
> >> >>          }
> >> >>          break;
> >> >>      case VIRTIO_MSI_CONFIG_VECTOR:
> >> >
> >> > Cc'ing Alexey for some SLOF and early boot of the ppc64 kernel expertise.
> >> >
> >> > Michael,
> >> >
> >> > This was enough to fix virtio-net in the rhel6.5 x86 guest case. Unfortunately,
> >> > this fails for rhel6.5 ppc64 because it is never called... 
> >> 
> >> > I did some debugging: it looks like the guest kernel calls the OF
> >> > quisece call to flush pending DMA and disables bus master on the
> >> > virtio-blk device (PCI_COMMAND == 0x3).
> >> 
> >> Getting confused, above you are talking about virtio-net and here it is
> >> virtio-blk.
> >> 
> >> Anyways, the routines still remains same for both of them.  From SLOF
> >> during init we set DRIVER_OK, and after using the device during the
> >> quiesce, called from linux kernel VIRTIO_CONFIG_S_FAILED is set and then
> >> a VIRTIO_DEVICE_RESET is done.
> >
> > BTW, you really should start enabling bus mastering, avoid relying
> > on the work-around we have for broken guests.
> 
> In SLOF, we do enable PCI MASTER during device scanning and then later
> disable it.
> 
> Regards
> Nikunj

But device is then reset, right Greg?
You get as far as reset?

If yes I doubt something that happens before reset
matters, unless we are leaking some state
across reset which would be a bug in itself.

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Michael S. Tsirkin]

On Wed, Sep 10, 2014 at 11:01:48AM +0200, Greg Kurz wrote:
> On Wed, 10 Sep 2014 12:32:30 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Sep 10, 2014 at 01:44:49PM +0530, Nikunj A Dadhania wrote:
> > > Greg Kurz <gkurz@linux.vnet.ibm.com> writes:
> > > 
> > > > On Mon, 8 Sep 2014 19:05:02 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > >> commit cc943c36faa192cd4b32af8fe5edb31894017d35
> > > >>     pci: Use bus master address space for delivering MSI/MSI-X messages
> > > >> breaks virtio-net for rhel6.[56] x86 guests because they don't
> > > >> enable bus mastering for virtio PCI devices
> > > >> 
> > > >> Old guests forgot to enable bus mastering, enable it
> > > >> automatically on DRIVER_OK.
> > > >> 
> > > >> Note: we should either back out the original patch from
> > > >> stable or apply this one on top.
> > > >> 
> > > >> Cc: qemu-stable@nongnu.org
> > > >> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >> ---
> > > >>  hw/virtio/virtio-pci.c | 2 ++
> > > >>  1 file changed, 2 insertions(+)
> > > >> 
> > > >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > >> index ddb5da1..af937d2 100644
> > > >> --- a/hw/virtio/virtio-pci.c
> > > >> +++ b/hw/virtio/virtio-pci.c
> > > >> @@ -320,6 +320,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> > > >>          if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > > >>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> > > >>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> > > >> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > > >> +                                      true);
> > > >>          }
> > > >>          break;
> > > >>      case VIRTIO_MSI_CONFIG_VECTOR:
> > > >
> > > > Cc'ing Alexey for some SLOF and early boot of the ppc64 kernel expertise.
> > > >
> > > > Michael,
> > > >
> > > > This was enough to fix virtio-net in the rhel6.5 x86 guest case. Unfortunately,
> > > > this fails for rhel6.5 ppc64 because it is never called... 
> > > 
> > > > I did some debugging: it looks like the guest kernel calls the OF
> > > > quisece call to flush pending DMA and disables bus master on the
> > > > virtio-blk device (PCI_COMMAND == 0x3).
> > > 
> > > Getting confused, above you are talking about virtio-net and here it is
> > > virtio-blk.
> > > 
> > > Anyways, the routines still remains same for both of them.  From SLOF
> > > during init we set DRIVER_OK, and after using the device during the
> > > quiesce, called from linux kernel VIRTIO_CONFIG_S_FAILED is set and then
> > > a VIRTIO_DEVICE_RESET is done.
> > 
> > BTW, you really should start enabling bus mastering, avoid relying
> > on the work-around we have for broken guests.
> > 
> 
> FWIW during my debug session, I see that SLOF enables bus mastering...
> unfortunately, it gets disabled at some point after the guest kernel
> is started (around the ppc64 prom_init() call).


OK I'm not sure I have all the details but does the patch I sent help
you?

-- 
MST

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Michael S. Tsirkin]

On Thu, Sep 11, 2014 at 08:47:01PM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 11, 2014 at 06:45:33PM +0200, Greg Kurz wrote:
> > From: Michael S. Tsirkin <mst@redhat.com>
> > 
> > commit cc943c36faa192cd4b32af8fe5edb31894017d35
> >     pci: Use bus master address space for delivering MSI/MSI-X messages
> > breaks virtio-net for rhel6.[56] x86 guests because they don't
> > enable bus mastering for virtio PCI devices. For the same reason,
> > rhel6.[56] ppc64 guests cannot boot on a virtio-blk disk anymore.
> > 
> > Old guests forgot to enable bus mastering, enable it automatically on
> > DRIVER (ppc64 guests may never reach DRIVER_OK). We also need to prevent
> > these old guests to disable bus mastering (this happens when the driver
> > probes the device). And we must also re-enable bus mastering after migration.
> > 
> > Cc: qemu-stable@nongnu.org
> > Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > [ old guest detection on DRIVER,
> >   squashed patch from Michael S. Tsirkin to re-enable bus mastering,
> >   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > Tested-by: Cedric Le Goater <clg@fr.ibm.com>
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> 
> I think this is too fragile. Let's try to figure out a strategy
> that does not depend on VIRTIO_PCI_FLAG_BUS_MASTER_BUG.

OTOH it does work, so I think I'll apply something similar (this one is
slightly broken wrt migration) and work on simplifications as a follow-up.

> > ---
> >  hw/virtio/virtio-pci.c |   18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index ddb5da1..f981841 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -317,9 +317,11 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >          /* Linux before 2.6.34 sets the device as OK without enabling
> >             the PCI device bus master bit. In this case we need to disable
> >             some safety checks. */
> > -        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > +        if ((val & VIRTIO_CONFIG_S_DRIVER) &&
> >              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> >              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> > +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > +                                      true);
> >          }
> >          break;
> >      case VIRTIO_MSI_CONFIG_VECTOR:
> > @@ -473,10 +475,14 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
> >      pci_default_write_config(pci_dev, address, val, len);
> >  
> >      if (range_covers_byte(address, len, PCI_COMMAND) &&
> > -        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER) &&
> > -        !(proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG)) {
> > -        virtio_pci_stop_ioeventfd(proxy);
> > -        virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
> > +        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> > +        if (proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG) {
> > +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > +                                      true);
> > +        } else {
> > +            virtio_pci_stop_ioeventfd(proxy);
> > +            virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
> > +        }
> >      }
> >  }
> >  
> > @@ -890,6 +896,8 @@ static void virtio_pci_vmstate_change(DeviceState *d, bool running)
> >          if ((vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> >              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> >              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> > +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > +                                      true);
> >          }
> >          virtio_pci_start_ioeventfd(proxy);
> >      } else {

Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Michael S. Tsirkin]

On Thu, Sep 11, 2014 at 06:45:33PM +0200, Greg Kurz wrote:
> From: Michael S. Tsirkin <mst@redhat.com>
> 
> commit cc943c36faa192cd4b32af8fe5edb31894017d35
>     pci: Use bus master address space for delivering MSI/MSI-X messages
> breaks virtio-net for rhel6.[56] x86 guests because they don't
> enable bus mastering for virtio PCI devices. For the same reason,
> rhel6.[56] ppc64 guests cannot boot on a virtio-blk disk anymore.
> 
> Old guests forgot to enable bus mastering, enable it automatically on
> DRIVER (ppc64 guests may never reach DRIVER_OK). We also need to prevent
> these old guests to disable bus mastering (this happens when the driver
> probes the device). And we must also re-enable bus mastering after migration.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> [ old guest detection on DRIVER,
>   squashed patch from Michael S. Tsirkin to re-enable bus mastering,
>   Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Tested-by: Cedric Le Goater <clg@fr.ibm.com>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

I think this is too fragile. Let's try to figure out a strategy
that does not depend on VIRTIO_PCI_FLAG_BUS_MASTER_BUG.

> ---
>  hw/virtio/virtio-pci.c |   18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ddb5da1..f981841 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -317,9 +317,11 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>          /* Linux before 2.6.34 sets the device as OK without enabling
>             the PCI device bus master bit. In this case we need to disable
>             some safety checks. */
> -        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> +        if ((val & VIRTIO_CONFIG_S_DRIVER) &&
>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> +                                      true);
>          }
>          break;
>      case VIRTIO_MSI_CONFIG_VECTOR:
> @@ -473,10 +475,14 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
>      pci_default_write_config(pci_dev, address, val, len);
>  
>      if (range_covers_byte(address, len, PCI_COMMAND) &&
> -        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER) &&
> -        !(proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG)) {
> -        virtio_pci_stop_ioeventfd(proxy);
> -        virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
> +        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> +        if (proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG) {
> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> +                                      true);
> +        } else {
> +            virtio_pci_stop_ioeventfd(proxy);
> +            virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
> +        }
>      }
>  }
>  
> @@ -890,6 +896,8 @@ static void virtio_pci_vmstate_change(DeviceState *d, bool running)
>          if ((vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&
>              !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
>              proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> +            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> +                                      true);
>          }
>          virtio_pci_start_ioeventfd(proxy);
>      } else {

[Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Greg Kurz]

From: Michael S. Tsirkin <mst@redhat.com>

commit cc943c36faa192cd4b32af8fe5edb31894017d35
    pci: Use bus master address space for delivering MSI/MSI-X messages
breaks virtio-net for rhel6.[56] x86 guests because they don't
enable bus mastering for virtio PCI devices. For the same reason,
rhel6.[56] ppc64 guests cannot boot on a virtio-blk disk anymore.

Old guests forgot to enable bus mastering, enable it automatically on
DRIVER (ppc64 guests may never reach DRIVER_OK). We also need to prevent
these old guests to disable bus mastering (this happens when the driver
probes the device). And we must also re-enable bus mastering after migration.

Cc: qemu-stable@nongnu.org
Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
[ old guest detection on DRIVER,
  squashed patch from Michael S. Tsirkin to re-enable bus mastering,
  Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Tested-by: Cedric Le Goater <clg@fr.ibm.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio-pci.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ddb5da1..f981841 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -317,9 +317,11 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         /* Linux before 2.6.34 sets the device as OK without enabling
            the PCI device bus master bit. In this case we need to disable
            some safety checks. */
-        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
+        if ((val & VIRTIO_CONFIG_S_DRIVER) &&
             !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
             proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
+            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
+                                      true);
         }
         break;
     case VIRTIO_MSI_CONFIG_VECTOR:
@@ -473,10 +475,14 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
     pci_default_write_config(pci_dev, address, val, len);
 
     if (range_covers_byte(address, len, PCI_COMMAND) &&
-        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER) &&
-        !(proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG)) {
-        virtio_pci_stop_ioeventfd(proxy);
-        virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
+        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
+        if (proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG) {
+            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
+                                      true);
+        } else {
+            virtio_pci_stop_ioeventfd(proxy);
+            virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
+        }
     }
 }
 
@@ -890,6 +896,8 @@ static void virtio_pci_vmstate_change(DeviceState *d, bool running)
         if ((vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&
             !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
             proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
+            memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
+                                      true);
         }
         virtio_pci_start_ioeventfd(proxy);
     } else {