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