Re: [libvirt PATCH v3 00/21] Add support for persistent mediated devices

Tested with v6.10.0-283-g1948d4e61e. 1.Can define/start/destroy mdev device successfully; 2.'virsh nodedev-list' has no '--active' option, which is inconsistent with the description in the patch： # virsh nodedev-list --active error: command 'nodedev-list' doesn't support option --active 3.virsh client hang when trying to destroy a mdev device which is using by a vm, and after that all 'virsh nodev*' cmds will hang. If restarting llibvirtd after that, libvirtd will hang.

4.Define a mdev device with the uuid specified, but the mdev device defined seems using another uuid. Maybe it make a little confusion about the use of uuid in the xml: #cat mdev.xml <device> <name>mdev_85531b6d_e5e4_41c1_aa2a_8844717f066a</name> **** <path>/sys/devices/pci0000:00/0000:00:02.0/85531b6d-e5e4-41c1-aa2a-8844717f066a</path> *** <parent>pci_0000_00_02_0</parent> <driver> <name>vfio_mdev</name> </driver> <capability type='mdev'> <type id='i915-GVTg_V5_8'/> <iommuGroup number='0'/> </capability> </device> # virsh nodedev-define /root/mdev.xml Node device ***mdev_6cdbaad0_7215_4741_82b9_c51e1a4decda**** defined from /root/mdev.xml -- Tested by Yan Fu(yafu(a)redhat.com)

On Fri, Dec 25, 2020 at 10:19 AM Han Han <hhan(a)redhat.com&gt; wrote: > > > On Thu, Dec 24, 2020 at 10:15 PM Jonathon Jongsma <jjongsma(a)redhat.com&gt; > 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 >> >> It also adds virsh commands mapping to these new APIs: nodedev-define, >> nodedev-undefine, and nodedev-start. >> > Trivial suggestions: > 1. Mention the bug to be resolved by this series in commit msg: > https://bugzilla.redhat.com/show_bug.cgi?id=1699274 > 2. Add doc of man page for the new virsh commands and options > >> >> Since we rely on mdevctl for the definition of ediated 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 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 >> >> Changes in v2: >> - rebase to latest git master >> >> Jonathon Jongsma (21): >> tests: remove extra trailing semicolon >> nodedev: introduce concept of 'active' node devices >> nodedev: Add ability to filter by active state >> nodedev: expose internal helper for naming devices >> nodedev: add ability to list and parse defined mdevs >> nodedev: add STOPPED/STARTED lifecycle events >> nodedev: add mdevctl devices to node device list >> nodedev: add helper functions to remove node devices >> nodedev: handle mdevs that disappear from mdevctl >> nodedev: rename dataReady to udevReady >> nodedev: Refresh mdev devices when changes are detected >> api: add virNodeDeviceDefineXML() >> virsh: Add --active, --inactive, --all to nodedev-list >> virsh: add nodedev-define command >> nodedev: refactor tests to support mdev undefine >> api: add virNodeDeviceUndefine() >> virsh: Factor out function to find node device >> virsh: add nodedev-undefine command >> api: add virNodeDeviceCreate() >> virsh: add "nodedev-start" command >> libvirt-nodedev.h: remove space-padded alignment >> >> examples/c/misc/event-test.c | 4 + >> include/libvirt/libvirt-nodedev.h | 98 ++-- >> src/access/viraccessperm.c | 2 +- >> src/access/viraccessperm.h | 6 + >> src/conf/node_device_conf.h | 9 + >> src/conf/virnodedeviceobj.c | 132 ++++- >> src/conf/virnodedeviceobj.h | 19 + >> src/driver-nodedev.h | 14 + >> src/libvirt-nodedev.c | 115 ++++ >> src/libvirt_private.syms | 4 + >> src/libvirt_public.syms | 3 + >> src/node_device/node_device_driver.c | 538 +++++++++++++++++- >> src/node_device/node_device_driver.h | 38 ++ >> src/node_device/node_device_udev.c | 308 ++++++++-- >> src/remote/remote_driver.c | 3 + >> src/remote/remote_protocol.x | 40 +- >> src/remote_protocol-structs | 16 + >> src/rpc/gendispatch.pl | 1 + >> ...19_36ea_4111_8f0a_8c9a70e21366-define.argv | 1 + >> ...19_36ea_4111_8f0a_8c9a70e21366-define.json | 1 + >> ...39_495e_4243_ad9f_beb3f14c23d9-define.argv | 1 + >> ...39_495e_4243_ad9f_beb3f14c23d9-define.json | 1 + >> ...16_1ca8_49ac_b176_871d16c13076-define.argv | 1 + >> ...16_1ca8_49ac_b176_871d16c13076-define.json | 1 + >> tests/nodedevmdevctldata/mdevctl-create.argv | 1 + >> .../mdevctl-list-defined.argv | 1 + >> .../mdevctl-list-multiple.json | 59 ++ >> .../mdevctl-list-multiple.out.xml | 39 ++ >> tests/nodedevmdevctltest.c | 222 +++++++- >> tools/virsh-nodedev.c | 276 +++++++-- >> 30 files changed, 1765 insertions(+), 189 deletions(-) >> create mode 100644 >> tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9= >> a70e21366-define.argv >> create mode 100644 >> tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9= >> a70e21366-define.json >> create mode 100644 >> tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb= >> 3f14c23d9-define.argv >> create mode 100644 >> tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb= >> 3f14c23d9-define.json >> create mode 100644 >> tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871= >> d16c13076-define.argv >> create mode 100644 >> tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871= >> d16c13076-define.json >> create mode 100644 tests/nodedevmdevctldata/mdevctl-create.argv >> create mode 100644 tests/nodedevmdevctldata/mdevctl-list-defined.argv >> create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.json >> create mode 100644 >> tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml >> >> --=20 >> 2.26.2 >> >> >>