[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus m
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master |
Date: |
Wed, 24 Sep 2014 19:20:38 +0200 |
On Tue, 23 Sep 2014 07:26:32 +0300
"Michael S. Tsirkin" <address@hidden> 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" <address@hidden> 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 <address@hidden>
> > > Cc: Greg Kurz <address@hidden>
> > > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > > ---
> >
> > Hi,
> >
> > This commit prevents pseries to boot. SLOF complains with the following
> > messages:
> >
> > Trying to load: from: /address@hidden/address@hidden ... 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/address@hidden
Populating /vdevice/address@hidden
Populating /address@hidden
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/address@hidden
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: /address@hidden/address@hidden ...
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:
address@hidden ~]$ 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[] = {
>
- [Qemu-devel] [PULL v3 08/15] qdev: Move global validation to a single function, (continued)
- [Qemu-devel] [PULL v3 08/15] qdev: Move global validation to a single function, Michael S. Tsirkin, 2014/09/18
- [Qemu-devel] [PULL v3 09/15] Revert "rng-egd: remove redundant free", Michael S. Tsirkin, 2014/09/18
- [Qemu-devel] [PULL v3 10/15] virtio-net: drop assert on vm stop, Michael S. Tsirkin, 2014/09/18
- [Qemu-devel] [PULL v3 11/15] Revert "virtio: don't call device on !vm_running", Michael S. Tsirkin, 2014/09/18
- [Qemu-devel] [PULL v3 13/15] vhost-user: fix VIRTIO_NET_F_MRG_RXBUF negotiation, Michael S. Tsirkin, 2014/09/18
- [Qemu-devel] [PULL v3 12/15] virtio-pci: enable bus master for old guests, Michael S. Tsirkin, 2014/09/18
- [Qemu-devel] [PULL v3 15/15] pc: leave more space for BIOS allocations, Michael S. Tsirkin, 2014/09/18
- [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master, Michael S. Tsirkin, 2014/09/18
- Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master, Michael S. Tsirkin, 2014/09/29
- Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master, Greg Kurz, 2014/09/29
- Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master, Nikunj A Dadhania, 2014/09/30
[Qemu-devel] [PULL v3 01/15] hw/machine: Free old values of string properties, Michael S. Tsirkin, 2014/09/18
Re: [Qemu-devel] [PULL v3 00/15] pci, pc, virtio, misc bugfixes, Peter Maydell, 2014/09/18