| Message ID | 1411066387-30889-15-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. -- Greg > 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[] = {
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 > > > 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[] = {
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[] = { >
Hi Alex/Peter, The below patch is already been picked in master and ppc-next and has broken pseries booting from virtio-blk device Greg Kurz <gkurz@linux.vnet.ibm.com> writes: > On Tue, 23 Sep 2014 07:26:32 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> > >> > 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. >> > >> > > @@ -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); >> > > } >> > > } >> > > >> Regards Nikunj
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. OK I'll revert for now. > 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. > > -- > Greg > > > 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[] = {
On Mon, 29 Sep 2014 19:15:05 +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. > > OK I'll revert for now. > It may be possible to change the way SLOF configures the virtq so that it wouldn't suffer from the device being reset. We have some time left before 2.2. I have a tentative patch for SLOF that I'll be glad to share with Nikunj ASAP. > > 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. > > > > -- > > Greg > > > > > 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[] = { >
Greg Kurz <gkurz@linux.vnet.ibm.com> writes: > On Mon, 29 Sep 2014 19:15:05 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > It may be possible to change the way SLOF configures the virtq so > that it wouldn't suffer from the device being reset. We have some > time left before 2.2. I have a tentative patch for SLOF that I'll > be glad to share with Nikunj ASAP. virtio-block is straight forward. I think we will have similar problem with virtio-scsi as well. That driver will be tricky as it is inter-linked with generic scsi code in SLOF. Regards Nikunj
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[] = {
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> --- hw/virtio/virtio-pci.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-)