From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NrdPj-0004pL-HY for qemu-devel@nongnu.org; Tue, 16 Mar 2010 16:30:55 -0400 Received: from [199.232.76.173] (port=57528 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NrdPi-0004pA-T6 for qemu-devel@nongnu.org; Tue, 16 Mar 2010 16:30:54 -0400 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NrdPh-0005hM-6l for qemu-devel@nongnu.org; Tue, 16 Mar 2010 16:30:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9536) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NrdPg-0005hI-Oq for qemu-devel@nongnu.org; Tue, 16 Mar 2010 16:30:53 -0400 Date: Tue, 16 Mar 2010 22:27:18 +0200 From: "Michael S. Tsirkin" Message-ID: <20100316202718.GC29493@redhat.com> References: <1268763487-23446-1-git-send-email-agraf@suse.de> <20100316202143.GA29493@redhat.com> <8DF00113-9D72-4FA0-BAF5-882F9301F710@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8DF00113-9D72-4FA0-BAF5-882F9301F710@suse.de> Subject: [Qemu-devel] Re: [PATCH] [Also for STABLE-0.12] Don't check for bus master for old guests List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: qemu-devel@nongnu.org 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 > >> CC: Michael S. Tsirkin > >> --- > >> 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