[libvirt] [RFC PATCH 0/5] hotplug: fix premature rebinding of VFIO devices to host

Michael Roth posted 5 patches 116 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1498695900-1648-1-git-send-email-mdroth@linux.vnet.ibm.com
src/libvirt_private.syms |   5 ++
src/qemu/qemu_hostdev.c  |  16 +++++
src/qemu/qemu_hostdev.h  |   4 ++
src/qemu/qemu_hotplug.c  |  56 ++++++++++++++----
src/util/virfile.c       |  52 +++++++++++++++++
src/util/virfile.h       |   1 +
src/util/virhostdev.c    | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
src/util/virhostdev.h    |  16 +++++
src/util/virpci.c        |  69 ++++++++++++++++++----
src/util/virpci.h        |   4 ++
10 files changed, 360 insertions(+), 36 deletions(-)

[libvirt] [RFC PATCH 0/5] hotplug: fix premature rebinding of VFIO devices to host

Posted by Michael Roth 116 weeks ago
Hi everyone. Hoping to get some feedback on this approach, or some
alternatives proposed below, to the following issue:

Currently libvirt immediately attempts to rebind a managed device back to the
host driver when it receives a DEVICE_DELETED event from QEMU. This is
problematic for 2 reasons:

1) If multiple devices from a group are attached to a guest, this can move
   the group into a "non-viable" state where some devices are assigned to
   the host and some to the guest.

2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase
   where additional cleanup occurs. In most cases libvirt can ignore this
   cleanup, but in the case of VFIO devices this is where closing of a VFIO
   group FD occurs, and failing to wait before rebinding the device to the
   host driver can result in unexpected behavior. In the case of powernv
   hosts at least, this can lead to a host driver crashing due to the default
   DMA windows not having been fully-restored yet. The window between this is
   and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've
   seen host dumps with Mellanox CX4 VFs being rebound to host driver during
   this period (on powernv hosts).

Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver
until all the devices in the group have been detached, at which point all
the hostdevs are rebound as a group. Until that point, the devices are traced
by the drvManager's inactiveList in a similar manner to hostdevs that are
assigned to VFIO via the nodedev-detach interface.

Patch 5 addresses 2) by adding an additional check that, when the last device
from a group is detached, polls /proc for open FDs referencing the VFIO group
path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we
time out, we abandon rebinding the hostdevs back to the host.

There are a couple alternatives to Patch 5 that might be worth considering:

a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of
   DEVICE_DELETED. Paired with patches 1-4 this would let us drop patch 5 in
   favor of minimal changes to libvirt's event handlers.

   The downsides are:
    - that we'd incur some latency for all device-detach calls, but it's not
      apparent to me whether this delay is significant for anything outside
      of VFIO.
    - there may be cases where finalization after DEVICE_DELETE/unparent are
      is not guaranteed, and I'm not sure QOM would encourage such
      expectations even if that's currently the case.

b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most
   direct solution. With this we could completely separate out the handling
   of rebinding to host driver based on receival of this event.

   The downsides are:
    - this would only work for newer versions of QEMU, though we could use
      the poll-wait in patch 5 as a fallback.
    - synchronizing sync/async device-detach threads with sync/async
      handlers for this would be a bit hairy, but I have a WIP in progress
      that seems *fairly reasonable*

c) Take the approach in Patch 5, either as a precursor to implementing b) or
   something else, or just sticking with that for now.

d) ???

Personally I'm leaning toward c), but I'm wondering if that's "good enough"
for now, or if we should pursue something more robust from the start, or
perhaps something else entirely?

Any feedback is greatly appreciated!

 src/libvirt_private.syms |   5 ++
 src/qemu/qemu_hostdev.c  |  16 +++++
 src/qemu/qemu_hostdev.h  |   4 ++
 src/qemu/qemu_hotplug.c  |  56 ++++++++++++++----
 src/util/virfile.c       |  52 +++++++++++++++++
 src/util/virfile.h       |   1 +
 src/util/virhostdev.c    | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 src/util/virhostdev.h    |  16 +++++
 src/util/virpci.c        |  69 ++++++++++++++++++----
 src/util/virpci.h        |   4 ++
 10 files changed, 360 insertions(+), 36 deletions(-)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [Qemu-devel] [RFC PATCH 0/5] hotplug: fix premature rebinding of VFIO devices to host

Posted by Daniel P. Berrange 116 weeks ago
On Wed, Jun 28, 2017 at 07:24:55PM -0500, Michael Roth wrote:
> Hi everyone. Hoping to get some feedback on this approach, or some
> alternatives proposed below, to the following issue:
> 
> Currently libvirt immediately attempts to rebind a managed device back to the
> host driver when it receives a DEVICE_DELETED event from QEMU. This is
> problematic for 2 reasons:
> 
> 1) If multiple devices from a group are attached to a guest, this can move
>    the group into a "non-viable" state where some devices are assigned to
>    the host and some to the guest.
> 
> 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase
>    where additional cleanup occurs. In most cases libvirt can ignore this
>    cleanup, but in the case of VFIO devices this is where closing of a VFIO
>    group FD occurs, and failing to wait before rebinding the device to the
>    host driver can result in unexpected behavior. In the case of powernv
>    hosts at least, this can lead to a host driver crashing due to the default
>    DMA windows not having been fully-restored yet. The window between this is
>    and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've
>    seen host dumps with Mellanox CX4 VFs being rebound to host driver during
>    this period (on powernv hosts).

Why on earth does QEMU's device finalization take 6 seconds to complete.
That feels very broken to me, unless QEMU is not being schedled due to
host being overcomitted. If that's not the case, then we have a bug to
investigate in QEMU to find out why cleanup is delayed so long.


From libvirt's POV, we consider 'DEVICE_DELETED' as meaning both that the
frontend has gone *and* the corresponding backend has gone. Aside from
cleaning the VFIO group, we use this as a trigger for all other device
related cleanup like SELinux labelling, cgroup device ACLs, etc. If the
backend is not guaranteed to be closed in QEMU when this emit is emitted
then either QEMU needs to delay the event until it is really cleaned up,
or QEMU needs to add a further event to emit when the backend is clean.


> Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver
> until all the devices in the group have been detached, at which point all
> the hostdevs are rebound as a group. Until that point, the devices are traced
> by the drvManager's inactiveList in a similar manner to hostdevs that are
> assigned to VFIO via the nodedev-detach interface.
> 
> Patch 5 addresses 2) by adding an additional check that, when the last device
> from a group is detached, polls /proc for open FDs referencing the VFIO group
> path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we
> time out, we abandon rebinding the hostdevs back to the host.

That is just gross - it is tieing libvirt to details of the QEMU internal
implementation. I really don't think we should be doing that. So NACK to
this from my POV.

> There are a couple alternatives to Patch 5 that might be worth considering:
> 
> a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of
>    DEVICE_DELETED. Paired with patches 1-4 this would let us drop patch 5 in
>    favor of minimal changes to libvirt's event handlers.
> 
>    The downsides are:
>     - that we'd incur some latency for all device-detach calls, but it's not
>       apparent to me whether this delay is significant for anything outside
>       of VFIO.
>     - there may be cases where finalization after DEVICE_DELETE/unparent are
>       is not guaranteed, and I'm not sure QOM would encourage such
>       expectations even if that's currently the case.
> 
> b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most
>    direct solution. With this we could completely separate out the handling
>    of rebinding to host driver based on receival of this event.
> 
>    The downsides are:
>     - this would only work for newer versions of QEMU, though we could use
>       the poll-wait in patch 5 as a fallback.
>     - synchronizing sync/async device-detach threads with sync/async
>       handlers for this would be a bit hairy, but I have a WIP in progress
>       that seems *fairly reasonable*
> 
> c) Take the approach in Patch 5, either as a precursor to implementing b) or
>    something else, or just sticking with that for now.
> 
> d) ???

Fix DEVICE_DELETE so its only emitted when the backend associated with
the device is fully cleaned up.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH 0/5] hotplug: fix premature rebinding of VFIO devices to host

Posted by Michael Roth 116 weeks ago
Quoting Daniel P. Berrange (2017-06-29 03:33:19)
> On Wed, Jun 28, 2017 at 07:24:55PM -0500, Michael Roth wrote:
> > Hi everyone. Hoping to get some feedback on this approach, or some
> > alternatives proposed below, to the following issue:
> > 
> > Currently libvirt immediately attempts to rebind a managed device back to the
> > host driver when it receives a DEVICE_DELETED event from QEMU. This is
> > problematic for 2 reasons:
> > 
> > 1) If multiple devices from a group are attached to a guest, this can move
> >    the group into a "non-viable" state where some devices are assigned to
> >    the host and some to the guest.
> > 
> > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase
> >    where additional cleanup occurs. In most cases libvirt can ignore this
> >    cleanup, but in the case of VFIO devices this is where closing of a VFIO
> >    group FD occurs, and failing to wait before rebinding the device to the
> >    host driver can result in unexpected behavior. In the case of powernv
> >    hosts at least, this can lead to a host driver crashing due to the default
> >    DMA windows not having been fully-restored yet. The window between this is
> >    and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've
> >    seen host dumps with Mellanox CX4 VFs being rebound to host driver during
> >    this period (on powernv hosts).
> 
> Why on earth does QEMU's device finalization take 6 seconds to complete.
> That feels very broken to me, unless QEMU is not being schedled due to
> host being overcomitted. If that's not the case, then we have a bug to
> investigate in QEMU to find out why cleanup is delayed so long.

In this particular case QEMU starts finalization almost immediately
after the DEVICE_DELETED, but it looks like most of the time between that and
closing of the group FD is spent in the kernel. Here's what it looks
like from QEMU with vfio*/monitor* traces enabled (in this case
unplugging a Mellanox CX3 PF):

# device_del called
...
# vfio device's device_unparent() called:
  # unrealize->exit->vfio_exitfn called:
61948@1498759308.951038:vfio_intx_disable  (0002:01:00.0)
61948@1498759308.953657:vfio_region_exit Device 0002:01:00.0, region 0
61948@1498759308.954532:vfio_region_exit Device 0002:01:00.0, region 2
  # unrealize->exit->vfio_exitfn returns, DEVICE_DELETED sent:
61948@1498759308.954633:monitor_protocol_event_queue event=9 data=0x3fff6c508690 rate=0
61948@1498759308.954669:monitor_protocol_event_emit event=9 data=0x3fff6c508690
  # last unref of vfio device, vfio_instance_finalize() called:
61948@1498759308.955660:vfio_region_finalize Device 0002:01:00.0, region 0
61948@1498759308.955742:vfio_region_finalize Device 0002:01:00.0, region 2
61948@1498759308.955751:vfio_put_base_device close vdev->fd=30

    # close(VFIODevice.fd) <- 5 SECOND DELAY

61948@1498759313.140515:vfio_put_base_device_completed close completed vdev->fd=30
61948@1498759313.181124:vfio_disconnect_container close container->fd=102
61948@1498759313.181152:vfio_put_group close group->fd=101
    # close(VFIOGroup.fd)
61948@1498759313.181157:vfio_put_group_completed close completed group->fd=101
  # vfio_instance_finalize() returns
# vfio device's device_unparent() returns

I poked around in the VFIO group close path and figured restoring ownership
of IOMMU to the host via vfio_iommu_driver_ops.release() (via
close(groupfd) was where all the time was spent, but didn't expect it to
be the close(VFIODevice.fd). Maybe Alexey/Alex have a better idea. I'll
look into it more as well.

But suffice to say there's not much QEMU can do about the delay other
than moving deferring the DEVICE_DELETED event or adding a later-stage
event.

> 
> 
> From libvirt's POV, we consider 'DEVICE_DELETED' as meaning both that the
> frontend has gone *and* the corresponding backend has gone. Aside from
> cleaning the VFIO group, we use this as a trigger for all other device
> related cleanup like SELinux labelling, cgroup device ACLs, etc. If the
> backend is not guaranteed to be closed in QEMU when this emit is emitted
> then either QEMU needs to delay the event until it is really cleaned up,
> or QEMU needs to add a further event to emit when the backend is clean.
> 
> 
> > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver
> > until all the devices in the group have been detached, at which point all
> > the hostdevs are rebound as a group. Until that point, the devices are traced
> > by the drvManager's inactiveList in a similar manner to hostdevs that are
> > assigned to VFIO via the nodedev-detach interface.
> > 
> > Patch 5 addresses 2) by adding an additional check that, when the last device
> > from a group is detached, polls /proc for open FDs referencing the VFIO group
> > path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we
> > time out, we abandon rebinding the hostdevs back to the host.
> 
> That is just gross - it is tieing libvirt to details of the QEMU internal
> implementation. I really don't think we should be doing that. So NACK to
> this from my POV.
> 
> > There are a couple alternatives to Patch 5 that might be worth considering:
> > 
> > a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of
> >    DEVICE_DELETED. Paired with patches 1-4 this would let us drop patch 5 in
> >    favor of minimal changes to libvirt's event handlers.
> > 
> >    The downsides are:
> >     - that we'd incur some latency for all device-detach calls, but it's not
> >       apparent to me whether this delay is significant for anything outside
> >       of VFIO.
> >     - there may be cases where finalization after DEVICE_DELETE/unparent are
> >       is not guaranteed, and I'm not sure QOM would encourage such
> >       expectations even if that's currently the case.
> > 
> > b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most
> >    direct solution. With this we could completely separate out the handling
> >    of rebinding to host driver based on receival of this event.
> > 
> >    The downsides are:
> >     - this would only work for newer versions of QEMU, though we could use
> >       the poll-wait in patch 5 as a fallback.
> >     - synchronizing sync/async device-detach threads with sync/async
> >       handlers for this would be a bit hairy, but I have a WIP in progress
> >       that seems *fairly reasonable*
> > 
> > c) Take the approach in Patch 5, either as a precursor to implementing b) or
> >    something else, or just sticking with that for now.
> > 
> > d) ???
> 
> Fix DEVICE_DELETE so its only emitted when the backend associated with
> the device is fully cleaned up.

Need to explore same concerns I had WRT to DEVICE_FINALIZED above, but
assuming those aren't an issue this would indeed makes things even
simpler. Will look into this more.

Would patch 5 be out of the question even as a fallback for downlevel
QEMUs? Or is that scenario too unlikely to warrant it?

Thanks!

> 
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 


Re: [libvirt] [RFC PATCH 0/5] hotplug: fix premature rebinding of VFIO devices to host

Posted by Alex Williamson 116 weeks ago
On Thu, 29 Jun 2017 13:22:44 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> Quoting Daniel P. Berrange (2017-06-29 03:33:19)
> > On Wed, Jun 28, 2017 at 07:24:55PM -0500, Michael Roth wrote:  
> > > Hi everyone. Hoping to get some feedback on this approach, or some
> > > alternatives proposed below, to the following issue:
> > > 
> > > Currently libvirt immediately attempts to rebind a managed device back to the
> > > host driver when it receives a DEVICE_DELETED event from QEMU. This is
> > > problematic for 2 reasons:
> > > 
> > > 1) If multiple devices from a group are attached to a guest, this can move
> > >    the group into a "non-viable" state where some devices are assigned to
> > >    the host and some to the guest.
> > > 
> > > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase
> > >    where additional cleanup occurs. In most cases libvirt can ignore this
> > >    cleanup, but in the case of VFIO devices this is where closing of a VFIO
> > >    group FD occurs, and failing to wait before rebinding the device to the
> > >    host driver can result in unexpected behavior. In the case of powernv
> > >    hosts at least, this can lead to a host driver crashing due to the default
> > >    DMA windows not having been fully-restored yet. The window between this is
> > >    and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've
> > >    seen host dumps with Mellanox CX4 VFs being rebound to host driver during
> > >    this period (on powernv hosts).  
> > 
> > Why on earth does QEMU's device finalization take 6 seconds to complete.
> > That feels very broken to me, unless QEMU is not being schedled due to
> > host being overcomitted. If that's not the case, then we have a bug to
> > investigate in QEMU to find out why cleanup is delayed so long.  
> 
> In this particular case QEMU starts finalization almost immediately
> after the DEVICE_DELETED, but it looks like most of the time between that and
> closing of the group FD is spent in the kernel. Here's what it looks
> like from QEMU with vfio*/monitor* traces enabled (in this case
> unplugging a Mellanox CX3 PF):
> 
> # device_del called
> ...
> # vfio device's device_unparent() called:
>   # unrealize->exit->vfio_exitfn called:
> 61948@1498759308.951038:vfio_intx_disable  (0002:01:00.0)
> 61948@1498759308.953657:vfio_region_exit Device 0002:01:00.0, region 0
> 61948@1498759308.954532:vfio_region_exit Device 0002:01:00.0, region 2
>   # unrealize->exit->vfio_exitfn returns, DEVICE_DELETED sent:
> 61948@1498759308.954633:monitor_protocol_event_queue event=9 data=0x3fff6c508690 rate=0
> 61948@1498759308.954669:monitor_protocol_event_emit event=9 data=0x3fff6c508690
>   # last unref of vfio device, vfio_instance_finalize() called:
> 61948@1498759308.955660:vfio_region_finalize Device 0002:01:00.0, region 0
> 61948@1498759308.955742:vfio_region_finalize Device 0002:01:00.0, region 2
> 61948@1498759308.955751:vfio_put_base_device close vdev->fd=30
> 
>     # close(VFIODevice.fd) <- 5 SECOND DELAY
> 
> 61948@1498759313.140515:vfio_put_base_device_completed close completed vdev->fd=30
> 61948@1498759313.181124:vfio_disconnect_container close container->fd=102
> 61948@1498759313.181152:vfio_put_group close group->fd=101
>     # close(VFIOGroup.fd)
> 61948@1498759313.181157:vfio_put_group_completed close completed group->fd=101
>   # vfio_instance_finalize() returns
> # vfio device's device_unparent() returns
> 
> I poked around in the VFIO group close path and figured restoring ownership
> of IOMMU to the host via vfio_iommu_driver_ops.release() (via
> close(groupfd) was where all the time was spent, but didn't expect it to
> be the close(VFIODevice.fd). Maybe Alexey/Alex have a better idea. I'll
> look into it more as well.
> 
> But suffice to say there's not much QEMU can do about the delay other
> than moving deferring the DEVICE_DELETED event or adding a later-stage
> event.

The device close needs to do a device reset and attempt to restore the
state of the device, plus whatever that EEH code is doing.  I would
have expected the group close to teardown the IOMMU, but maybe I'm
forgetting some subtlety, in either case it's going to happen as part
of the finalize and it's going to take some time.  Thanks,

Alex
 
> > From libvirt's POV, we consider 'DEVICE_DELETED' as meaning both that the
> > frontend has gone *and* the corresponding backend has gone. Aside from
> > cleaning the VFIO group, we use this as a trigger for all other device
> > related cleanup like SELinux labelling, cgroup device ACLs, etc. If the
> > backend is not guaranteed to be closed in QEMU when this emit is emitted
> > then either QEMU needs to delay the event until it is really cleaned up,
> > or QEMU needs to add a further event to emit when the backend is clean.
> > 
> >   
> > > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver
> > > until all the devices in the group have been detached, at which point all
> > > the hostdevs are rebound as a group. Until that point, the devices are traced
> > > by the drvManager's inactiveList in a similar manner to hostdevs that are
> > > assigned to VFIO via the nodedev-detach interface.
> > > 
> > > Patch 5 addresses 2) by adding an additional check that, when the last device
> > > from a group is detached, polls /proc for open FDs referencing the VFIO group
> > > path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we
> > > time out, we abandon rebinding the hostdevs back to the host.  
> > 
> > That is just gross - it is tieing libvirt to details of the QEMU internal
> > implementation. I really don't think we should be doing that. So NACK to
> > this from my POV.
> >   
> > > There are a couple alternatives to Patch 5 that might be worth considering:
> > > 
> > > a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of
> > >    DEVICE_DELETED. Paired with patches 1-4 this would let us drop patch 5 in
> > >    favor of minimal changes to libvirt's event handlers.
> > > 
> > >    The downsides are:
> > >     - that we'd incur some latency for all device-detach calls, but it's not
> > >       apparent to me whether this delay is significant for anything outside
> > >       of VFIO.
> > >     - there may be cases where finalization after DEVICE_DELETE/unparent are
> > >       is not guaranteed, and I'm not sure QOM would encourage such
> > >       expectations even if that's currently the case.
> > > 
> > > b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most
> > >    direct solution. With this we could completely separate out the handling
> > >    of rebinding to host driver based on receival of this event.
> > > 
> > >    The downsides are:
> > >     - this would only work for newer versions of QEMU, though we could use
> > >       the poll-wait in patch 5 as a fallback.
> > >     - synchronizing sync/async device-detach threads with sync/async
> > >       handlers for this would be a bit hairy, but I have a WIP in progress
> > >       that seems *fairly reasonable*
> > > 
> > > c) Take the approach in Patch 5, either as a precursor to implementing b) or
> > >    something else, or just sticking with that for now.
> > > 
> > > d) ???  
> > 
> > Fix DEVICE_DELETE so its only emitted when the backend associated with
> > the device is fully cleaned up.  
> 
> Need to explore same concerns I had WRT to DEVICE_FINALIZED above, but
> assuming those aren't an issue this would indeed makes things even
> simpler. Will look into this more.
> 
> Would patch 5 be out of the question even as a fallback for downlevel
> QEMUs? Or is that scenario too unlikely to warrant it?
> 
> Thanks!
> 
> > 
> > 
> > Regards,
> > Daniel
> > -- 
> > |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> > |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> > |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> >   
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 0/5] hotplug: fix premature rebinding of VFIO devices to host

Posted by Alex Williamson 116 weeks ago
On Thu, 29 Jun 2017 09:33:19 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Wed, Jun 28, 2017 at 07:24:55PM -0500, Michael Roth wrote:
> > Hi everyone. Hoping to get some feedback on this approach, or some
> > alternatives proposed below, to the following issue:
> > 
> > Currently libvirt immediately attempts to rebind a managed device back to the
> > host driver when it receives a DEVICE_DELETED event from QEMU. This is
> > problematic for 2 reasons:
> > 
> > 1) If multiple devices from a group are attached to a guest, this can move
> >    the group into a "non-viable" state where some devices are assigned to
> >    the host and some to the guest.
> > 
> > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase
> >    where additional cleanup occurs. In most cases libvirt can ignore this
> >    cleanup, but in the case of VFIO devices this is where closing of a VFIO
> >    group FD occurs, and failing to wait before rebinding the device to the
> >    host driver can result in unexpected behavior. In the case of powernv
> >    hosts at least, this can lead to a host driver crashing due to the default
> >    DMA windows not having been fully-restored yet. The window between this is
> >    and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've
> >    seen host dumps with Mellanox CX4 VFs being rebound to host driver during
> >    this period (on powernv hosts).  

I've been trying to tackle this from the kernel side too, currently
Linux's driver model really neither allows vfio bus drivers to nak
unbinding a device from an in-use group nor nak binding a device
from an in-use group to an incompatible driver.  The issue you identify
in QEMU/libvirt exacerbates the problem as QEMU has not yet finalized
the device/group references before libvirt tries to unbind the device
from the vfio bus driver and attach it to a host driver.  I'd love to
solve this from both sides by allowing the kernel to prevent driver
binds that we'd consider compromising and also introduce a bit of
patience in the QEMU/libvirt path to avoid the kernel needing to impose
that driver blocking.

> Why on earth does QEMU's device finalization take 6 seconds to complete.
> That feels very broken to me, unless QEMU is not being schedled due to
> host being overcomitted. If that's not the case, then we have a bug to
> investigate in QEMU to find out why cleanup is delayed so long.

I wouldn't necessarily jump to the conclusion that this is a bug, if
it's relating to tearing down the IOMMU mappings for the device, gigs
of mappings can take non-trivial time.  Is that the scenario here?  Is
that 6s somehow proportional to guest memory size?
 
> From libvirt's POV, we consider 'DEVICE_DELETED' as meaning both that the
> frontend has gone *and* the corresponding backend has gone. Aside from
> cleaning the VFIO group, we use this as a trigger for all other device
> related cleanup like SELinux labelling, cgroup device ACLs, etc. If the
> backend is not guaranteed to be closed in QEMU when this emit is emitted
> then either QEMU needs to delay the event until it is really cleaned up,
> or QEMU needs to add a further event to emit when the backend is clean.

Clearly libvirt and QEMU's idea of what DEVICE_DELETED means don't
align.

> > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver
> > until all the devices in the group have been detached, at which point all
> > the hostdevs are rebound as a group. Until that point, the devices are traced
> > by the drvManager's inactiveList in a similar manner to hostdevs that are
> > assigned to VFIO via the nodedev-detach interface.

There are certainly some benefits to group-awareness here, currently
an admin user like libvirt can trigger a BUG_ON by trying to bind a
device back to a host driver when a group is still in use, at best we
might improve that to rejecting the compromising bind.  

> > Patch 5 addresses 2) by adding an additional check that, when the last device
> > from a group is detached, polls /proc for open FDs referencing the VFIO group
> > path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we
> > time out, we abandon rebinding the hostdevs back to the host.  
> 
> That is just gross - it is tieing libvirt to details of the QEMU internal
> implementation. I really don't think we should be doing that. So NACK to
> this from my POV.

It seems a little silly for QEMU to emit the event while it's still in
use, clearly emitting the event at the right point would negate any
need for snooping around in proc.

> > There are a couple alternatives to Patch 5 that might be worth considering:
> > 
> > a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of
> >    DEVICE_DELETED. Paired with patches 1-4 this would let us drop patch 5 in
> >    favor of minimal changes to libvirt's event handlers.
> > 
> >    The downsides are:
> >     - that we'd incur some latency for all device-detach calls, but it's not
> >       apparent to me whether this delay is significant for anything outside
> >       of VFIO.
> >     - there may be cases where finalization after DEVICE_DELETE/unparent are
> >       is not guaranteed, and I'm not sure QOM would encourage such
> >       expectations even if that's currently the case.
> > 
> > b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most
> >    direct solution. With this we could completely separate out the handling
> >    of rebinding to host driver based on receival of this event.
> > 
> >    The downsides are:
> >     - this would only work for newer versions of QEMU, though we could use
> >       the poll-wait in patch 5 as a fallback.
> >     - synchronizing sync/async device-detach threads with sync/async
> >       handlers for this would be a bit hairy, but I have a WIP in progress
> >       that seems *fairly reasonable*
> > 
> > c) Take the approach in Patch 5, either as a precursor to implementing b) or
> >    something else, or just sticking with that for now.
> > 
> > d) ???  
> 
> Fix DEVICE_DELETE so its only emitted when the backend associated with
> the device is fully cleaned up.

Adding a FINALIZE seems to require a two-step fix, fix QEMU then fix
libvirt, whereas moving DELETE to the correct location automatically
fixes the behavior with existing libvirt.  I don't know that a
GROUP_DELETED makes much sense, libvirt can know about groups on its
own and it just leads to a vfio specific path.  Thanks,

Alex

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 0/5] hotplug: fix premature rebinding of VFIO devices to host

Posted by Michael Roth 116 weeks ago
Quoting Alex Williamson (2017-06-29 14:28:11)
> On Thu, 29 Jun 2017 09:33:19 +0100
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > On Wed, Jun 28, 2017 at 07:24:55PM -0500, Michael Roth wrote:
> > > Hi everyone. Hoping to get some feedback on this approach, or some
> > > alternatives proposed below, to the following issue:
> > > 
> > > Currently libvirt immediately attempts to rebind a managed device back to the
> > > host driver when it receives a DEVICE_DELETED event from QEMU. This is
> > > problematic for 2 reasons:
> > > 
> > > 1) If multiple devices from a group are attached to a guest, this can move
> > >    the group into a "non-viable" state where some devices are assigned to
> > >    the host and some to the guest.
> > > 
> > > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase
> > >    where additional cleanup occurs. In most cases libvirt can ignore this
> > >    cleanup, but in the case of VFIO devices this is where closing of a VFIO
> > >    group FD occurs, and failing to wait before rebinding the device to the
> > >    host driver can result in unexpected behavior. In the case of powernv
> > >    hosts at least, this can lead to a host driver crashing due to the default
> > >    DMA windows not having been fully-restored yet. The window between this is
> > >    and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've
> > >    seen host dumps with Mellanox CX4 VFs being rebound to host driver during
> > >    this period (on powernv hosts).  
> 
> I've been trying to tackle this from the kernel side too, currently
> Linux's driver model really neither allows vfio bus drivers to nak
> unbinding a device from an in-use group nor nak binding a device
> from an in-use group to an incompatible driver.  The issue you identify
> in QEMU/libvirt exacerbates the problem as QEMU has not yet finalized
> the device/group references before libvirt tries to unbind the device
> from the vfio bus driver and attach it to a host driver.  I'd love to
> solve this from both sides by allowing the kernel to prevent driver
> binds that we'd consider compromising and also introduce a bit of
> patience in the QEMU/libvirt path to avoid the kernel needing to impose
> that driver blocking.

Perfect, I'd meant to ask about this and it skipped my mind. I had some
concerns about the fact that even with these qemu/libvirt changes in place
we could still trigger these sorts of crashes by issuing such a rebind
outside of libvirt, so good to know that aspect is being looked at as well
and that the 2 aren't in conflict.

> 
> > Why on earth does QEMU's device finalization take 6 seconds to complete.
> > That feels very broken to me, unless QEMU is not being schedled due to
> > host being overcomitted. If that's not the case, then we have a bug to
> > investigate in QEMU to find out why cleanup is delayed so long.
> 
> I wouldn't necessarily jump to the conclusion that this is a bug, if
> it's relating to tearing down the IOMMU mappings for the device, gigs
> of mappings can take non-trivial time.  Is that the scenario here?  Is
> that 6s somehow proportional to guest memory size?

I think you're right that the teardown is via group close. Since the traces
seem to pin it on the vfio device FD close, the device reset you mentioned
is probably what's going on here, but I'll try to confirm.

> 
> > From libvirt's POV, we consider 'DEVICE_DELETED' as meaning both that the
> > frontend has gone *and* the corresponding backend has gone. Aside from
> > cleaning the VFIO group, we use this as a trigger for all other device
> > related cleanup like SELinux labelling, cgroup device ACLs, etc. If the
> > backend is not guaranteed to be closed in QEMU when this emit is emitted
> > then either QEMU needs to delay the event until it is really cleaned up,
> > or QEMU needs to add a further event to emit when the backend is clean.
> 
> Clearly libvirt and QEMU's idea of what DEVICE_DELETED means don't
> align.
> 
> > > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver
> > > until all the devices in the group have been detached, at which point all
> > > the hostdevs are rebound as a group. Until that point, the devices are traced
> > > by the drvManager's inactiveList in a similar manner to hostdevs that are
> > > assigned to VFIO via the nodedev-detach interface.
> 
> There are certainly some benefits to group-awareness here, currently
> an admin user like libvirt can trigger a BUG_ON by trying to bind a
> device back to a host driver when a group is still in use, at best we
> might improve that to rejecting the compromising bind.  
> 
> > > Patch 5 addresses 2) by adding an additional check that, when the last device
> > > from a group is detached, polls /proc for open FDs referencing the VFIO group
> > > path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we
> > > time out, we abandon rebinding the hostdevs back to the host.  
> > 
> > That is just gross - it is tieing libvirt to details of the QEMU internal
> > implementation. I really don't think we should be doing that. So NACK to
> > this from my POV.
> 
> It seems a little silly for QEMU to emit the event while it's still in
> use, clearly emitting the event at the right point would negate any
> need for snooping around in proc.
> 
> > > There are a couple alternatives to Patch 5 that might be worth considering:
> > > 
> > > a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of
> > >    DEVICE_DELETED. Paired with patches 1-4 this would let us drop patch 5 in
> > >    favor of minimal changes to libvirt's event handlers.
> > > 
> > >    The downsides are:
> > >     - that we'd incur some latency for all device-detach calls, but it's not
> > >       apparent to me whether this delay is significant for anything outside
> > >       of VFIO.
> > >     - there may be cases where finalization after DEVICE_DELETE/unparent are
> > >       is not guaranteed, and I'm not sure QOM would encourage such
> > >       expectations even if that's currently the case.
> > > 
> > > b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most
> > >    direct solution. With this we could completely separate out the handling
> > >    of rebinding to host driver based on receival of this event.
> > > 
> > >    The downsides are:
> > >     - this would only work for newer versions of QEMU, though we could use
> > >       the poll-wait in patch 5 as a fallback.
> > >     - synchronizing sync/async device-detach threads with sync/async
> > >       handlers for this would be a bit hairy, but I have a WIP in progress
> > >       that seems *fairly reasonable*
> > > 
> > > c) Take the approach in Patch 5, either as a precursor to implementing b) or
> > >    something else, or just sticking with that for now.
> > > 
> > > d) ???  
> > 
> > Fix DEVICE_DELETE so its only emitted when the backend associated with
> > the device is fully cleaned up.
> 
> Adding a FINALIZE seems to require a two-step fix, fix QEMU then fix
> libvirt, whereas moving DELETE to the correct location automatically
> fixes the behavior with existing libvirt.  I don't know that a
> GROUP_DELETED makes much sense, libvirt can know about groups on its
> own and it just leads to a vfio specific path.  Thanks,

Ok, seems like there's a consensus there. I'll report back if there's
any obvious showstopper with this, but otherwise I'll work on a patch to
defer the DEVICE_DELETED, and drop patch 5 from the libvirt side of
things.

Thanks!

> 
> Alex
> 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 0/5] hotplug: fix premature rebinding of VFIO devices to host

Posted by Laine Stump 116 weeks ago
On 06/28/2017 08:24 PM, Michael Roth wrote:
> Hi everyone. Hoping to get some feedback on this approach, or some
> alternatives proposed below, to the following issue:
> 
> Currently libvirt immediately attempts to rebind a managed device back to the
> host driver when it receives a DEVICE_DELETED event from QEMU. This is
> problematic for 2 reasons:
> 
> 1) If multiple devices from a group are attached to a guest, this can move
>    the group into a "non-viable" state where some devices are assigned to
>    the host and some to the guest.

Since we don't support hotplug with managed='yes' of individual (or
multiple) functions of a multifunction host device, I don't know that
it's very useful to support hot *un*plug of it - it would only be useful
if the multi-function device were present in the guest when it was
started, and then was hot-unplugged later. And this is all a lot of
extra complexity, though, so it would be useful to know what are the
scenarios where it would actually be used (i.e. is this a legitimate
need, or just an interesting exercise?)

> 
> 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase
>    where additional cleanup occurs. In most cases libvirt can ignore this
>    cleanup, but in the case of VFIO devices this is where closing of a VFIO
>    group FD occurs, and failing to wait before rebinding the device to the
>    host driver can result in unexpected behavior. In the case of powernv
>    hosts at least, this can lead to a host driver crashing due to the default
>    DMA windows not having been fully-restored yet. The window between this is
>    and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've
>    seen host dumps with Mellanox CX4 VFs being rebound to host driver during
>    this period (on powernv hosts).

I agree with Dan that the situation described here should be considered
a qemu bug - according to my understanding (from back at the time
DEVICE_DELETED was added to qemu (I think at libvirt's request) qemu
should never emit the DEVICE_DELETED event until *everything* related to
the device is finished - that was the whole point of adding the event in
the first palce. Covering up this bug with a bunch of extra libvirt
complexity is just creating the potential for even more bugs in the more
complex code.

> 
> Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver
> until all the devices in the group have been detached, at which point all
> the hostdevs are rebound as a group. Until that point, the devices are traced
> by the drvManager's inactiveList in a similar manner to hostdevs that are
> assigned to VFIO via the nodedev-detach interface.

What happens if libvirtd is restarted during this period? Is the
inactiveList rebuilt with all the info necessary to assure that the
nodedev-reattach does/doesn't happen (as appropriate) for all devices?


> 
> Patch 5 addresses 2) by adding an additional check that, when the last device
> from a group is detached, polls /proc for open FDs referencing the VFIO group
> path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we
> time out, we abandon rebinding the hostdevs back to the host.
> 
> There are a couple alternatives to Patch 5 that might be worth considering:
> 
> a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of
>    DEVICE_DELETED.

Is the "device is *almost* completely deleted" event (current
DEVIE_DELETED) really something that anyone wants/needs to know about?
Or is the only useful event just the one that you're calling
DEVICE_FINALIZED? If the latter, then I think it would be better to just
change when DEVICE_DELETED is emitted.

 Paired with patches 1-4 this would let us drop patch 5 in
>    favor of minimal changes to libvirt's event handlers.
> 
>    The downsides are:
>     - that we'd incur some latency for all device-detach calls, but it's not
>       apparent to me whether this delay is significant for anything outside
>       of VFIO.
>     - there may be cases where finalization after DEVICE_DELETE/unparent are
>       is not guaranteed, and I'm not sure QOM would encourage such
>       expectations even if that's currently the case.
> 
> b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most
>    direct solution. With this we could completely separate out the handling
>    of rebinding to host driver based on receival of this event.
> 
>    The downsides are:
>     - this would only work for newer versions of QEMU, though we could use
>       the poll-wait in patch 5 as a fallback.

I don't think we should add such a complex patch as a fallback to
support older versions of qemu that don't have the bug fixed. Instead,
just tell people to upgrade qemu (after all, they're going to need to
update *something* (either libvirt or qemu); no need to update libvirt
just in order to avoid updating qemu).


>     - synchronizing sync/async device-detach threads with sync/async
>       handlers for this would be a bit hairy, but I have a WIP in progress
>       that seems *fairly reasonable*
> 
> c) Take the approach in Patch 5, either as a precursor to implementing b) or
>    something else, or just sticking with that for now.
> 
> d) ???
> 
> Personally I'm leaning toward c), but I'm wondering if that's "good enough"
> for now, or if we should pursue something more robust from the start, or
> perhaps something else entirely?
> 
> Any feedback is greatly appreciated!
> 
>  src/libvirt_private.syms |   5 ++
>  src/qemu/qemu_hostdev.c  |  16 +++++
>  src/qemu/qemu_hostdev.h  |   4 ++
>  src/qemu/qemu_hotplug.c  |  56 ++++++++++++++----
>  src/util/virfile.c       |  52 +++++++++++++++++
>  src/util/virfile.h       |   1 +
>  src/util/virhostdev.c    | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  src/util/virhostdev.h    |  16 +++++
>  src/util/virpci.c        |  69 ++++++++++++++++++----
>  src/util/virpci.h        |   4 ++
>  10 files changed, 360 insertions(+), 36 deletions(-)
> 
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 0/5] hotplug: fix premature rebinding of VFIO devices to host

Posted by Alex Williamson 116 weeks ago
On Thu, 29 Jun 2017 14:50:15 -0400
Laine Stump <laine@redhat.com> wrote:

> On 06/28/2017 08:24 PM, Michael Roth wrote:
> > Hi everyone. Hoping to get some feedback on this approach, or some
> > alternatives proposed below, to the following issue:
> > 
> > Currently libvirt immediately attempts to rebind a managed device back to the
> > host driver when it receives a DEVICE_DELETED event from QEMU. This is
> > problematic for 2 reasons:
> > 
> > 1) If multiple devices from a group are attached to a guest, this can move
> >    the group into a "non-viable" state where some devices are assigned to
> >    the host and some to the guest.  
> 
> Since we don't support hotplug with managed='yes' of individual (or
> multiple) functions of a multifunction host device, I don't know that
> it's very useful to support hot *un*plug of it - it would only be useful
> if the multi-function device were present in the guest when it was
> started, and then was hot-unplugged later. And this is all a lot of
> extra complexity, though, so it would be useful to know what are the
> scenarios where it would actually be used (i.e. is this a legitimate
> need, or just an interesting exercise?)

This doesn't make sense to me, since when do we not support hotplug
with managed='yes' and how is it prevented?  Also, let's just not talk
about multifunction, a multifunction device does not imply a shared
group, nor does a shared group imply multifunction.  So is it hotplug
of a device which is in a shared group that is not supported, and if so
how?  I think libvirt tries to do the hot-add, but it hits the
non-viable group when it gives it to QEMU.  On hot-remove, I'm pretty
sure libvirt just lets the host crash into the ground by re-binding the
device to the host driver.  If we don't want to support it, that's one
thing, but the current model is more just neglectful than unsupported.


> > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase
> >    where additional cleanup occurs. In most cases libvirt can ignore this
> >    cleanup, but in the case of VFIO devices this is where closing of a VFIO
> >    group FD occurs, and failing to wait before rebinding the device to the
> >    host driver can result in unexpected behavior. In the case of powernv
> >    hosts at least, this can lead to a host driver crashing due to the default
> >    DMA windows not having been fully-restored yet. The window between this is
> >    and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've
> >    seen host dumps with Mellanox CX4 VFs being rebound to host driver during
> >    this period (on powernv hosts).  
> 
> I agree with Dan that the situation described here should be considered
> a qemu bug - according to my understanding (from back at the time
> DEVICE_DELETED was added to qemu (I think at libvirt's request) qemu
> should never emit the DEVICE_DELETED event until *everything* related to
> the device is finished - that was the whole point of adding the event in
> the first palce. Covering up this bug with a bunch of extra libvirt
> complexity is just creating the potential for even more bugs in the more
> complex code.

Agree, but ISTR not everyone thinks that way.  I don't remember the
opposing viewpoint though.  Thanks,

Alex

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [Qemu-devel] [RFC PATCH 0/5] hotplug: fix premature rebinding of VFIO devices to host

Posted by Laine Stump 116 weeks ago
On 06/29/2017 03:44 PM, Alex Williamson wrote:
> On Thu, 29 Jun 2017 14:50:15 -0400
> Laine Stump <laine@redhat.com> wrote:
> 
>> On 06/28/2017 08:24 PM, Michael Roth wrote:
>>> Hi everyone. Hoping to get some feedback on this approach, or some
>>> alternatives proposed below, to the following issue:
>>>
>>> Currently libvirt immediately attempts to rebind a managed device back to the
>>> host driver when it receives a DEVICE_DELETED event from QEMU. This is
>>> problematic for 2 reasons:
>>>
>>> 1) If multiple devices from a group are attached to a guest, this can move
>>>    the group into a "non-viable" state where some devices are assigned to
>>>    the host and some to the guest.  
>>
>> Since we don't support hotplug with managed='yes' of individual (or
>> multiple) functions of a multifunction host device, I don't know that
>> it's very useful to support hot *un*plug of it - it would only be useful
>> if the multi-function device were present in the guest when it was
>> started, and then was hot-unplugged later. And this is all a lot of
>> extra complexity, though, so it would be useful to know what are the
>> scenarios where it would actually be used (i.e. is this a legitimate
>> need, or just an interesting exercise?)
> 
> This doesn't make sense to me, since when do we not support hotplug
> with managed='yes' and how is it prevented?

Wait a minute. I wasn't thinking straight, got sidetracked into a
different problem, and completely screwed up what I was trying to say.

So basically forget everything in that paragraph :-/

What I started out to say was simply that we didn't support
managed='yes' hotplugging of a single device that's in an iommu group
containing multiple devices (and that led me to wander into the topic of
multifunction devices, which as you say is really unrelated). We don't
do anything specifically to prevent it, but as you say, it will fail
when it gets to qemu.

At one time someone talked about having libvirt automatically detach the
other devices in the same group at times like this, but that was
rightfully detected as ridiculous (it may have been me talking to
myself, and I realized the stupidity of the idea before I managed to
repeat it to anyone else).

>  Also, let's just not talk
> about multifunction, a multifunction device does not imply a shared
> group, nor does a shared group imply multifunction. 

Yeah, that was me being distracted while thinking about the situation
and mistakenly conflating two different problems.

> So is it hotplug
> of a device which is in a shared group that is not supported, and if so
> how?

"not supported" != "actively prevented", it just means "there's no way
to successfully do that"

>  I think libvirt tries to do the hot-add, but it hits the
> non-viable group when it gives it to QEMU.

Correct.


> On hot-remove, I'm pretty
> sure libvirt just lets the host crash into the ground by re-binding the
> device to the host driver.

I haven't tried, but you're likely right.

>  If we don't want to support it, that's one
> thing, but the current model is more just neglectful than unsupported.

Semantics. Potato, Potahto. The end result is "it doesn't work". If
we're going to try to fix one, we should try to fix the other too.

> 
> 
>>> 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase
>>>    where additional cleanup occurs. In most cases libvirt can ignore this
>>>    cleanup, but in the case of VFIO devices this is where closing of a VFIO
>>>    group FD occurs, and failing to wait before rebinding the device to the
>>>    host driver can result in unexpected behavior. In the case of powernv
>>>    hosts at least, this can lead to a host driver crashing due to the default
>>>    DMA windows not having been fully-restored yet. The window between this is
>>>    and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've
>>>    seen host dumps with Mellanox CX4 VFs being rebound to host driver during
>>>    this period (on powernv hosts).  
>>
>> I agree with Dan that the situation described here should be considered
>> a qemu bug - according to my understanding (from back at the time
>> DEVICE_DELETED was added to qemu (I think at libvirt's request) qemu
>> should never emit the DEVICE_DELETED event until *everything* related to
>> the device is finished - that was the whole point of adding the event in
>> the first palce. Covering up this bug with a bunch of extra libvirt
>> complexity is just creating the potential for even more bugs in the more
>> complex code.
> 
> Agree, but ISTR not everyone thinks that way.  I don't remember the
> opposing viewpoint though.

Possible. I only retain about 1% of what goes by the screen, and even
most of that is gone within 6 months. I thought I recalled that there
was no DEVICE_DELETED event before libvirt requested it, so it would
make sense for it to behave according to what libvirt needs (assuming
nobody else is using it). That may be a narcissistic and incorrect view
though.


Re: [libvirt] [RFC PATCH 0/5] hotplug: fix premature rebinding of VFIO devices to host

Posted by Michael Roth 116 weeks ago
Quoting Laine Stump (2017-06-29 13:50:15)
> On 06/28/2017 08:24 PM, Michael Roth wrote:
> > Hi everyone. Hoping to get some feedback on this approach, or some
> > alternatives proposed below, to the following issue:
> > 
> > Currently libvirt immediately attempts to rebind a managed device back to the
> > host driver when it receives a DEVICE_DELETED event from QEMU. This is
> > problematic for 2 reasons:
> > 
> > 1) If multiple devices from a group are attached to a guest, this can move
> >    the group into a "non-viable" state where some devices are assigned to
> >    the host and some to the guest.
> 
> Since we don't support hotplug with managed='yes' of individual (or
> multiple) functions of a multifunction host device, I don't know that
> it's very useful to support hot *un*plug of it - it would only be useful
> if the multi-function device were present in the guest when it was
> started, and then was hot-unplugged later. And this is all a lot of
> extra complexity, though, so it would be useful to know what are the
> scenarios where it would actually be used (i.e. is this a legitimate
> need, or just an interesting exercise?)
> 
> > 
> > 2) When QEMU emits the DEVICE_DELETED event, there's still a "finalize" phase
> >    where additional cleanup occurs. In most cases libvirt can ignore this
> >    cleanup, but in the case of VFIO devices this is where closing of a VFIO
> >    group FD occurs, and failing to wait before rebinding the device to the
> >    host driver can result in unexpected behavior. In the case of powernv
> >    hosts at least, this can lead to a host driver crashing due to the default
> >    DMA windows not having been fully-restored yet. The window between this is
> >    and the initial DEVICE_DELETED seems to be ~6 seconds in practice. We've
> >    seen host dumps with Mellanox CX4 VFs being rebound to host driver during
> >    this period (on powernv hosts).
> 
> I agree with Dan that the situation described here should be considered
> a qemu bug - according to my understanding (from back at the time
> DEVICE_DELETED was added to qemu (I think at libvirt's request) qemu
> should never emit the DEVICE_DELETED event until *everything* related to
> the device is finished - that was the whole point of adding the event in
> the first palce. Covering up this bug with a bunch of extra libvirt
> complexity is just creating the potential for even more bugs in the more
> complex code.
> 
> > 
> > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver
> > until all the devices in the group have been detached, at which point all
> > the hostdevs are rebound as a group. Until that point, the devices are traced
> > by the drvManager's inactiveList in a similar manner to hostdevs that are
> > assigned to VFIO via the nodedev-detach interface.
> 
> What happens if libvirtd is restarted during this period? Is the
> inactiveList rebuilt with all the info necessary to assure that the
> nodedev-reattach does/doesn't happen (as appropriate) for all devices?

Hmm, good question.

The Unbindable() check only needs to know that nothing in the activeList
belongs to the group we're checking, and that list at least seems to get
rebuilt appropriately on restart.

But the Unbind() relies on inactiveList and the behavior there is what
nodedev-detach currently does, which is to add it to inactive list while
libvirtd is running, and then just forget about it when libvirtd restarts.
For nodedev-detach it's fine since virHostdevPreparePCIDevices() re-adds
it as needed in the device-attach path. But yah, for this purpose it ends
up losing track of hostdevs that are still pending rebind to the host, and
that means some devices may not get rebound at the appropriate time if
there was a libvirtd restart.

Unlike with device-attach, we can't just re-add it on-demand because we
actually need to know whether or not it was previously in the list. So
I think we'd need to add some persistent state to track this. Will look
into adding handling for that.

> 
> 
> > 
> > Patch 5 addresses 2) by adding an additional check that, when the last device
> > from a group is detached, polls /proc for open FDs referencing the VFIO group
> > path in /dev/vfio/<iommu_group> and waiting for the FD to be closed. If we
> > time out, we abandon rebinding the hostdevs back to the host.
> > 
> > There are a couple alternatives to Patch 5 that might be worth considering:
> > 
> > a) Add a DEVICE_FINALIZED event to QEMU and wait for that instead of
> >    DEVICE_DELETED.
> 
> Is the "device is *almost* completely deleted" event (current
> DEVIE_DELETED) really something that anyone wants/needs to know about?
> Or is the only useful event just the one that you're calling
> DEVICE_FINALIZED? If the latter, then I think it would be better to just
> change when DEVICE_DELETED is emitted.
> 
>  Paired with patches 1-4 this would let us drop patch 5 in
> >    favor of minimal changes to libvirt's event handlers.
> > 
> >    The downsides are:
> >     - that we'd incur some latency for all device-detach calls, but it's not
> >       apparent to me whether this delay is significant for anything outside
> >       of VFIO.
> >     - there may be cases where finalization after DEVICE_DELETE/unparent are
> >       is not guaranteed, and I'm not sure QOM would encourage such
> >       expectations even if that's currently the case.
> > 
> > b) Add a GROUP_DELETED event to VFIO's finalize callback. This is the most
> >    direct solution. With this we could completely separate out the handling
> >    of rebinding to host driver based on receival of this event.
> > 
> >    The downsides are:
> >     - this would only work for newer versions of QEMU, though we could use
> >       the poll-wait in patch 5 as a fallback.
> 
> I don't think we should add such a complex patch as a fallback to
> support older versions of qemu that don't have the bug fixed. Instead,
> just tell people to upgrade qemu (after all, they're going to need to
> update *something* (either libvirt or qemu); no need to update libvirt
> just in order to avoid updating qemu).

Ok, makes sense.

Thanks!

> 
> 
> >     - synchronizing sync/async device-detach threads with sync/async
> >       handlers for this would be a bit hairy, but I have a WIP in progress
> >       that seems *fairly reasonable*
> > 
> > c) Take the approach in Patch 5, either as a precursor to implementing b) or
> >    something else, or just sticking with that for now.
> > 
> > d) ???
> > 
> > Personally I'm leaning toward c), but I'm wondering if that's "good enough"
> > for now, or if we should pursue something more robust from the start, or
> > perhaps something else entirely?
> > 
> > Any feedback is greatly appreciated!
> > 
> >  src/libvirt_private.syms |   5 ++
> >  src/qemu/qemu_hostdev.c  |  16 +++++
> >  src/qemu/qemu_hostdev.h  |   4 ++
> >  src/qemu/qemu_hotplug.c  |  56 ++++++++++++++----
> >  src/util/virfile.c       |  52 +++++++++++++++++
> >  src/util/virfile.h       |   1 +
> >  src/util/virhostdev.c    | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  src/util/virhostdev.h    |  16 +++++
> >  src/util/virpci.c        |  69 ++++++++++++++++++----
> >  src/util/virpci.h        |   4 ++
> >  10 files changed, 360 insertions(+), 36 deletions(-)
> > 
> > 
> 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 0/5] hotplug: fix premature rebinding of VFIO devices to host

Posted by Andrea Bolognani 116 weeks ago
On Thu, 2017-06-29 at 15:59 -0500, Michael Roth wrote:
> > > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host driver
> > > until all the devices in the group have been detached, at which point all
> > > the hostdevs are rebound as a group. Until that point, the devices are traced
> > > by the drvManager's inactiveList in a similar manner to hostdevs that are
> > > assigned to VFIO via the nodedev-detach interface.
> > 
> > What happens if libvirtd is restarted during this period? Is the
> > inactiveList rebuilt with all the info necessary to assure that the
> > nodedev-reattach does/doesn't happen (as appropriate) for all devices?
> 
> Hmm, good question.
> 
> The Unbindable() check only needs to know that nothing in the activeList
> belongs to the group we're checking, and that list at least seems to get
> rebuilt appropriately on restart.
> 
> But the Unbind() relies on inactiveList and the behavior there is what
> nodedev-detach currently does, which is to add it to inactive list while
> libvirtd is running, and then just forget about it when libvirtd restarts.
> For nodedev-detach it's fine since virHostdevPreparePCIDevices() re-adds
> it as needed in the device-attach path. But yah, for this purpose it ends
> up losing track of hostdevs that are still pending rebind to the host, and
> that means some devices may not get rebound at the appropriate time if
> there was a libvirtd restart.
> 
> Unlike with device-attach, we can't just re-add it on-demand because we
> actually need to know whether or not it was previously in the list. So
> I think we'd need to add some persistent state to track this. Will look
> into adding handling for that.

FWIW last time a tried to attack this issue[1] I got pretty
much as far as this, eg. the code worked as intended but
restarting libvirtd would result in an inconsistent state
which prevented you from performing some operations.

Unfortunately I got sidetracked by other work and stopped
just short of implementing a way to persist the relevant
information on disk :(


[1] ~1.5 years ago, according to git log
-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list