[Qemu-devel] Re: [PATCH] [Also for STABLE-0.12] Don't check for bus master for old guests

["Michael S. Tsirkin" <mst@redhat.com>]

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.

-- 
MST