qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH RFC] virtio-pci: disable vring processing when bus-mastering


From: Michael S. Tsirkin
Subject: Re: [PATCH RFC] virtio-pci: disable vring processing when bus-mastering is disabled
Date: Thu, 14 Nov 2019 04:12:00 -0500

On Thu, Nov 14, 2019 at 04:10:36AM -0500, Michael S. Tsirkin wrote:
> On Wed, Nov 13, 2019 at 07:07:36PM -0600, Michael Roth wrote:
> > Quoting Michael S. Tsirkin (2019-11-13 04:09:02)
> > > 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 <address@hidden>,
> > > > Cc: Alexey Kardashevskiy <address@hidden>
> > > > Cc: "Michael S. Tsirkin" <address@hidden>
> > > > Signed-off-by: Michael Roth <address@hidden>
> > > > ---
> > > >  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.
> > 
> > Agreed. Some comments on that below.
> > 
> > > 
> > > 
> > > > @@ -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?
> > 
> > Currently vdev->broken is already migrated via a subsection if it is set.
> > 
> > So, with the current proposed patch I would expect the following:
> > 
> >  old -> new:
> >    dev->broken set due to proposed virtio_set_disabled():
> >      - N/A
> >    dev->broken set due to other/existing reasons:
> >      - value is migrated, migration succeeds
> >      - device continues not working until a reset, or bus-mastering
> >        enabled/re-enabled. (which is good, but also makes me realize
> >        that a device that was put into broken state for reasons
> >        other than virtio_set_disabled() should not get 'unbroken'
> >        simply because bus-master bit was cycled. a separate flag
> >        is probably needed)
> >      - PASS
> >    dev->broken not set:
> >      - not migrated
> >      - PASS
> >  new -> old:
> >    dev->broken set due to proposed virtio_set_disabled():
> >      - migration succeeds to any QEMU that already has handling for
> >        dev->broken.
> >      - migration fails for any QEMU that doesn't have handling for
> >        dev->broken as it does now, but will a higher chance of
> >        triggering
> >      - device won't work until it is reset. for most guests we will
> >        probably get a reset before the device is used again anyway. for
> >        the SLOF case the device will stay broken after bus-mastering
> >        re-enabled, but that's the case for existing QEMU now anyway
> >      - PASS, but with increased chance of migration breakage for
> >        QEMU's that don't have handling for dev->broken.
> >    dev->broken set due to other/existing reasons:
> >      - migration succeeds to any QEMU that already has handling for
> >        dev->broken. device will require a reset as it does now.
> >      - migration fails for any QEMU that doesn't have handling for
> >        dev->broken as it does now
> >      - PASS
> >    dev->broken not set:
> >      - not migrated
> >      - PASS
> > 
> > With a new dev->disabled flag, which we'd likely send using a subsection
> > like with dev->broken, I would expect the following:
> > 
> >  old -> new:
> >    dev->disabled set:
> >      - N/A
> >    dev->disabled not set:
> >      - not migrated
> >      - if source disables BM just before migration we will likely go
> >        into an error state that either sets dev->broken or puts the
> >        device in some other possibly bad state. either case would require
> >        subsequent reset, just as they would without migration
> >      - PASS
> >  new -> old:
> >    dev->disabled set:
> >      - migration stream fails
> >      - FAIL, but as expected, and doesn't seem common currently outside
> >        of SLOF early-boot else we'd probably have more reports of
> >        breakage from vring access while BM isn't set.
> >    dev->broken not set:
> >      - not migrated
> >      - PASS
> > 
> > So re-using 'broken' is slightly better from a migration standpoint, but
> > as noted above it is probably wrong to unset 'broken' just because BM bit
> > gets cycled, so a new 'disabled' flag is probably needed.
> > 
> > We could probably get by with just adding a check for dev->disabled in 
> > virtio_queue_empty(), or even earlier in
> > virtio_queue_host_notifier_aio_poll(), but it seems more proper to also
> > add it in most of the same places we currently check for dev->broken.
> > 
> > That seems somewhat redundant though, so I think maybe the best approach
> > is to:
> > 
> >  - replace most dev->broken checks with checks for dev->disabled
> 
> I propose calling an inline functions so we are not tied to a specific
> field.
> 
> >  - set dev->disabled whenever dev->broken gets set
> >  - add a check in virtio_set_disabled() that only allows us to re-enable if
> >    dev->broken hasn't also been set.
> > 
> > I'll work on a follow-up using that approach if it seems reasonable to
> > you.


Also, since the new field is not migrated anyway, how about a flag
that old machine types set, and behave the way we do now?
> > 
> > Thanks!
> > 
> > > 
> > > > -- 
> > > > 2.17.1
> > > 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]