On Tue, Nov 12, 2019 at 11:43:01PM -0600, Michael Roth wrote:
> Currently the SLOF firmware for pseries guests will disable/re-enable
> a PCI device multiple times via IO/MEM/MASTER bits of PCI_COMMAND
> register after the initial probe/feature negotiation, as it tends to
> work with a single device at a time at various stages like probing
> and running block/network bootloaders without doing a full reset
> in-between.
>
> In QEMU, when PCI_COMMAND_MASTER is disabled we disable the
> corresponding IOMMU memory region, so DMA accesses (including to vring
> fields like idx/flags) will no longer undergo the necessary
> translation. Normally we wouldn't expect this to happen since it would
> be misbehavior on the driver side to continue driving DMA requests.
>
> However, in the case of pseries, with iommu_platform=on, we trigger the
> following sequence when tearing down the virtio-blk dataplane ioeventfd
> in response to the guest unsetting PCI_COMMAND_MASTER:
>
> #2 0x0000555555922651 in virtqueue_map_desc
> (vdev=vdev@entry=0x555556dbcfb0, p_num_sg=p_num_sg@entry=0x7fffe657e1a8,
> addr=addr@entry=0x7fffe657e240, iov=iov@entry=0x7fffe6580240,
> max_num_sg=max_num_sg@entry=1024, is_write=is_write@entry=false, pa=0, sz=0)
> at /home/mdroth/w/qemu.git/hw/virtio/virtio.c:757
> #3 0x0000555555922a89 in virtqueue_pop (vq=vq@entry=0x555556dc8660,
> sz=sz@entry=184)
> at /home/mdroth/w/qemu.git/hw/virtio/virtio.c:950
> #4 0x00005555558d3eca in virtio_blk_get_request (vq=0x555556dc8660,
> s=0x555556dbcfb0)
> at /home/mdroth/w/qemu.git/hw/block/virtio-blk.c:255
> #5 0x00005555558d3eca in virtio_blk_handle_vq (s=0x555556dbcfb0,
> vq=0x555556dc8660)
> at /home/mdroth/w/qemu.git/hw/block/virtio-blk.c:776
> #6 0x000055555591dd66 in virtio_queue_notify_aio_vq
> (vq=vq@entry=0x555556dc8660)
> at /home/mdroth/w/qemu.git/hw/virtio/virtio.c:1550
> #7 0x000055555591ecef in virtio_queue_notify_aio_vq (vq=0x555556dc8660)
> at /home/mdroth/w/qemu.git/hw/virtio/virtio.c:1546
> #8 0x000055555591ecef in virtio_queue_host_notifier_aio_poll
> (opaque=0x555556dc86c8)
> at /home/mdroth/w/qemu.git/hw/virtio/virtio.c:2527
> #9 0x0000555555d02164 in run_poll_handlers_once
> (ctx=ctx@entry=0x55555688bfc0, timeout=timeout@entry=0x7fffe65844a8)
> at /home/mdroth/w/qemu.git/util/aio-posix.c:520
> #10 0x0000555555d02d1b in try_poll_mode (timeout=0x7fffe65844a8,
> ctx=0x55555688bfc0)
> at /home/mdroth/w/qemu.git/util/aio-posix.c:607
> #11 0x0000555555d02d1b in aio_poll (ctx=ctx@entry=0x55555688bfc0,
> blocking=blocking@entry=true)
> at /home/mdroth/w/qemu.git/util/aio-posix.c:639
> #12 0x0000555555d0004d in aio_wait_bh_oneshot (ctx=0x55555688bfc0,
> cb=cb@entry=0x5555558d5130 <virtio_blk_data_plane_stop_bh>,
> opaque=opaque@entry=0x555556de86f0)
> at /home/mdroth/w/qemu.git/util/aio-wait.c:71
> #13 0x00005555558d59bf in virtio_blk_data_plane_stop (vdev=<optimized out>)
> at /home/mdroth/w/qemu.git/hw/block/dataplane/virtio-blk.c:288
> #14 0x0000555555b906a1 in virtio_bus_stop_ioeventfd
> (bus=bus@entry=0x555556dbcf38)
> at /home/mdroth/w/qemu.git/hw/virtio/virtio-bus.c:245
> #15 0x0000555555b90dbb in virtio_bus_stop_ioeventfd
> (bus=bus@entry=0x555556dbcf38)
> at /home/mdroth/w/qemu.git/hw/virtio/virtio-bus.c:237
> #16 0x0000555555b92a8e in virtio_pci_stop_ioeventfd (proxy=0x555556db4e40)
> at /home/mdroth/w/qemu.git/hw/virtio/virtio-pci.c:292
> #17 0x0000555555b92a8e in virtio_write_config (pci_dev=0x555556db4e40,
> address=<optimized out>, val=1048832, len=<optimized out>)
> at /home/mdroth/w/qemu.git/hw/virtio/virtio-pci.c:613
>
> I.e. the calling code is only scheduling a one-shot BH for
> virtio_blk_data_plane_stop_bh, but somehow we end up trying to process
> an additional virtqueue entry before we get there. This is likely due
> to the following check in virtio_queue_host_notifier_aio_poll:
>
> static bool virtio_queue_host_notifier_aio_poll(void *opaque)
> {
> EventNotifier *n = opaque;
> VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
> bool progress;
>
> if (!vq->vring.desc || virtio_queue_empty(vq)) {
> return false;
> }
>
> progress = virtio_queue_notify_aio_vq(vq);
>
> namely the call to virtio_queue_empty(). In this case, since no new
> requests have actually been issued, shadow_avail_idx == last_avail_idx,
> so we actually try to access the vring via vring_avail_idx() to get
> the latest non-shadowed idx:
>
> int virtio_queue_empty(VirtQueue *vq)
> {
> bool empty;
> ...
>
> if (vq->shadow_avail_idx != vq->last_avail_idx) {
> return 0;
> }
>
> rcu_read_lock();
> empty = vring_avail_idx(vq) == vq->last_avail_idx;
> rcu_read_unlock();
> return empty;
>
> but since the IOMMU region has been disabled we get a bogus value (0
> usually), which causes virtio_queue_empty() to falsely report that
> there are entries to be processed, which causes errors such as:
>
> "virtio: zero sized buffers are not allowed"
>
> or
>
> "virtio-blk missing headers"
>
> and puts the device in an error state.
>
> This patch works around the issue by introducing virtio_set_disabled(),
> which piggy-backs off the vdev->broken flag we already use to bypass
> checks like virtio_queue_empty(), and sets/unsets it in response to
> enabling/disabling bus-mastering.
>
> NOTES:
>
> - It's possible we could also work around this in SLOF by doing a
> full reset instead of relying on PCI_COMMAND to enable/disable, but
> in any case the QEMU behavior seems wrong.
> - This leaves some other oddities in play, like the fact that
> DRIVER_OK also gets unset in response to bus-mastering being
> disabled, but not restored (however the device seems to continue
> working)
> - Similarly, we disable the host notifier via
> virtio_bus_stop_ioeventfd(), which seems to move the handling out
> of virtio-blk dataplane and back into the main IO thread, and it
> ends up staying there till a reset (but otherwise continues working
> normally)
>
> Cc: David Gibson <da...@gibson.dropbear.id.au>,
> Cc: Alexey Kardashevskiy <a...@ozlabs.ru>
> Cc: "Michael S. Tsirkin" <m...@redhat.com>
> Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com>
> ---
> hw/virtio/virtio-pci.c | 12 ++++++++----
> hw/virtio/virtio.c | 7 ++++++-
> include/hw/virtio/virtio.h | 1 +
> 3 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index c6b47a9c73..394d409fb9 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -608,10 +608,14 @@ static void virtio_write_config(PCIDevice *pci_dev,
> uint32_t address,
> pcie_cap_flr_write_config(pci_dev, address, val, len);
> }
>
> - if (range_covers_byte(address, len, PCI_COMMAND) &&
> - !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> - virtio_pci_stop_ioeventfd(proxy);
> - virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
> + if (range_covers_byte(address, len, PCI_COMMAND)) {
> + if (!(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> + virtio_set_disabled(vdev, true);
> + virtio_pci_stop_ioeventfd(proxy);
> + virtio_set_status(vdev, vdev->status &
> ~VIRTIO_CONFIG_S_DRIVER_OK);
> + } else {
> + virtio_set_disabled(vdev, false);
> + }
> }
>
> if (proxy->config_cap &&
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 527df03bfd..46580c357d 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2575,6 +2575,11 @@ void virtio_device_set_child_bus_name(VirtIODevice
> *vdev, char *bus_name)
> vdev->bus_name = g_strdup(bus_name);
> }
>
> +void virtio_set_disabled(VirtIODevice *vdev, bool disable)
> +{
> + vdev->broken = disable;
> +}
> +
> void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt,
> ...)
> {
> va_list ap;
Hmm. I think just clear and immediate set of bus master while device was
not doing any DMA actually should be fine and should not require a
reset. Now it's true that right now guests reset first thing which will
clear the broken flag, but I'd say it's wrong to require this specific
order.
I think the easiest thing is to add a "disabled" flag.
> @@ -2588,7 +2593,7 @@ void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice
> *vdev, const char *fmt, ...)
> virtio_notify_config(vdev);
> }
>
> - vdev->broken = true;
> + virtio_set_disabled(vdev, true);
> }
>
> static void virtio_memory_listener_commit(MemoryListener *listener)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 48e8d04ff6..921945b7e8 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -168,6 +168,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
> uint16_t device_id, size_t config_size);
> void virtio_cleanup(VirtIODevice *vdev);
>
> +void virtio_set_disabled(VirtIODevice *vdev, bool disable);
> void virtio_error(VirtIODevice *vdev, const char *fmt, ...) GCC_FMT_ATTR(2,
> 3);
>
> /* Set the child bus name. */
One open question here is cross version migration.
What exactly happens if we migrate to an old qemu?
from an old qemu?
> --
> 2.17.1
