Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master

[Greg Kurz <gkurz@linux.vnet.ibm.com>]

On Tue, 23 Sep 2014 07:26:32 +0300
"Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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 <jasowang@redhat.com>
> > > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > 
> > 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[] = {
>