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 <ag...@suse.de> > >> CC: Michael S. Tsirkin <m...@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