From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43143) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWqFR-0004J2-L8 for qemu-devel@nongnu.org; Wed, 24 Sep 2014 13:21:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XWqFJ-0004aX-3i for qemu-devel@nongnu.org; Wed, 24 Sep 2014 13:21:01 -0400 Received: from e06smtp17.uk.ibm.com ([195.75.94.113]:44385) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWqFI-0004Z6-Rf for qemu-devel@nongnu.org; Wed, 24 Sep 2014 13:20:53 -0400 Received: from /spool/local by e06smtp17.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 24 Sep 2014 18:20:45 +0100 Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 9D33B17D8045 for ; Wed, 24 Sep 2014 18:22:50 +0100 (BST) Received: from d06av01.portsmouth.uk.ibm.com (d06av01.portsmouth.uk.ibm.com [9.149.37.212]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id s8OHKhoR21299314 for ; Wed, 24 Sep 2014 17:20:43 GMT Received: from d06av01.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av01.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s8OHKg3U031185 for ; Wed, 24 Sep 2014 11:20:43 -0600 Date: Wed, 24 Sep 2014 19:20:38 +0200 From: Greg Kurz Message-ID: <20140924192038.2216c11c@bahia.local> In-Reply-To: <20140923042632.GA15022@redhat.com> References: <1411066387-30889-1-git-send-email-mst@redhat.com> <1411066387-30889-15-git-send-email-mst@redhat.com> <20140922192857.18930639@bahia.local> <20140923042632.GA15022@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Peter Maydell , Jason Wang , qemu-devel@nongnu.org, Anthony Liguori , Alexander Graf On Tue, 23 Sep 2014 07:26:32 +0300 "Michael S. Tsirkin" wrote: > On Mon, Sep 22, 2014 at 07:28:57PM +0200, Greg Kurz wrote: > > On Thu, 18 Sep 2014 21:54:58 +0300 > > "Michael S. Tsirkin" wrote: > > > > > Current support for bus master (clearing OK bit) > > > together with the need to support guests which do not > > > enable PCI bus mastering, leads to extra state in > > > VIRTIO_PCI_FLAG_BUS_MASTER_BUG bit, which isn't robust > > > in case of cross-version migration for the case when > > > guests use the device before setting DRIVER_OK. > > > > > > Rip out VIRTIO_PCI_FLAG_BUS_MASTER_BUG and implement a simpler > > > work-around: treat clearing of PCI_COMMAND as a virtio reset. Old > > > guests never touch this bit so they will work. > > > > > > As reset clears device status, DRIVER and MASTER bits are > > > now in sync, so we can fix up cross-version migration simply > > > by synchronising them, without need to detect a buggy guest > > > explicitly. > > > > > > Drop tracking VIRTIO_PCI_FLAG_BUS_MASTER_BUG completely. > > > > > > As reset makes the device quiescent, in the future we'll be able to drop > > > checking OK bit in a bunch of places. > > > > > > Cc: Jason Wang > > > Cc: Greg Kurz > > > Signed-off-by: Michael S. Tsirkin > > > --- > > > > Hi, > > > > This commit prevents pseries to boot. SLOF complains with the following messages: > > > > Trying to load: from: /pci@800000020000000/scsi@0 ... virtioblk_read failed! status = 255 > > virtioblk_read failed! status = 255 > > virtioblk_read failed! status = 255 > > ... > > > > I'll try to debug some more. > > > > Cheers. > > A trace recording all reads and writes of pci status > and virtio status would help. > Thanks! > > > -- > > Greg > > I've added the traces for reads/writes of PCI_COMMAND, PCI_STATUS and VIRTIO_PCI_STATUS. Also an extra trace in your patch where virtio_reset is called in case BM gets disabled. SLOF ********************************************************************** QEMU Starting Build Date = Jul 3 2014 23:12:05 FW Version = git-f284ab3f03ae69a2 Press "s" to enter Open Firmware. Populating /vdevice methods Populating /vdevice/vty@71000000 Populating /vdevice/nvram@71000001 Populating /pci@800000020000000 Adapters on 0800000020000000 pci_default_read_config: addr = 0x4 val = 0x100000 len= 4 pci_default_write_config: addr = 0x4 val = 0x100000 len= 4 pci_default_read_config: addr = 0x4 val = 0x0 len= 2 pci_default_write_config: addr = 0x4 val = 0x140 len= 2 00 0000 (D) : 1af4 1001 virtio [ block ] pci_default_read_config: addr = 0x4 val = 0x100100 len= 4 pci_default_read_config: addr = 0x4 val = 0x100100 len= 4 pci_default_read_config: addr = 0x4 val = 0x100100 len= 4 pci_default_read_config: addr = 0x4 val = 0x100100 len= 4 pci_default_read_config: addr = 0x4 val = 0x100100 len= 4 pci_default_write_config: addr = 0x4 val = 0x100104 len= 4 pci_default_read_config: addr = 0x4 val = 0x104 len= 2 pci_default_write_config: addr = 0x4 val = 0x106 len= 2 pci_default_read_config: addr = 0x4 val = 0x106 len= 2 pci_default_write_config: addr = 0x4 val = 0x107 len= 2 BM gets enabled pci_default_read_config: addr = 0x4 val = 0x100107 len= 4 pci_default_write_config: addr = 0x4 val = 0x100100 len= 4 virtio_write_config: RESET device gets disabled => virtio_reset() gets called pci_default_read_config: addr = 0x4 val = 0x100000 len= 4 pci_default_write_config: addr = 0x4 val = 0x100000 len= 4 pci_default_read_config: addr = 0x4 val = 0x0 len= 2 pci_default_write_config: addr = 0x4 val = 0x140 len= 2 00 0800 (D) : 1af4 1000 virtio [ net ] pci_default_read_config: addr = 0x4 val = 0x100100 len= 4 pci_default_read_config: addr = 0x4 val = 0x100100 len= 4 pci_default_read_config: addr = 0x4 val = 0x100100 len= 4 pci_default_read_config: addr = 0x4 val = 0x100100 len= 4 pci_default_read_config: addr = 0x4 val = 0x100 len= 2 pci_default_write_config: addr = 0x4 val = 0x101 len= 2 pci_default_read_config: addr = 0x4 val = 0x100101 len= 4 pci_default_write_config: addr = 0x4 val = 0x100100 len= 4 No NVRAM common partition, re-initializing... Scanning USB Using default console: /vdevice/vty@71000000 Welcome to Open Firmware Copyright (c) 2004, 2011 IBM Corporation All rights reserved. This program and the accompanying materials are made available under the terms of the BSD License available at http://www.opensource.org/licenses/bsd-license.php Trying to load: from: /pci@800000020000000/scsi@0 ... pci_default_read_config: addr = 0x4 val = 0x100100 len= 4 pci_default_write_config: addr = 0x4 val = 0x100104 len= 4 pci_default_read_config: addr = 0x4 val = 0x104 len= 2 pci_default_write_config: addr = 0x4 val = 0x106 len= 2 pci_default_read_config: addr = 0x4 val = 0x106 len= 2 pci_default_write_config: addr = 0x4 val = 0x107 len= 2 virtio_ioport_write: VIRTIO_PCI_STATUS = 0x1 virtio_ioport_write: VIRTIO_PCI_STATUS = 0x3 pci_default_write_config: addr = 0x4 val = 0x7 len= 1 virtio_ioport_write: VIRTIO_PCI_STATUS = 0x7 virtioblk_read failed! status = 255 virtioblk_read failed! status = 255 virtioblk_read failed! status = 255 ... The guest boots well without your patch. Here's a diff of the outputs for both runs: [greg@alize ~]$ diff virtio-pci-ok.log virtio-pci-broken.log 28a29 > virtio_write_config: RESET 64,66c65,67 < pci_default_read_config: addr = 0x4 val = 0x100107 len= 4 < pci_default_write_config: addr = 0x4 val = 0x100100 len= 4 < Successfully loaded --- > virtioblk_read failed! status = 255 > virtioblk_read failed! status = 255 > virtioblk_read failed! status = 255 FWIW, if I comment the call to virtio_reset() when BM gets disabled, the guest boots... I don't why SLOF doesn't like the device to be reset during the PCI bus probing though... A suivre. > > > hw/virtio/virtio-pci.c | 39 ++++++++++++++++++++------------------- > > > 1 file changed, 20 insertions(+), 19 deletions(-) > > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > > index a827cd4..f560814 100644 > > > --- a/hw/virtio/virtio-pci.c > > > +++ b/hw/virtio/virtio-pci.c > > > @@ -86,9 +86,6 @@ > > > * 12 is historical, and due to x86 page size. */ > > > #define VIRTIO_PCI_QUEUE_ADDR_SHIFT 12 > > > > > > -/* Flags track per-device state like workarounds for quirks in older guests. */ > > > -#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 << 0) > > > - > > > static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size, > > > VirtIOPCIProxy *dev); > > > > > > @@ -323,14 +320,6 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) > > > proxy->pci_dev.config[PCI_COMMAND] | > > > PCI_COMMAND_MASTER, 1); > > > } > > > - > > > - /* 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->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; > > > - } > > > break; > > > case VIRTIO_MSI_CONFIG_VECTOR: > > > msix_vector_unuse(&proxy->pci_dev, vdev->config_vector); > > > @@ -480,13 +469,18 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > > > VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > > > VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > > > > > > + uint8_t cmd = proxy->pci_dev.config[PCI_COMMAND]; > > > + > > > pci_default_write_config(pci_dev, address, val, len); > > > > > > if (range_covers_byte(address, len, PCI_COMMAND) && > > > !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER) && > > > - !(proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG)) { > > > + (cmd & PCI_COMMAND_MASTER)) { > > > + /* Bus driver disables bus mastering - make it act > > > + * as a kind of reset to render the device quiescent. */ > > > virtio_pci_stop_ioeventfd(proxy); > > > - virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK); > > > + virtio_reset(vdev); > > > + msix_unuse_all_vectors(&proxy->pci_dev); > > > } > > > } > > > > > > @@ -895,11 +889,19 @@ static void virtio_pci_vmstate_change(DeviceState *d, bool running) > > > VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > > > > > > if (running) { > > > - /* Try to find out if the guest has bus master disabled, but is > > > - in ready state. Then we have a buggy guest OS. */ > > > - if ((vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) && > > > - !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) { > > > - proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; > > > + /* Linux before 2.6.34 drives the device without enabling > > > + the PCI device bus master bit. Enable it automatically > > > + for the guest. This is a PCI spec violation but so is > > > + initiating DMA with bus master bit clear. > > > + Note: this only makes a difference when migrating > > > + across QEMU versions from an old QEMU, as for new QEMU > > > + bus master and driver bits are always in sync. > > > + TODO: consider enabling conditionally for compat machine types. */ > > > + if (vdev->status & (VIRTIO_CONFIG_S_ACKNOWLEDGE | > > > + VIRTIO_CONFIG_S_DRIVER)) { > > > + pci_default_write_config(&proxy->pci_dev, PCI_COMMAND, > > > + proxy->pci_dev.config[PCI_COMMAND] | > > > + PCI_COMMAND_MASTER, 1); > > > } > > > virtio_pci_start_ioeventfd(proxy); > > > } else { > > > @@ -1040,7 +1042,6 @@ static void virtio_pci_reset(DeviceState *qdev) > > > virtio_pci_stop_ioeventfd(proxy); > > > virtio_bus_reset(bus); > > > msix_unuse_all_vectors(&proxy->pci_dev); > > > - proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG; > > > } > > > > > > static Property virtio_pci_properties[] = { >