Skip to content

Commit

Permalink
virtio-pci: disable vring processing when bus-mastering is disabled
Browse files Browse the repository at this point in the history 
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 sets a 'disabled' flag to bypass checks like virtio_queue_empty()
when bus-mastering is disabled. Since we'd check this flag at all the
same sites as vdev->broken, we replace those checks with an inline
function which checks for either vdev->broken or vdev->disabled.

The 'disabled' flag is only migrated when set, which should be fairly
rare, but to maintain migration compatibility we disable it's use for
older machine types. Users requiring the use of the flag in conjunction
with older machine types can set it explicitly as a virtio-device
option.

NOTES:

 - 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 <david@gibson.dropbear.id.au>,
Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Message-Id: <20191120005003.27035-1-mdroth@linux.vnet.ibm.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
  • Loading branch information
@mdroth @AndrewFasano
mdroth authored and AndrewFasano committed Oct 16, 2023
1 parent 94078b2 commit cde7cb6
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 11 deletions.
12 changes: 8 additions & 4 deletions hw/virtio/virtio-pci.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down
35 changes: 28 additions & 7 deletions hw/virtio/virtio.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,7 @@ static inline bool is_desc_avail(uint16_t flags, bool wrap_counter)
* Called within rcu_read_lock(). */
static int virtio_queue_empty_rcu(VirtQueue *vq)
{
if (unlikely(vq->vdev->broken)) {
if (virtio_device_disabled(vq->vdev)) {
return 1;
}

Expand All @@ -565,7 +565,7 @@ static int virtio_queue_split_empty(VirtQueue *vq)
{
bool empty;

if (unlikely(vq->vdev->broken)) {
if (virtio_device_disabled(vq->vdev)) {
return 1;
}

Expand Down Expand Up @@ -783,7 +783,7 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,

virtqueue_unmap_sg(vq, elem, len);

if (unlikely(vq->vdev->broken)) {
if (virtio_device_disabled(vq->vdev)) {
return;
}

Expand Down Expand Up @@ -839,7 +839,7 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)

void virtqueue_flush(VirtQueue *vq, unsigned int count)
{
if (unlikely(vq->vdev->broken)) {
if (virtio_device_disabled(vq->vdev)) {
vq->inuse -= count;
return;
}
Expand Down Expand Up @@ -1602,7 +1602,7 @@ static void *virtqueue_packed_pop(VirtQueue *vq, size_t sz)

void *virtqueue_pop(VirtQueue *vq, size_t sz)
{
if (unlikely(vq->vdev->broken)) {
if (virtio_device_disabled(vq->vdev)) {
return NULL;
}

Expand Down Expand Up @@ -1698,7 +1698,7 @@ unsigned int virtqueue_drop_all(VirtQueue *vq)
{
struct VirtIODevice *vdev = vq->vdev;

if (unlikely(vdev->broken)) {
if (virtio_device_disabled(vq->vdev)) {
return 0;
}

Expand Down Expand Up @@ -1816,7 +1816,7 @@ static void virtio_notify_vector(VirtIODevice *vdev, uint16_t vector)
BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);

if (unlikely(vdev->broken)) {
if (virtio_device_disabled(vdev)) {
return;
}

Expand Down Expand Up @@ -1920,6 +1920,7 @@ void virtio_reset(void *opaque)
vdev->guest_features = 0;
vdev->queue_sel = 0;
vdev->status = 0;
vdev->disabled = false;
atomic_set(&vdev->isr, 0);
vdev->config_vector = VIRTIO_NO_VECTOR;
virtio_notify_vector(vdev, vdev->config_vector);
Expand Down Expand Up @@ -2559,6 +2560,13 @@ static bool virtio_started_needed(void *opaque)
return vdev->started;
}

static bool virtio_disabled_needed(void *opaque)
{
VirtIODevice *vdev = opaque;

return vdev->disabled;
}

static const VMStateDescription vmstate_virtqueue = {
.name = "virtqueue_state",
.version_id = 1,
Expand Down Expand Up @@ -2724,6 +2732,17 @@ static const VMStateDescription vmstate_virtio_started = {
}
};

static const VMStateDescription vmstate_virtio_disabled = {
.name = "virtio/disabled",
.version_id = 1,
.minimum_version_id = 1,
.needed = &virtio_disabled_needed,
.fields = (VMStateField[]) {
VMSTATE_BOOL(disabled, VirtIODevice),
VMSTATE_END_OF_LIST()
}
};

static const VMStateDescription vmstate_virtio = {
.name = "virtio",
.version_id = 1,
Expand All @@ -2741,6 +2760,7 @@ static const VMStateDescription vmstate_virtio = {
&vmstate_virtio_extra_state,
&vmstate_virtio_started,
&vmstate_virtio_packed_virtqueues,
&vmstate_virtio_disabled,
NULL
}
};
Expand Down Expand Up @@ -3575,6 +3595,7 @@ static void virtio_device_instance_finalize(Object *obj)
static Property virtio_properties[] = {
DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),
DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true),
DEFINE_PROP_BOOL("use-disabled-flag", VirtIODevice, use_disabled_flag, true),
DEFINE_PROP_END_OF_LIST(),
};

Expand Down
15 changes: 15 additions & 0 deletions include/hw/virtio/virtio.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ struct VirtIODevice
uint16_t device_id;
bool vm_running;
bool broken; /* device in invalid state, needs reset */
bool use_disabled_flag; /* allow use of 'disable' flag when needed */
bool disabled; /* device in temporarily disabled state */
bool use_started;
bool started;
bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
Expand Down Expand Up @@ -380,4 +382,17 @@ static inline void virtio_set_started(VirtIODevice *vdev, bool started)
vdev->started = started;
}
}

static inline void virtio_set_disabled(VirtIODevice *vdev, bool disable)
{
if (vdev->use_disabled_flag) {
vdev->disabled = disable;
}
}

static inline bool virtio_device_disabled(VirtIODevice *vdev)
{
return unlikely(vdev->disabled || vdev->broken);
}

#endif

0 comments on commit cde7cb6

Please sign in to comment.