Patchwork [Also,for,STABLE-0.12] Don't check for bus master for old guests

login
register
mail settings
Submitter Alexander Graf
Date March 16, 2010, 6:18 p.m.
Message ID <1268763487-23446-1-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/47886/
State New
Headers show

Comments

Alexander Graf - March 16, 2010, 6:18 p.m.
Older Linux guests don't activate the bus master enable bit. So for those we
can just try to be clever and track if they set the DEVICE_OK bit even though
bus mastering is still disabled.

Under that condition we can disable the windows safety check. With that logic
in place both guests should work just fine. Without PCI hotplug breaks
virtio-net in Linux < 2.6.34 guests.

Signed-off-by: Alexander Graf <agraf@suse.de>
CC: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-pci.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)
Michael S. Tsirkin - March 16, 2010, 8:21 p.m.
On Tue, Mar 16, 2010 at 07:18:07PM +0100, Alexander Graf wrote:
> Older Linux guests don't activate the bus master enable bit. So for those we
> can just try to be clever and track if they set the DEVICE_OK bit even though
> bus mastering is still disabled.
> 
> Under that condition we can disable the windows safety check. With that logic
> in place both guests should work just fine. Without PCI hotplug breaks
> virtio-net in Linux < 2.6.34 guests.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> CC: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio-pci.c |   25 ++++++++++++++++++++++++-
>  1 files changed, 24 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 3594152..4fc4b3c 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -76,6 +76,10 @@
>   * 12 is historical, and due to x86 page size. */
>  #define VIRTIO_PCI_QUEUE_ADDR_SHIFT    12
>  
> +/* We can catch some guest bugs inside here so we continue supporting older
> +   guests. */
> +#define VIRTIO_PCI_BUG_BUS_MASTER	(1 << 0)
> +
>  /* QEMU doesn't strictly need write barriers since everything runs in
>   * lock-step.  We'll leave the calls to wmb() in though to make it obvious for
>   * KVM or if kqemu gets SMP support.
> @@ -87,6 +91,7 @@
>  typedef struct {
>      PCIDevice pci_dev;
>      VirtIODevice *vdev;
> +    uint32_t bugs;
>      uint32_t addr;
>      uint32_t class_code;
>      uint32_t nvectors;
> @@ -138,6 +143,13 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
>      if (proxy->vdev->config_vector != VIRTIO_NO_VECTOR) {
>          return msix_vector_use(&proxy->pci_dev, proxy->vdev->config_vector);
>      }
> +
> +    /* Try to find out if the guest has bus master disabled, but is
> +       in ready state. Then we have a buggy guest OS. */
> +    if (!(proxy->vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&

should not this be (proxy->vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)?

> +        !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> +        proxy->bugs |= VIRTIO_PCI_BUG_BUS_MASTER;
> +    }
>      return 0;
>  }
>  
> @@ -162,6 +174,7 @@ static void virtio_pci_reset(DeviceState *d)
>      VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
>      virtio_reset(proxy->vdev);
>      msix_reset(&proxy->pci_dev);
> +    proxy->bugs = 0;
>  }
>  
>  static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> @@ -205,6 +218,14 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>              virtio_reset(proxy->vdev);
>              msix_unuse_all_vectors(&proxy->pci_dev);
>          }
> +
> +        /* 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) &&
> +            !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> +            proxy->bugs |= VIRTIO_PCI_BUG_BUS_MASTER;
> +        }
>          break;
>      case VIRTIO_MSI_CONFIG_VECTOR:
>          msix_vector_unuse(&proxy->pci_dev, vdev->config_vector);
> @@ -372,7 +393,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
>  
>      if (PCI_COMMAND == address) {
>          if (!(val & PCI_COMMAND_MASTER)) {
> -            proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
> +            if (!(proxy->bugs & VIRTIO_PCI_BUG_BUS_MASTER)) {

nested if statements are confusing

if (!(val & PCI_COMMAND_MASTER) &&
     !(proxy->bugs & VIRTIO_PCI_BUG_BUS_MASTER))

would be clearer.

> +                proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
> +            }
>          }
>      }
>  
> -- 
> 1.6.0.2
Alexander Graf - March 16, 2010, 8:26 p.m.
On 16.03.2010, at 21:21, Michael S. Tsirkin wrote:

> On Tue, Mar 16, 2010 at 07:18:07PM +0100, Alexander Graf wrote:
>> Older Linux guests don't activate the bus master enable bit. So for those we
>> can just try to be clever and track if they set the DEVICE_OK bit even though
>> bus mastering is still disabled.
>> 
>> Under that condition we can disable the windows safety check. With that logic
>> in place both guests should work just fine. Without PCI hotplug breaks
>> virtio-net in Linux < 2.6.34 guests.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> CC: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> hw/virtio-pci.c |   25 ++++++++++++++++++++++++-
>> 1 files changed, 24 insertions(+), 1 deletions(-)
>> 
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index 3594152..4fc4b3c 100644
>> --- a/hw/virtio-pci.c
>> +++ b/hw/virtio-pci.c
>> @@ -76,6 +76,10 @@
>>  * 12 is historical, and due to x86 page size. */
>> #define VIRTIO_PCI_QUEUE_ADDR_SHIFT    12
>> 
>> +/* We can catch some guest bugs inside here so we continue supporting older
>> +   guests. */
>> +#define VIRTIO_PCI_BUG_BUS_MASTER	(1 << 0)
>> +
>> /* QEMU doesn't strictly need write barriers since everything runs in
>>  * lock-step.  We'll leave the calls to wmb() in though to make it obvious for
>>  * KVM or if kqemu gets SMP support.
>> @@ -87,6 +91,7 @@
>> typedef struct {
>>     PCIDevice pci_dev;
>>     VirtIODevice *vdev;
>> +    uint32_t bugs;
>>     uint32_t addr;
>>     uint32_t class_code;
>>     uint32_t nvectors;
>> @@ -138,6 +143,13 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
>>     if (proxy->vdev->config_vector != VIRTIO_NO_VECTOR) {
>>         return msix_vector_use(&proxy->pci_dev, proxy->vdev->config_vector);
>>     }
>> +
>> +    /* Try to find out if the guest has bus master disabled, but is
>> +       in ready state. Then we have a buggy guest OS. */
>> +    if (!(proxy->vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> 
> should not this be (proxy->vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)?

Yikes. Of course.

> 
>> +        !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
>> +        proxy->bugs |= VIRTIO_PCI_BUG_BUS_MASTER;
>> +    }
>>     return 0;
>> }
>> 
>> @@ -162,6 +174,7 @@ static void virtio_pci_reset(DeviceState *d)
>>     VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
>>     virtio_reset(proxy->vdev);
>>     msix_reset(&proxy->pci_dev);
>> +    proxy->bugs = 0;
>> }
>> 
>> static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>> @@ -205,6 +218,14 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>             virtio_reset(proxy->vdev);
>>             msix_unuse_all_vectors(&proxy->pci_dev);
>>         }
>> +
>> +        /* 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) &&
>> +            !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
>> +            proxy->bugs |= VIRTIO_PCI_BUG_BUS_MASTER;
>> +        }
>>         break;
>>     case VIRTIO_MSI_CONFIG_VECTOR:
>>         msix_vector_unuse(&proxy->pci_dev, vdev->config_vector);
>> @@ -372,7 +393,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
>> 
>>     if (PCI_COMMAND == address) {
>>         if (!(val & PCI_COMMAND_MASTER)) {
>> -            proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
>> +            if (!(proxy->bugs & VIRTIO_PCI_BUG_BUS_MASTER)) {
> 
> nested if statements are confusing
> 
> if (!(val & PCI_COMMAND_MASTER) &&
>     !(proxy->bugs & VIRTIO_PCI_BUG_BUS_MASTER))
> 
> would be clearer.

While I agree in general, I figured I'd try to be as little intrusive as possible. And I didn't really want to do different patches for -stable and master.


Alex
Michael S. Tsirkin - March 16, 2010, 8:27 p.m.
On Tue, Mar 16, 2010 at 09:26:53PM +0100, Alexander Graf wrote:
> 
> On 16.03.2010, at 21:21, Michael S. Tsirkin wrote:
> 
> > On Tue, Mar 16, 2010 at 07:18:07PM +0100, Alexander Graf wrote:
> >> Older Linux guests don't activate the bus master enable bit. So for those we
> >> can just try to be clever and track if they set the DEVICE_OK bit even though
> >> bus mastering is still disabled.
> >> 
> >> Under that condition we can disable the windows safety check. With that logic
> >> in place both guests should work just fine. Without PCI hotplug breaks
> >> virtio-net in Linux < 2.6.34 guests.
> >> 
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> CC: Michael S. Tsirkin <mst@redhat.com>
> >> ---
> >> hw/virtio-pci.c |   25 ++++++++++++++++++++++++-
> >> 1 files changed, 24 insertions(+), 1 deletions(-)
> >> 
> >> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> >> index 3594152..4fc4b3c 100644
> >> --- a/hw/virtio-pci.c
> >> +++ b/hw/virtio-pci.c
> >> @@ -76,6 +76,10 @@
> >>  * 12 is historical, and due to x86 page size. */
> >> #define VIRTIO_PCI_QUEUE_ADDR_SHIFT    12
> >> 
> >> +/* We can catch some guest bugs inside here so we continue supporting older
> >> +   guests. */
> >> +#define VIRTIO_PCI_BUG_BUS_MASTER	(1 << 0)
> >> +
> >> /* QEMU doesn't strictly need write barriers since everything runs in
> >>  * lock-step.  We'll leave the calls to wmb() in though to make it obvious for
> >>  * KVM or if kqemu gets SMP support.
> >> @@ -87,6 +91,7 @@
> >> typedef struct {
> >>     PCIDevice pci_dev;
> >>     VirtIODevice *vdev;
> >> +    uint32_t bugs;
> >>     uint32_t addr;
> >>     uint32_t class_code;
> >>     uint32_t nvectors;
> >> @@ -138,6 +143,13 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
> >>     if (proxy->vdev->config_vector != VIRTIO_NO_VECTOR) {
> >>         return msix_vector_use(&proxy->pci_dev, proxy->vdev->config_vector);
> >>     }
> >> +
> >> +    /* Try to find out if the guest has bus master disabled, but is
> >> +       in ready state. Then we have a buggy guest OS. */
> >> +    if (!(proxy->vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > 
> > should not this be (proxy->vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)?
> 
> Yikes. Of course.
> 
> > 
> >> +        !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> >> +        proxy->bugs |= VIRTIO_PCI_BUG_BUS_MASTER;
> >> +    }
> >>     return 0;
> >> }
> >> 
> >> @@ -162,6 +174,7 @@ static void virtio_pci_reset(DeviceState *d)
> >>     VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
> >>     virtio_reset(proxy->vdev);
> >>     msix_reset(&proxy->pci_dev);
> >> +    proxy->bugs = 0;
> >> }
> >> 
> >> static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >> @@ -205,6 +218,14 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>             virtio_reset(proxy->vdev);
> >>             msix_unuse_all_vectors(&proxy->pci_dev);
> >>         }
> >> +
> >> +        /* 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) &&
> >> +            !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> >> +            proxy->bugs |= VIRTIO_PCI_BUG_BUS_MASTER;
> >> +        }
> >>         break;
> >>     case VIRTIO_MSI_CONFIG_VECTOR:
> >>         msix_vector_unuse(&proxy->pci_dev, vdev->config_vector);
> >> @@ -372,7 +393,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
> >> 
> >>     if (PCI_COMMAND == address) {
> >>         if (!(val & PCI_COMMAND_MASTER)) {
> >> -            proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
> >> +            if (!(proxy->bugs & VIRTIO_PCI_BUG_BUS_MASTER)) {
> > 
> > nested if statements are confusing
> > 
> > if (!(val & PCI_COMMAND_MASTER) &&
> >     !(proxy->bugs & VIRTIO_PCI_BUG_BUS_MASTER))
> > 
> > would be clearer.
> 
> While I agree in general, I figured I'd try to be as little intrusive as possible. And I didn't really want to do different patches for -stable and master.
> 
> 
> Alex

Roll the master patch, Anthony or I can do -stable.
Anthony Liguori - March 31, 2010, 4:42 p.m.
On 03/16/2010 01:18 PM, Alexander Graf wrote:
> Older Linux guests don't activate the bus master enable bit. So for those we
> can just try to be clever and track if they set the DEVICE_OK bit even though
> bus mastering is still disabled.
>
> Under that condition we can disable the windows safety check. With that logic
> in place both guests should work just fine. Without PCI hotplug breaks
> virtio-net in Linux<  2.6.34 guests.
>
> Signed-off-by: Alexander Graf<agraf@suse.de>
> CC: Michael S. Tsirkin<mst@redhat.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   hw/virtio-pci.c |   25 ++++++++++++++++++++++++-
>   1 files changed, 24 insertions(+), 1 deletions(-)
>
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 3594152..4fc4b3c 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -76,6 +76,10 @@
>    * 12 is historical, and due to x86 page size. */
>   #define VIRTIO_PCI_QUEUE_ADDR_SHIFT    12
>
> +/* We can catch some guest bugs inside here so we continue supporting older
> +   guests. */
> +#define VIRTIO_PCI_BUG_BUS_MASTER	(1<<  0)
> +
>   /* QEMU doesn't strictly need write barriers since everything runs in
>    * lock-step.  We'll leave the calls to wmb() in though to make it obvious for
>    * KVM or if kqemu gets SMP support.
> @@ -87,6 +91,7 @@
>   typedef struct {
>       PCIDevice pci_dev;
>       VirtIODevice *vdev;
> +    uint32_t bugs;
>       uint32_t addr;
>       uint32_t class_code;
>       uint32_t nvectors;
> @@ -138,6 +143,13 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
>       if (proxy->vdev->config_vector != VIRTIO_NO_VECTOR) {
>           return msix_vector_use(&proxy->pci_dev, proxy->vdev->config_vector);
>       }
> +
> +    /* Try to find out if the guest has bus master disabled, but is
> +       in ready state. Then we have a buggy guest OS. */
> +    if (!(proxy->vdev->status&  VIRTIO_CONFIG_S_DRIVER_OK)&&
> +        !(proxy->pci_dev.config[PCI_COMMAND]&  PCI_COMMAND_MASTER)) {
> +        proxy->bugs |= VIRTIO_PCI_BUG_BUS_MASTER;
> +    }
>       return 0;
>   }
>
> @@ -162,6 +174,7 @@ static void virtio_pci_reset(DeviceState *d)
>       VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
>       virtio_reset(proxy->vdev);
>       msix_reset(&proxy->pci_dev);
> +    proxy->bugs = 0;
>   }
>
>   static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> @@ -205,6 +218,14 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>               virtio_reset(proxy->vdev);
>               msix_unuse_all_vectors(&proxy->pci_dev);
>           }
> +
> +        /* 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)&&
> +            !(proxy->pci_dev.config[PCI_COMMAND]&  PCI_COMMAND_MASTER)) {
> +            proxy->bugs |= VIRTIO_PCI_BUG_BUS_MASTER;
> +        }
>           break;
>       case VIRTIO_MSI_CONFIG_VECTOR:
>           msix_vector_unuse(&proxy->pci_dev, vdev->config_vector);
> @@ -372,7 +393,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
>
>       if (PCI_COMMAND == address) {
>           if (!(val&  PCI_COMMAND_MASTER)) {
> -            proxy->vdev->status&= ~VIRTIO_CONFIG_S_DRIVER_OK;
> +            if (!(proxy->bugs&  VIRTIO_PCI_BUG_BUS_MASTER)) {
> +                proxy->vdev->status&= ~VIRTIO_CONFIG_S_DRIVER_OK;
> +            }
>           }
>       }
>
>

Patch

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 3594152..4fc4b3c 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -76,6 +76,10 @@ 
  * 12 is historical, and due to x86 page size. */
 #define VIRTIO_PCI_QUEUE_ADDR_SHIFT    12
 
+/* We can catch some guest bugs inside here so we continue supporting older
+   guests. */
+#define VIRTIO_PCI_BUG_BUS_MASTER	(1 << 0)
+
 /* QEMU doesn't strictly need write barriers since everything runs in
  * lock-step.  We'll leave the calls to wmb() in though to make it obvious for
  * KVM or if kqemu gets SMP support.
@@ -87,6 +91,7 @@ 
 typedef struct {
     PCIDevice pci_dev;
     VirtIODevice *vdev;
+    uint32_t bugs;
     uint32_t addr;
     uint32_t class_code;
     uint32_t nvectors;
@@ -138,6 +143,13 @@  static int virtio_pci_load_config(void * opaque, QEMUFile *f)
     if (proxy->vdev->config_vector != VIRTIO_NO_VECTOR) {
         return msix_vector_use(&proxy->pci_dev, proxy->vdev->config_vector);
     }
+
+    /* Try to find out if the guest has bus master disabled, but is
+       in ready state. Then we have a buggy guest OS. */
+    if (!(proxy->vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+        !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
+        proxy->bugs |= VIRTIO_PCI_BUG_BUS_MASTER;
+    }
     return 0;
 }
 
@@ -162,6 +174,7 @@  static void virtio_pci_reset(DeviceState *d)
     VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     virtio_reset(proxy->vdev);
     msix_reset(&proxy->pci_dev);
+    proxy->bugs = 0;
 }
 
 static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
@@ -205,6 +218,14 @@  static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             virtio_reset(proxy->vdev);
             msix_unuse_all_vectors(&proxy->pci_dev);
         }
+
+        /* 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) &&
+            !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
+            proxy->bugs |= VIRTIO_PCI_BUG_BUS_MASTER;
+        }
         break;
     case VIRTIO_MSI_CONFIG_VECTOR:
         msix_vector_unuse(&proxy->pci_dev, vdev->config_vector);
@@ -372,7 +393,9 @@  static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
 
     if (PCI_COMMAND == address) {
         if (!(val & PCI_COMMAND_MASTER)) {
-            proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
+            if (!(proxy->bugs & VIRTIO_PCI_BUG_BUS_MASTER)) {
+                proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK;
+            }
         }
     }