[libvirt PATCH v4 00/25] Add support for persistent mediated devices

[Erik Skultety]

On Wed, Feb 03, 2021 at 11:38:44AM -0600, Jonathon Jongsma wrote:
> This patch series follows the previously-merged series which added support for
> transient mediated devices. This series expands mdev support to include
> persistent device definitions. Again, it relies on mdevctl as the backend.
> 
> It follows the common libvirt pattern of APIs by adding the following new APIs
> for node devices:
>     - virNodeDeviceDefineXML() - defines a persistent device
>     - virNodeDeviceUndefine() - undefines a persistent device
>     - virNodeDeviceCreate() - starts a previously-defined device

So, I tested the patches on Friday and have a few notes:
- all of the driver implementations mentioned ^above need to pass an error
  buffer to the respective mdevctl wrapper, because the generic error "Unable
to XZY mediated device" doesn't help at all. Kind of like with QEMU errors, we
need to extract it from an error buffer (you already have input and output, so
just add another one) --> this relates to patches 14,18,21

- trying to undefine an active mdev reports an error in libvirt
    --> this is both inconsistent with the rest of libvirt's subsystems and
        there's also no reason for it since mdevctl supports it
    --> we need to support transient devices as well

- trying to define the following XML hangs for 5s and then fails:
    <device>
      <parent>pci_0000_06_00_0</parent>
      <driver>
        <name>vfio_mdev</name>
      </driver>
      <capability type='mdev'>
        <type id='nvidia-11'/>
        <uuid>d41787d2-2e0e-4021-8ed2-b6f1ef305a9f</uuid>
      </capability>
    </device>

    -> I debugged this on Friday evening and for some reason driver->devs hash
       table of devices was NULL going through the nodeDeviceFindNewMediatedDevice
       call, but I haven't managed to figure out why it was NULL, listing
       devices worked just fine

- I also prepared several adjustments to how we define the mdevctl wrappers +
  some test adjustments which I wanted to share with you, but I haven't tested
  those thoroughly since I was debugging why that XML couldn't be defined, I'll
  share it when we eliminate the underlying problem
    -> if you're wondering why I didn't just put it into the review, well, I
       figured sharing the code was more descriptive than if I used words

- one thing that I didn't test with your patches are events



> 
> It also adds virsh commands mapping to these new APIs: nodedev-define,
> nodedev-undefine, and nodedev-start.
> 
> Since we rely on mdevctl for the definition of mediated devices, we need a way
> to stay up-to-date with devices that are defined by mdevctl (outside of
> libvirt).  The method for staying up-to-date is currently a little bit crude
> due to the fact that mdevctl does not emit any events when new devices are
> added or removed. As a workaround, we create a file monitor for the mdevctl
> config directory and re-query mdevctl when we detect changes within that
> directory. In the future, mdevctl may introduce a more elegant solution.
> 
> Changes in v4:
>  - rebase to git master
>  - switch to throwaway thread for querying mdevctl
>  - fixed a bug when removing devices because I was accidentally using
>    virHashForEach() instead of virHashForeachSafe()
>  - use DEFINED/UNDEFINED events instead of STARTED/STOPPED events
>  - changes related to merging information about mdev devices from both udev a=
> nd
>    mdevctl:
>    - Re-used the same function to copy extra data from mdevctl regardless of
>      whether we're processing a udev event or a mdevctl event (recommended by
>      Erik). This results in slightly more complex handling of the object
>      lifetimes (see patch 9), but it consolidates some code.
>    - nodeDeviceDefCopyFromMdevctl() previously only copied the data that was
>      unique to mdevctl and didn't exist in udev. It now copies additional data
>      (possibly overwriting some udev).  This solves a problem where a device =
> is
>      defined but not active (i.e.  we have not gotten any data from udev), and
>      then changed (e.g. somebody calls 'mdevctl modify' to change the mdev
>      type), but libvirt was not updating to the new definition.
>  - fix a bug where we were mistakenly emitting 'update' events for devices th=
> at
>    had not changed
>  - Added the ability to specify a uuid for an mdev via device XML.
>  - split some commits into multiple patches
>  - updated new API version info to 7.1.0
>  - Fixed a bug reported by Yan Fu which hangs the client when attempting to
>    destroy a nodedev that is in use by an active vm. See
>    https://www.redhat.com/archives/libvir-list/2021-February/msg00116.html for
>    solution suggested by Alex.
>  - numerous smaller fixes from review findings
> 
> changes in v3:
>  - streamlined tests -- removed some unnecessary duplication
>  - split out patch to factor out node device name generation function
>  - split nodeDeviceParseMdevctlChildDevice() into a separate function
>  - added follow-up patch to remove space-padded alignment in header
>  - refactored the mdevctl update handling significantly:
>    - no longer a separate persistent thread that gets signaled by a timer
>    - now piggybacks onto the existing udev thread and signals the thread in t=
> he
>      same way that the udev event does.
>    - Daniel suggested spawning a throw-away thread to handle mdevctl updates,
>      but that introduces the complexity of possibly serializing multiple
>      throw-away threads (e.g. if we get an 'created' event followed immediate=
> ly
>      by a 'deleted' event, two threads may be spawned and we'd need to ensure
>      they are properly ordered)
>  - added virNodeDeviceObjListForEach() and virNodeDeviceObjListRemoveLocked()
>    to simplify removing devices that are removed from mdevctl.
>  - coding style fixes
>  - NOTE: per Erik's request, I experimented with changing the way that mdevctl
>    commands were generated and tested (e.g. introducing something like
>    virMdevctlGetCommand(def, MDEVCTL_COMMAND_<SUBCOMMAND>, ...)), but it was
>    too invasive and awkward and didn't seem worthwhile

    I wrote the code for ^this and to be honest, it didn't seem that bad after
    all, like I said, I'll share the follow up patches when we know more about
    the issue I encountered.

Erik