[libvirt] [RFC PATCH 00/11] Add mdev reporting capability to the nodedev driver

This series enables the node device driver to report information about the existing mediated devices on the host. There is no device creation involved yet. The information reported by the node device driver is split into two parts, one that is reported within the physical parent's capabilities (the generic stuff that comes from the mdev types' sysfs attributes, note the 'description' attribute which is verbatim - raw,unstructured string) and the other that is reported within the mdev child device and merely contains the mdev type id, which the device was instantiated from, and the iommu group number. Basically, the format of the XML I went for is as follows: PCI parent: <device> <name>pci_0000_06_00_0</name> <path>/sys/devices/.../0000:06:00.0</path> <parent>pci_0000_05_08_0</parent> ... <capability type='pci'> ... <capability type='mdev'> <type id='nvidia-11'> <name>GRID M60-0B</name> <description>num_heads=2, frl_config=45, framebuffer=512M, max_resolution=2560x1600, max_instance=16</description> <device_api>vfio-pci</device_api> <available_instances>16</available_instances> </type> ... </capability> <iommuGroup number='20'> <address domain='0x0000' bus='0x06' slot='0x00' function='0x0'/> </iommuGroup> ... </capability> </device> mdev child: <device> <name>mdev_ef1212ff_ff23_4534_ffcd_01ff12017801</name> <path>/sys/devices/.../ef1212ff-ff23-4534-ffcd-01ff12017801</path> <parent>pci_0000_06_00_0</parent> <driver> <name>vfio_mdev</name> </driver> <capability type='mdev'> <type id='nvidia-18'/> <iommuGroup number='63'/> </capability> </device> Also, since we didn't have any node device driver documentation, I created a stub (comments are very welcome, you can shred it to pieces ;)) focusing on the PCI devices and then adding the mdev part into that. As I said, it's still a stub that needs lots of work on it, namely adding USBs and SCSI devices, but I figured that the fact some parts are missing should not be a show stopper for the nodedev-mdev patches. Thanks, Erik Erik Skultety (11): nodedev: Fix guideline violations in nodedev modules nodedev: Make use of the compile-time missing enum in switch error conf: nodedev: Split virNodeDeviceDefFormat into more functions nodedev: udevProcessPCI: Drop syspath variable docs: Use our XSLT template to generate list of supported pool types nodedev: Introduce the mdev capability to the nodedev driver structure nodedev: Fill in the mdev info for the parent PCI device nodedev: Introduce udevProcessMediatedDevice nodedev: Format the mdev capability of the PCI parent device docs: Provide a nodedev driver stub documentation docs: Document the mediated devices within the nodedev driver docs/drivers.html.in | 6 +- docs/drvnodedev.html.in | 275 +++++++++++++ docs/storage.html.in | 62 +-- include/libvirt/libvirt-nodedev.h | 1 + src/conf/node_device_conf.c | 625 ++++++++++++++++++------------ src/conf/node_device_conf.h | 21 +- src/conf/virnodedeviceobj.c | 3 +- src/libvirt-nodedev.c | 1 + src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 30 +- src/node_device/node_device_driver.h | 82 ++-- src/node_device/node_device_hal.c | 9 + src/node_device/node_device_linux_sysfs.c | 1 + src/node_device/node_device_linux_sysfs.h | 9 +- src/node_device/node_device_udev.c | 456 ++++++++++++++++------ tools/virsh-nodedev.c | 2 + 16 files changed, 1110 insertions(+), 474 deletions(-) create mode 100644 docs/drvnodedev.html.in -- 2.12.2

We recently started to enforce certain guideline rule a bit more, e.g. functions delimited by 2 newlines in *.c, by 1 newline in *.h, return types on a separate line, etc. This patch adjusts these issues in all nodedev modules. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_driver.c | 29 +++- src/node_device/node_device_driver.h | 82 +++++++---- src/node_device/node_device_hal.c | 9 ++ src/node_device/node_device_linux_sysfs.c | 1 + src/node_device/node_device_linux_sysfs.h | 9 +- src/node_device/node_device_udev.c | 233 ++++++++++++++++++------------ 6 files changed, 238 insertions(+), 125 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 99f7bc5476..9b1c5bb5e9 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -47,7 +47,8 @@ virNodeDeviceDriverStatePtr driver; -static int update_caps(virNodeDeviceObjPtr dev) +static int +update_caps(virNodeDeviceObjPtr dev) { virNodeDevCapsDefPtr cap = dev->def->caps; @@ -101,7 +102,8 @@ static int update_caps(virNodeDeviceObjPtr dev) * the driver name for a device each time its entry is used, both for * udev *and* HAL backends. */ -static int update_driver_name(virNodeDeviceObjPtr dev) +static int +update_driver_name(virNodeDeviceObjPtr dev) { char *driver_link = NULL; char *devpath = NULL; @@ -138,22 +140,28 @@ static int update_driver_name(virNodeDeviceObjPtr dev) } #else /* XXX: Implement me for non-linux */ -static int update_driver_name(virNodeDeviceObjPtr dev ATTRIBUTE_UNUSED) +static int +update_driver_name(virNodeDeviceObjPtr dev ATTRIBUTE_UNUSED) { return 0; } #endif -void nodeDeviceLock(void) +void +nodeDeviceLock(void) { virMutexLock(&driver->lock); } -void nodeDeviceUnlock(void) + + +void +nodeDeviceUnlock(void) { virMutexUnlock(&driver->lock); } + int nodeNumOfDevices(virConnectPtr conn, const char *cap, @@ -182,6 +190,7 @@ nodeNumOfDevices(virConnectPtr conn, return ndevs; } + int nodeListDevices(virConnectPtr conn, const char *cap, @@ -222,6 +231,7 @@ nodeListDevices(virConnectPtr conn, return -1; } + int nodeConnectListAllNodeDevices(virConnectPtr conn, virNodeDevicePtr **devices, @@ -242,6 +252,7 @@ nodeConnectListAllNodeDevices(virConnectPtr conn, return ret; } + virNodeDevicePtr nodeDeviceLookupByName(virConnectPtr conn, const char *name) { @@ -500,6 +511,7 @@ nodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames) return ret; } + static int get_time(time_t *t) { @@ -565,6 +577,7 @@ find_new_device(virConnectPtr conn, const char *wwnn, const char *wwpn) return dev; } + virNodeDevicePtr nodeDeviceCreateXML(virConnectPtr conn, const char *xmlDesc, @@ -663,6 +676,7 @@ nodeDeviceDestroy(virNodeDevicePtr dev) return ret; } + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, virNodeDevicePtr dev, @@ -684,6 +698,7 @@ nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, return callbackID; } + int nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn, int callbackID) @@ -704,7 +719,9 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn, return ret; } -int nodedevRegister(void) + +int +nodedevRegister(void) { #ifdef WITH_UDEV return udevNodeRegister(); diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index bc8af8a9e7..0f55e7b71e 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -30,37 +30,67 @@ # define LINUX_NEW_DEVICE_WAIT_TIME 60 +extern virNodeDeviceDriverStatePtr driver; + # ifdef WITH_HAL -int halNodeRegister(void); +int +halNodeRegister(void); # endif # ifdef WITH_UDEV -int udevNodeRegister(void); +int +udevNodeRegister(void); # endif -void nodeDeviceLock(void); -void nodeDeviceUnlock(void); - -extern virNodeDeviceDriverStatePtr driver; - -int nodedevRegister(void); - -int nodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags); -int nodeListDevices(virConnectPtr conn, const char *cap, char **const names, - int maxnames, unsigned int flags); -int nodeConnectListAllNodeDevices(virConnectPtr conn, - virNodeDevicePtr **devices, - unsigned int flags); -virNodeDevicePtr nodeDeviceLookupByName(virConnectPtr conn, const char *name); -virNodeDevicePtr nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, - const char *wwnn, - const char *wwpn, - unsigned int flags); -char *nodeDeviceGetXMLDesc(virNodeDevicePtr dev, unsigned int flags); -char *nodeDeviceGetParent(virNodeDevicePtr dev); -int nodeDeviceNumOfCaps(virNodeDevicePtr dev); -int nodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames); -virNodeDevicePtr nodeDeviceCreateXML(virConnectPtr conn, - const char *xmlDesc, unsigned int flags); +void +nodeDeviceLock(void); + +void +nodeDeviceUnlock(void); + +int +nodedevRegister(void); + +int +nodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags); + +int +nodeListDevices(virConnectPtr conn, + const char *cap, + char **const names, + int maxnames, + unsigned int flags); + +int +nodeConnectListAllNodeDevices(virConnectPtr conn, + virNodeDevicePtr **devices, + unsigned int flags); + +virNodeDevicePtr +nodeDeviceLookupByName(virConnectPtr conn, const char *name); + +virNodeDevicePtr +nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, + const char *wwnn, + const char *wwpn, + unsigned int flags); + +char * +nodeDeviceGetXMLDesc(virNodeDevicePtr dev, unsigned int flags); + +char * +nodeDeviceGetParent(virNodeDevicePtr dev); + +int +nodeDeviceNumOfCaps(virNodeDevicePtr dev); + +int +nodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames); + +virNodeDevicePtr +nodeDeviceCreateXML(virConnectPtr conn, + const char *xmlDesc, + unsigned int flags); + int nodeDeviceDestroy(virNodeDevicePtr dev); int diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 81e5ecc41d..5339218775 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -84,6 +84,7 @@ get_str_prop(LibHalContext *ctxt, const char *udi, return -1; } + static int get_int_prop(LibHalContext *ctxt, const char *udi, const char *prop, int *val_p) @@ -102,6 +103,7 @@ get_int_prop(LibHalContext *ctxt, const char *udi, return rv; } + static int get_bool_prop(LibHalContext *ctxt, const char *udi, const char *prop, int *val_p) @@ -120,6 +122,7 @@ get_bool_prop(LibHalContext *ctxt, const char *udi, return rv; } + static int get_uint64_prop(LibHalContext *ctxt, const char *udi, const char *prop, unsigned long long *val_p) @@ -138,6 +141,7 @@ get_uint64_prop(LibHalContext *ctxt, const char *udi, return rv; } + static int gather_pci_cap(LibHalContext *ctx, const char *udi, virNodeDevCapDataPtr d) @@ -290,6 +294,7 @@ gather_storage_cap(LibHalContext *ctx, const char *udi, return 0; } + static int gather_scsi_generic_cap(LibHalContext *ctx, const char *udi, virNodeDevCapDataPtr d) @@ -446,12 +451,14 @@ gather_capabilities(LibHalContext *ctx, const char *udi, return rv; } + static void free_udi(void *udi) { VIR_FREE(udi); } + static void dev_create(const char *udi) { @@ -516,6 +523,7 @@ dev_create(const char *udi) nodeDeviceUnlock(); } + static void dev_refresh(const char *udi) { @@ -538,6 +546,7 @@ dev_refresh(const char *udi) dev_create(udi); } + static void device_added(LibHalContext *ctx ATTRIBUTE_UNUSED, const char *udi) diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index 1b7aa9435c..ff22d6d55c 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -230,6 +230,7 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host ATTRIBUTE_UNUS return -1; } + int nodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath ATTRIBUTE_UNUSED, virNodeDevCapPCIDevPtr pci_dev ATTRIBUTE_UNUSED) diff --git a/src/node_device/node_device_linux_sysfs.h b/src/node_device/node_device_linux_sysfs.h index 8deea66998..11b96e86b8 100644 --- a/src/node_device/node_device_linux_sysfs.h +++ b/src/node_device/node_device_linux_sysfs.h @@ -25,8 +25,11 @@ # include "node_device_conf.h" -int nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host); -int nodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath, - virNodeDevCapPCIDevPtr pci_dev); +int +nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host); + +int +nodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath, + virNodeDevCapPCIDevPtr pci_dev); #endif /* __VIR_NODE_DEVICE_LINUX_SYSFS_H__ */ diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index bcae444d85..4a837e04dc 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -70,8 +70,9 @@ udevHasDeviceProperty(struct udev_device *dev, } -static const char *udevGetDeviceProperty(struct udev_device *udev_device, - const char *property_key) +static const char * +udevGetDeviceProperty(struct udev_device *udev_device, + const char *property_key) { const char *ret = NULL; @@ -84,9 +85,10 @@ static const char *udevGetDeviceProperty(struct udev_device *udev_device, } -static int udevGetStringProperty(struct udev_device *udev_device, - const char *property_key, - char **value) +static int +udevGetStringProperty(struct udev_device *udev_device, + const char *property_key, + char **value) { if (VIR_STRDUP(*value, udevGetDeviceProperty(udev_device, property_key)) < 0) @@ -96,10 +98,11 @@ static int udevGetStringProperty(struct udev_device *udev_device, } -static int udevGetIntProperty(struct udev_device *udev_device, - const char *property_key, - int *value, - int base) +static int +udevGetIntProperty(struct udev_device *udev_device, + const char *property_key, + int *value, + int base) { const char *str = NULL; @@ -114,10 +117,11 @@ static int udevGetIntProperty(struct udev_device *udev_device, } -static int udevGetUintProperty(struct udev_device *udev_device, - const char *property_key, - unsigned int *value, - int base) +static int +udevGetUintProperty(struct udev_device *udev_device, + const char *property_key, + unsigned int *value, + int base) { const char *str = NULL; @@ -132,8 +136,9 @@ static int udevGetUintProperty(struct udev_device *udev_device, } -static const char *udevGetDeviceSysfsAttr(struct udev_device *udev_device, - const char *attr_name) +static const char * +udevGetDeviceSysfsAttr(struct udev_device *udev_device, + const char *attr_name) { const char *ret = NULL; @@ -147,9 +152,10 @@ static const char *udevGetDeviceSysfsAttr(struct udev_device *udev_device, } -static int udevGetStringSysfsAttr(struct udev_device *udev_device, - const char *attr_name, - char **value) +static int +udevGetStringSysfsAttr(struct udev_device *udev_device, + const char *attr_name, + char **value) { if (VIR_STRDUP(*value, udevGetDeviceSysfsAttr(udev_device, attr_name)) < 0) return -1; @@ -163,10 +169,11 @@ static int udevGetStringSysfsAttr(struct udev_device *udev_device, } -static int udevGetIntSysfsAttr(struct udev_device *udev_device, - const char *attr_name, - int *value, - int base) +static int +udevGetIntSysfsAttr(struct udev_device *udev_device, + const char *attr_name, + int *value, + int base) { const char *str = NULL; @@ -182,10 +189,11 @@ static int udevGetIntSysfsAttr(struct udev_device *udev_device, } -static int udevGetUintSysfsAttr(struct udev_device *udev_device, - const char *attr_name, - unsigned int *value, - int base) +static int +udevGetUintSysfsAttr(struct udev_device *udev_device, + const char *attr_name, + unsigned int *value, + int base) { const char *str = NULL; @@ -201,9 +209,10 @@ static int udevGetUintSysfsAttr(struct udev_device *udev_device, } -static int udevGetUint64SysfsAttr(struct udev_device *udev_device, - const char *attr_name, - unsigned long long *value) +static int +udevGetUint64SysfsAttr(struct udev_device *udev_device, + const char *attr_name, + unsigned long long *value) { const char *str = NULL; @@ -219,9 +228,10 @@ static int udevGetUint64SysfsAttr(struct udev_device *udev_device, } -static int udevGenerateDeviceName(struct udev_device *device, - virNodeDeviceDefPtr def, - const char *s) +static int +udevGenerateDeviceName(struct udev_device *device, + virNodeDeviceDefPtr def, + const char *s) { size_t i; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -282,10 +292,11 @@ udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED, #endif -static int udevTranslatePCIIds(unsigned int vendor, - unsigned int product, - char **vendor_string, - char **product_string) +static int +udevTranslatePCIIds(unsigned int vendor, + unsigned int product, + char **vendor_string, + char **product_string) { struct pci_id_match m; const char *vendor_name = NULL, *device_name = NULL; @@ -313,8 +324,9 @@ static int udevTranslatePCIIds(unsigned int vendor, } -static int udevProcessPCI(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevProcessPCI(struct udev_device *device, + virNodeDeviceDefPtr def) { const char *syspath = NULL; virNodeDevCapPCIDevPtr pci_dev = &def->caps->data.pci_dev; @@ -411,7 +423,9 @@ static int udevProcessPCI(struct udev_device *device, return ret; } -static int drmGetMinorType(int minor) + +static int +drmGetMinorType(int minor) { int type = minor >> 6; @@ -428,8 +442,10 @@ static int drmGetMinorType(int minor) } } -static int udevProcessDRMDevice(struct udev_device *device, - virNodeDeviceDefPtr def) + +static int +udevProcessDRMDevice(struct udev_device *device, + virNodeDeviceDefPtr def) { virNodeDevCapDRMPtr drm = &def->caps->data.drm; int minor; @@ -448,8 +464,10 @@ static int udevProcessDRMDevice(struct udev_device *device, return 0; } -static int udevProcessUSBDevice(struct udev_device *device, - virNodeDeviceDefPtr def) + +static int +udevProcessUSBDevice(struct udev_device *device, + virNodeDeviceDefPtr def) { virNodeDevCapUSBDevPtr usb_dev = &def->caps->data.usb_dev; @@ -490,8 +508,9 @@ static int udevProcessUSBDevice(struct udev_device *device, } -static int udevProcessUSBInterface(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevProcessUSBInterface(struct udev_device *device, + virNodeDeviceDefPtr def) { virNodeDevCapUSBIfPtr usb_if = &def->caps->data.usb_if; @@ -518,8 +537,9 @@ static int udevProcessUSBInterface(struct udev_device *device, } -static int udevProcessNetworkInterface(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevProcessNetworkInterface(struct udev_device *device, + virNodeDeviceDefPtr def) { const char *devtype = udev_device_get_devtype(device); virNodeDevCapNetPtr net = &def->caps->data.net; @@ -581,8 +601,9 @@ static int udevProcessSCSIHost(struct udev_device *device ATTRIBUTE_UNUSED, } -static int udevProcessSCSITarget(struct udev_device *device ATTRIBUTE_UNUSED, - virNodeDeviceDefPtr def) +static int +udevProcessSCSITarget(struct udev_device *device ATTRIBUTE_UNUSED, + virNodeDeviceDefPtr def) { const char *sysname = NULL; virNodeDevCapSCSITargetPtr scsi_target = &def->caps->data.scsi_target; @@ -599,8 +620,9 @@ static int udevProcessSCSITarget(struct udev_device *device ATTRIBUTE_UNUSED, } -static int udevGetSCSIType(virNodeDeviceDefPtr def ATTRIBUTE_UNUSED, - unsigned int type, char **typestring) +static int +udevGetSCSIType(virNodeDeviceDefPtr def ATTRIBUTE_UNUSED, + unsigned int type, char **typestring) { int ret = 0; int foundtype = 1; @@ -657,8 +679,9 @@ static int udevGetSCSIType(virNodeDeviceDefPtr def ATTRIBUTE_UNUSED, } -static int udevProcessSCSIDevice(struct udev_device *device ATTRIBUTE_UNUSED, - virNodeDeviceDefPtr def) +static int +udevProcessSCSIDevice(struct udev_device *device ATTRIBUTE_UNUSED, + virNodeDeviceDefPtr def) { int ret = -1; unsigned int tmp = 0; @@ -700,8 +723,9 @@ static int udevProcessSCSIDevice(struct udev_device *device ATTRIBUTE_UNUSED, } -static int udevProcessDisk(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevProcessDisk(struct udev_device *device, + virNodeDeviceDefPtr def) { virNodeDevCapStoragePtr storage = &def->caps->data.storage; @@ -718,9 +742,10 @@ static int udevProcessDisk(struct udev_device *device, } -static int udevProcessRemoveableMedia(struct udev_device *device, - virNodeDeviceDefPtr def, - int has_media) +static int +udevProcessRemoveableMedia(struct udev_device *device, + virNodeDeviceDefPtr def, + int has_media) { virNodeDevCapStoragePtr storage = &def->caps->data.storage; int is_removable = 0; @@ -759,8 +784,10 @@ static int udevProcessRemoveableMedia(struct udev_device *device, return 0; } -static int udevProcessCDROM(struct udev_device *device, - virNodeDeviceDefPtr def) + +static int +udevProcessCDROM(struct udev_device *device, + virNodeDeviceDefPtr def) { int has_media = 0; @@ -779,8 +806,10 @@ static int udevProcessCDROM(struct udev_device *device, return udevProcessRemoveableMedia(device, def, has_media); } -static int udevProcessFloppy(struct udev_device *device, - virNodeDeviceDefPtr def) + +static int +udevProcessFloppy(struct udev_device *device, + virNodeDeviceDefPtr def) { int has_media = 0; @@ -797,8 +826,9 @@ static int udevProcessFloppy(struct udev_device *device, } -static int udevProcessSD(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevProcessSD(struct udev_device *device, + virNodeDeviceDefPtr def) { virNodeDevCapStoragePtr storage = &def->caps->data.storage; @@ -821,7 +851,8 @@ static int udevProcessSD(struct udev_device *device, * not provide a device type in the usual place, but udev told us it's * a storage device, and we can make a good guess at what kind of * storage device it is from other information that is provided. */ -static int udevKludgeStorageType(virNodeDeviceDefPtr def) +static int +udevKludgeStorageType(virNodeDeviceDefPtr def) { VIR_DEBUG("Could not find definitive storage type for device " "with sysfs path '%s', trying to guess it", @@ -842,8 +873,9 @@ static int udevKludgeStorageType(virNodeDeviceDefPtr def) } -static int udevProcessStorage(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevProcessStorage(struct udev_device *device, + virNodeDeviceDefPtr def) { virNodeDevCapStoragePtr storage = &def->caps->data.storage; int ret = -1; @@ -938,6 +970,7 @@ static int udevProcessStorage(struct udev_device *device, return ret; } + static int udevProcessSCSIGeneric(struct udev_device *dev, virNodeDeviceDefPtr def) @@ -952,6 +985,7 @@ udevProcessSCSIGeneric(struct udev_device *dev, return 0; } + static int udevGetDeviceNodes(struct udev_device *device, virNodeDeviceDefPtr def) @@ -980,6 +1014,7 @@ udevGetDeviceNodes(struct udev_device *device, return 0; } + static int udevGetDeviceType(struct udev_device *device, virNodeDevCapType *type) @@ -1040,8 +1075,9 @@ udevGetDeviceType(struct udev_device *device, } -static int udevGetDeviceDetails(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevGetDeviceDetails(struct udev_device *device, + virNodeDeviceDefPtr def) { int ret = 0; @@ -1090,7 +1126,8 @@ static int udevGetDeviceDetails(struct udev_device *device, } -static int udevRemoveOneDevice(struct udev_device *device) +static int +udevRemoveOneDevice(struct udev_device *device) { virNodeDeviceObjPtr dev = NULL; virObjectEventPtr event = NULL; @@ -1122,8 +1159,9 @@ static int udevRemoveOneDevice(struct udev_device *device) } -static int udevSetParent(struct udev_device *device, - virNodeDeviceDefPtr def) +static int +udevSetParent(struct udev_device *device, + virNodeDeviceDefPtr def) { struct udev_device *parent_device = NULL; const char *parent_sysfs_path = NULL; @@ -1170,7 +1208,8 @@ static int udevSetParent(struct udev_device *device, } -static int udevAddOneDevice(struct udev_device *device) +static int +udevAddOneDevice(struct udev_device *device) { virNodeDeviceDefPtr def = NULL; virNodeDeviceObjPtr dev = NULL; @@ -1239,8 +1278,9 @@ static int udevAddOneDevice(struct udev_device *device) } -static int udevProcessDeviceListEntry(struct udev *udev, - struct udev_list_entry *list_entry) +static int +udevProcessDeviceListEntry(struct udev *udev, + struct udev_list_entry *list_entry) { struct udev_device *device; const char *name = NULL; @@ -1272,7 +1312,8 @@ const char *subsystem_blacklist[] = { "acpi", "tty", "vc", "i2c", }; -static int udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate) +static int +udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate) { size_t i; @@ -1287,7 +1328,8 @@ static int udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate) } -static int udevEnumerateDevices(struct udev *udev) +static int +udevEnumerateDevices(struct udev *udev) { struct udev_enumerate *udev_enumerate = NULL; struct udev_list_entry *list_entry = NULL; @@ -1317,7 +1359,8 @@ static int udevEnumerateDevices(struct udev *udev) } -static void udevPCITranslateDeinit(void) +static void +udevPCITranslateDeinit(void) { #if defined __s390__ || defined __s390x_ /* Nothing was initialized, nothing needs to be cleaned up */ @@ -1329,7 +1372,8 @@ static void udevPCITranslateDeinit(void) } -static int nodeStateCleanup(void) +static int +nodeStateCleanup(void) { udevPrivate *priv = NULL; struct udev_monitor *udev_monitor = NULL; @@ -1370,10 +1414,11 @@ static int nodeStateCleanup(void) } -static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, - int fd, - int events ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED) +static void +udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, + int fd, + int events ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED) { struct udev_device *device = NULL; struct udev_monitor *udev_monitor = DRV_STATE_UDEV_MONITOR(driver); @@ -1474,7 +1519,8 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap) #endif -static int udevSetupSystemDev(void) +static int +udevSetupSystemDev(void) { virNodeDeviceDefPtr def = NULL; virNodeDeviceObjPtr dev = NULL; @@ -1508,7 +1554,9 @@ static int udevSetupSystemDev(void) return ret; } -static int udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED) + +static int +udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED) { #if defined __s390__ || defined __s390x_ /* On s390(x) system there is no PCI bus. @@ -1530,9 +1578,11 @@ static int udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED) return 0; } -static int nodeStateInitialize(bool privileged, - virStateInhibitCallback callback ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + +static int +nodeStateInitialize(bool privileged, + virStateInhibitCallback callback ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) { udevPrivate *priv = NULL; struct udev *udev = NULL; @@ -1618,7 +1668,8 @@ static int nodeStateInitialize(bool privileged, } -static int nodeStateReload(void) +static int +nodeStateReload(void) { return 0; } @@ -1648,7 +1699,9 @@ static virStateDriver udevStateDriver = { .stateReload = nodeStateReload, /* 0.7.3 */ }; -int udevNodeRegister(void) + +int +udevNodeRegister(void) { VIR_DEBUG("Registering udev node device backend"); -- 2.12.2

On Wed, Mar 29, 2017 at 14:51:11 +0200, Erik Skultety wrote:
We recently started to enforce certain guideline rule a bit more, e.g. functions delimited by 2 newlines in *.c, by 1 newline in *.h, return types on a separate line, etc. This patch adjusts these issues in all nodedev modules.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_driver.c | 29 +++- src/node_device/node_device_driver.h | 82 +++++++---- src/node_device/node_device_hal.c | 9 ++ src/node_device/node_device_linux_sysfs.c | 1 + src/node_device/node_device_linux_sysfs.h | 9 +- src/node_device/node_device_udev.c | 233 ++++++++++++++++++------------ 6 files changed, 238 insertions(+), 125 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 99f7bc5476..9b1c5bb5e9 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -47,7 +47,8 @@
virNodeDeviceDriverStatePtr driver;
-static int update_caps(virNodeDeviceObjPtr dev) +static int +update_caps(virNodeDeviceObjPtr dev)
This does not really adhere to the guidelines. Guidelines say that functions have to start with 'vir' prefix and use camel case. Also they have to contain the name of the module.

On Wed, Mar 29, 2017 at 03:50:28PM +0200, Peter Krempa wrote:
On Wed, Mar 29, 2017 at 14:51:11 +0200, Erik Skultety wrote:
We recently started to enforce certain guideline rule a bit more, e.g. functions delimited by 2 newlines in *.c, by 1 newline in *.h, return types on a separate line, etc. This patch adjusts these issues in all nodedev modules.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_driver.c | 29 +++- src/node_device/node_device_driver.h | 82 +++++++---- src/node_device/node_device_hal.c | 9 ++ src/node_device/node_device_linux_sysfs.c | 1 + src/node_device/node_device_linux_sysfs.h | 9 +- src/node_device/node_device_udev.c | 233 ++++++++++++++++++------------ 6 files changed, 238 insertions(+), 125 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 99f7bc5476..9b1c5bb5e9 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -47,7 +47,8 @@
virNodeDeviceDriverStatePtr driver;
-static int update_caps(virNodeDeviceObjPtr dev) +static int +update_caps(virNodeDeviceObjPtr dev)
This does not really adhere to the guidelines. Guidelines say that functions have to start with 'vir' prefix and use camel case. Also they have to contain the name of the module.
Ehm....you just shredded me here to DIN P-6 [1] at least :D, but yeah, you're absolutely right. Erik [1] https://en.wikipedia.org/wiki/Paper_shredder

So udevGetDeviceDetails was one those functions using an enum in a switch, but since it had a 'default' case, compiler didn't warn about an unhandled enum. Moreover, the error about an unsupported device type reported in the default case is unnecessary, since by the time we get there, udevGetDeviceType (which was called before) already made sure that any unrecognized device types had been handled properly. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_udev.c | 45 +++++++++++++------------------------- 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4a837e04dc..976ccad710 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1079,50 +1079,35 @@ static int udevGetDeviceDetails(struct udev_device *device, virNodeDeviceDefPtr def) { - int ret = 0; - switch (def->caps->data.type) { - case VIR_NODE_DEV_CAP_SYSTEM: - /* There's no libudev equivalent of system, so ignore it. */ - break; case VIR_NODE_DEV_CAP_PCI_DEV: - ret = udevProcessPCI(device, def); - break; + return udevProcessPCI(device, def); case VIR_NODE_DEV_CAP_USB_DEV: - ret = udevProcessUSBDevice(device, def); - break; + return udevProcessUSBDevice(device, def); case VIR_NODE_DEV_CAP_USB_INTERFACE: - ret = udevProcessUSBInterface(device, def); - break; + return udevProcessUSBInterface(device, def); case VIR_NODE_DEV_CAP_NET: - ret = udevProcessNetworkInterface(device, def); - break; + return udevProcessNetworkInterface(device, def); case VIR_NODE_DEV_CAP_SCSI_HOST: - ret = udevProcessSCSIHost(device, def); - break; + return udevProcessSCSIHost(device, def); case VIR_NODE_DEV_CAP_SCSI_TARGET: - ret = udevProcessSCSITarget(device, def); - break; + return udevProcessSCSITarget(device, def); case VIR_NODE_DEV_CAP_SCSI: - ret = udevProcessSCSIDevice(device, def); - break; + return udevProcessSCSIDevice(device, def); case VIR_NODE_DEV_CAP_STORAGE: - ret = udevProcessStorage(device, def); - break; + return udevProcessStorage(device, def); case VIR_NODE_DEV_CAP_SCSI_GENERIC: - ret = udevProcessSCSIGeneric(device, def); - break; + return udevProcessSCSIGeneric(device, def); case VIR_NODE_DEV_CAP_DRM: - ret = udevProcessDRMDevice(device, def); - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown device type %d"), def->caps->data.type); - ret = -1; + return udevProcessDRMDevice(device, def); + case VIR_NODE_DEV_CAP_SYSTEM: + case VIR_NODE_DEV_CAP_FC_HOST: + case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_LAST: break; } - return ret; + return 0; } -- 2.12.2

Make the code look cleaner by moving the capability specific bits into separate functions. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/node_device_conf.c | 578 ++++++++++++++++++++++++-------------------- 1 file changed, 322 insertions(+), 256 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 7d0baa9d1a..72fb9a5611 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -155,6 +155,320 @@ virPCIEDeviceInfoFormat(virBufferPtr buf, } +static void +virNodeDeviceCapSystemDefFormat(virBufferPtr buf, + const virNodeDevCapData *data) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (data->system.product_name) + virBufferEscapeString(buf, "<product>%s</product>\n", + data->system.product_name); + virBufferAddLit(buf, "<hardware>\n"); + virBufferAdjustIndent(buf, 2); + if (data->system.hardware.vendor_name) + virBufferEscapeString(buf, "<vendor>%s</vendor>\n", + data->system.hardware.vendor_name); + if (data->system.hardware.version) + virBufferEscapeString(buf, "<version>%s</version>\n", + data->system.hardware.version); + if (data->system.hardware.serial) + virBufferEscapeString(buf, "<serial>%s</serial>\n", + data->system.hardware.serial); + virUUIDFormat(data->system.hardware.uuid, uuidstr); + virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuidstr); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</hardware>\n"); + + virBufferAddLit(buf, "<firmware>\n"); + virBufferAdjustIndent(buf, 2); + if (data->system.firmware.vendor_name) + virBufferEscapeString(buf, "<vendor>%s</vendor>\n", + data->system.firmware.vendor_name); + if (data->system.firmware.version) + virBufferEscapeString(buf, "<version>%s</version>\n", + data->system.firmware.version); + if (data->system.firmware.release_date) + virBufferEscapeString(buf, "<release_date>%s</release_date>\n", + data->system.firmware.release_date); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</firmware>\n"); +} + + +static void +virNodeDeviceCapPCIDefFormat(virBufferPtr buf, + const virNodeDevCapData *data) +{ + size_t i; + + virBufferAsprintf(buf, "<domain>%d</domain>\n", + data->pci_dev.domain); + virBufferAsprintf(buf, "<bus>%d</bus>\n", data->pci_dev.bus); + virBufferAsprintf(buf, "<slot>%d</slot>\n", + data->pci_dev.slot); + virBufferAsprintf(buf, "<function>%d</function>\n", + data->pci_dev.function); + virBufferAsprintf(buf, "<product id='0x%04x'", + data->pci_dev.product); + if (data->pci_dev.product_name) + virBufferEscapeString(buf, ">%s</product>\n", + data->pci_dev.product_name); + else + virBufferAddLit(buf, " />\n"); + virBufferAsprintf(buf, "<vendor id='0x%04x'", + data->pci_dev.vendor); + if (data->pci_dev.vendor_name) + virBufferEscapeString(buf, ">%s</vendor>\n", + data->pci_dev.vendor_name); + else + virBufferAddLit(buf, " />\n"); + if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION) { + virBufferAddLit(buf, "<capability type='phys_function'>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, + "<address domain='0x%.4x' bus='0x%.2x' " + "slot='0x%.2x' function='0x%.1x'/>\n", + data->pci_dev.physical_function->domain, + data->pci_dev.physical_function->bus, + data->pci_dev.physical_function->slot, + data->pci_dev.physical_function->function); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</capability>\n"); + } + if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION) { + virBufferAddLit(buf, "<capability type='virt_functions'"); + if (data->pci_dev.max_virtual_functions) + virBufferAsprintf(buf, " maxCount='%u'", + data->pci_dev.max_virtual_functions); + if (data->pci_dev.num_virtual_functions == 0) { + virBufferAddLit(buf, "/>\n"); + } else { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + for (i = 0; i < data->pci_dev.num_virtual_functions; i++) { + virBufferAsprintf(buf, + "<address domain='0x%.4x' bus='0x%.2x' " + "slot='0x%.2x' function='0x%.1x'/>\n", + data->pci_dev.virtual_functions[i]->domain, + data->pci_dev.virtual_functions[i]->bus, + data->pci_dev.virtual_functions[i]->slot, + data->pci_dev.virtual_functions[i]->function); + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</capability>\n"); + } + } + if (data->pci_dev.hdrType) { + virBufferAsprintf(buf, "<capability type='%s'/>\n", + virPCIHeaderTypeToString(data->pci_dev.hdrType)); + } + if (data->pci_dev.nIommuGroupDevices) { + virBufferAsprintf(buf, "<iommuGroup number='%d'>\n", + data->pci_dev.iommuGroupNumber); + virBufferAdjustIndent(buf, 2); + for (i = 0; i < data->pci_dev.nIommuGroupDevices; i++) { + virBufferAsprintf(buf, + "<address domain='0x%.4x' bus='0x%.2x' " + "slot='0x%.2x' function='0x%.1x'/>\n", + data->pci_dev.iommuGroupDevices[i]->domain, + data->pci_dev.iommuGroupDevices[i]->bus, + data->pci_dev.iommuGroupDevices[i]->slot, + data->pci_dev.iommuGroupDevices[i]->function); + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</iommuGroup>\n"); + } + if (data->pci_dev.numa_node >= 0) + virBufferAsprintf(buf, "<numa node='%d'/>\n", + data->pci_dev.numa_node); + + if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCIE) + virPCIEDeviceInfoFormat(buf, data->pci_dev.pci_express); +} + + +static void +virNodeDeviceCapUSBDevDefFormat(virBufferPtr buf, + const virNodeDevCapData *data) +{ + virBufferAsprintf(buf, "<bus>%d</bus>\n", data->usb_dev.bus); + virBufferAsprintf(buf, "<device>%d</device>\n", + data->usb_dev.device); + virBufferAsprintf(buf, "<product id='0x%04x'", + data->usb_dev.product); + if (data->usb_dev.product_name) + virBufferEscapeString(buf, ">%s</product>\n", + data->usb_dev.product_name); + else + virBufferAddLit(buf, " />\n"); + virBufferAsprintf(buf, "<vendor id='0x%04x'", + data->usb_dev.vendor); + if (data->usb_dev.vendor_name) + virBufferEscapeString(buf, ">%s</vendor>\n", + data->usb_dev.vendor_name); + else + virBufferAddLit(buf, " />\n"); +} + + +static void +virNodeDeviceCapUSBInterfaceDefFormat(virBufferPtr buf, + const virNodeDevCapData *data) +{ + virBufferAsprintf(buf, "<number>%d</number>\n", + data->usb_if.number); + virBufferAsprintf(buf, "<class>%d</class>\n", + data->usb_if._class); + virBufferAsprintf(buf, "<subclass>%d</subclass>\n", + data->usb_if.subclass); + virBufferAsprintf(buf, "<protocol>%d</protocol>\n", + data->usb_if.protocol); + if (data->usb_if.description) + virBufferEscapeString(buf, + "<description>%s</description>\n", + data->usb_if.description); +} + + +static void +virNodeDeviceCapNetDefFormat(virBufferPtr buf, + const virNodeDevCapData *data) +{ + size_t i; + + virBufferEscapeString(buf, "<interface>%s</interface>\n", + data->net.ifname); + if (data->net.address) + virBufferEscapeString(buf, "<address>%s</address>\n", + data->net.address); + virInterfaceLinkFormat(buf, &data->net.lnk); + if (data->net.features) { + for (i = 0; i < VIR_NET_DEV_FEAT_LAST; i++) { + if (virBitmapIsBitSet(data->net.features, i)) { + virBufferAsprintf(buf, "<feature name='%s'/>\n", + virNetDevFeatureTypeToString(i)); + } + } + } + if (data->net.subtype != VIR_NODE_DEV_CAP_NET_LAST) { + const char *subtyp = + virNodeDevNetCapTypeToString(data->net.subtype); + virBufferEscapeString(buf, "<capability type='%s'/>\n", + subtyp); + } +} + + +static void +virNodeDeviceCapSCSIHostDefFormat(virBufferPtr buf, + const virNodeDevCapData *data) +{ + virBufferAsprintf(buf, "<host>%d</host>\n", + data->scsi_host.host); + if (data->scsi_host.unique_id != -1) + virBufferAsprintf(buf, "<unique_id>%d</unique_id>\n", + data->scsi_host.unique_id); + if (data->scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { + virBufferAddLit(buf, "<capability type='fc_host'>\n"); + virBufferAdjustIndent(buf, 2); + virBufferEscapeString(buf, "<wwnn>%s</wwnn>\n", + data->scsi_host.wwnn); + virBufferEscapeString(buf, "<wwpn>%s</wwpn>\n", + data->scsi_host.wwpn); + virBufferEscapeString(buf, "<fabric_wwn>%s</fabric_wwn>\n", + data->scsi_host.fabric_wwn); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</capability>\n"); + } + if (data->scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS) { + virBufferAddLit(buf, "<capability type='vport_ops'>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<max_vports>%d</max_vports>\n", + data->scsi_host.max_vports); + virBufferAsprintf(buf, "<vports>%d</vports>\n", + data->scsi_host.vports); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</capability>\n"); + } +} + + +static void +virNodeDeviceCapSCSIDefFormat(virBufferPtr buf, + const virNodeDevCapData *data) +{ + virBufferAsprintf(buf, "<host>%d</host>\n", data->scsi.host); + virBufferAsprintf(buf, "<bus>%d</bus>\n", data->scsi.bus); + virBufferAsprintf(buf, "<target>%d</target>\n", + data->scsi.target); + virBufferAsprintf(buf, "<lun>%d</lun>\n", data->scsi.lun); + if (data->scsi.type) + virBufferEscapeString(buf, "<type>%s</type>\n", + data->scsi.type); +} + + +static void +virNodeDeviceCapStorageDefFormat(virBufferPtr buf, + const virNodeDevCapData *data) +{ + virBufferEscapeString(buf, "<block>%s</block>\n", + data->storage.block); + if (data->storage.bus) + virBufferEscapeString(buf, "<bus>%s</bus>\n", + data->storage.bus); + if (data->storage.drive_type) + virBufferEscapeString(buf, "<drive_type>%s</drive_type>\n", + data->storage.drive_type); + if (data->storage.model) + virBufferEscapeString(buf, "<model>%s</model>\n", + data->storage.model); + if (data->storage.vendor) + virBufferEscapeString(buf, "<vendor>%s</vendor>\n", + data->storage.vendor); + if (data->storage.serial) + virBufferEscapeString(buf, "<serial>%s</serial>\n", + data->storage.serial); + if (data->storage.flags & VIR_NODE_DEV_CAP_STORAGE_REMOVABLE) { + int avl = data->storage.flags & + VIR_NODE_DEV_CAP_STORAGE_REMOVABLE_MEDIA_AVAILABLE; + virBufferAddLit(buf, "<capability type='removable'>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<media_available>%d" + "</media_available>\n", avl ? 1 : 0); + virBufferAsprintf(buf, "<media_size>%llu</media_size>\n", + data->storage.removable_media_size); + if (data->storage.media_label) + virBufferEscapeString(buf, + "<media_label>%s</media_label>\n", + data->storage.media_label); + if (data->storage.logical_block_size > 0) + virBufferAsprintf(buf, "<logical_block_size>%llu" + "</logical_block_size>\n", + data->storage.logical_block_size); + if (data->storage.num_blocks > 0) + virBufferAsprintf(buf, + "<num_blocks>%llu</num_blocks>\n", + data->storage.num_blocks); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</capability>\n"); + } else { + virBufferAsprintf(buf, "<size>%llu</size>\n", + data->storage.size); + if (data->storage.logical_block_size > 0) + virBufferAsprintf(buf, "<logical_block_size>%llu" + "</logical_block_size>\n", + data->storage.logical_block_size); + if (data->storage.num_blocks > 0) + virBufferAsprintf(buf, "<num_blocks>%llu</num_blocks>\n", + data->storage.num_blocks); + } + if (data->storage.flags & VIR_NODE_DEV_CAP_STORAGE_HOTPLUGGABLE) + virBufferAddLit(buf, "<capability type='hotpluggable'/>\n"); +} + + char * virNodeDeviceDefFormat(const virNodeDeviceDef *def) { @@ -185,7 +499,6 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def) } for (caps = def->caps; caps; caps = caps->next) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; virNodeDevCapDataPtr data = &caps->data; virBufferAsprintf(&buf, "<capability type='%s'>\n", @@ -193,279 +506,32 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def) virBufferAdjustIndent(&buf, 2); switch (caps->data.type) { case VIR_NODE_DEV_CAP_SYSTEM: - if (data->system.product_name) - virBufferEscapeString(&buf, "<product>%s</product>\n", - data->system.product_name); - virBufferAddLit(&buf, "<hardware>\n"); - virBufferAdjustIndent(&buf, 2); - if (data->system.hardware.vendor_name) - virBufferEscapeString(&buf, "<vendor>%s</vendor>\n", - data->system.hardware.vendor_name); - if (data->system.hardware.version) - virBufferEscapeString(&buf, "<version>%s</version>\n", - data->system.hardware.version); - if (data->system.hardware.serial) - virBufferEscapeString(&buf, "<serial>%s</serial>\n", - data->system.hardware.serial); - virUUIDFormat(data->system.hardware.uuid, uuidstr); - virBufferAsprintf(&buf, "<uuid>%s</uuid>\n", uuidstr); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</hardware>\n"); - - virBufferAddLit(&buf, "<firmware>\n"); - virBufferAdjustIndent(&buf, 2); - if (data->system.firmware.vendor_name) - virBufferEscapeString(&buf, "<vendor>%s</vendor>\n", - data->system.firmware.vendor_name); - if (data->system.firmware.version) - virBufferEscapeString(&buf, "<version>%s</version>\n", - data->system.firmware.version); - if (data->system.firmware.release_date) - virBufferEscapeString(&buf, "<release_date>%s</release_date>\n", - data->system.firmware.release_date); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</firmware>\n"); + virNodeDeviceCapSystemDefFormat(&buf, data); break; case VIR_NODE_DEV_CAP_PCI_DEV: - virBufferAsprintf(&buf, "<domain>%d</domain>\n", - data->pci_dev.domain); - virBufferAsprintf(&buf, "<bus>%d</bus>\n", data->pci_dev.bus); - virBufferAsprintf(&buf, "<slot>%d</slot>\n", - data->pci_dev.slot); - virBufferAsprintf(&buf, "<function>%d</function>\n", - data->pci_dev.function); - virBufferAsprintf(&buf, "<product id='0x%04x'", - data->pci_dev.product); - if (data->pci_dev.product_name) - virBufferEscapeString(&buf, ">%s</product>\n", - data->pci_dev.product_name); - else - virBufferAddLit(&buf, " />\n"); - virBufferAsprintf(&buf, "<vendor id='0x%04x'", - data->pci_dev.vendor); - if (data->pci_dev.vendor_name) - virBufferEscapeString(&buf, ">%s</vendor>\n", - data->pci_dev.vendor_name); - else - virBufferAddLit(&buf, " />\n"); - if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION) { - virBufferAddLit(&buf, "<capability type='phys_function'>\n"); - virBufferAdjustIndent(&buf, 2); - virBufferAsprintf(&buf, - "<address domain='0x%.4x' bus='0x%.2x' " - "slot='0x%.2x' function='0x%.1x'/>\n", - data->pci_dev.physical_function->domain, - data->pci_dev.physical_function->bus, - data->pci_dev.physical_function->slot, - data->pci_dev.physical_function->function); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</capability>\n"); - } - if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION) { - virBufferAddLit(&buf, "<capability type='virt_functions'"); - if (data->pci_dev.max_virtual_functions) - virBufferAsprintf(&buf, " maxCount='%u'", - data->pci_dev.max_virtual_functions); - if (data->pci_dev.num_virtual_functions == 0) { - virBufferAddLit(&buf, "/>\n"); - } else { - virBufferAddLit(&buf, ">\n"); - virBufferAdjustIndent(&buf, 2); - for (i = 0; i < data->pci_dev.num_virtual_functions; i++) { - virBufferAsprintf(&buf, - "<address domain='0x%.4x' bus='0x%.2x' " - "slot='0x%.2x' function='0x%.1x'/>\n", - data->pci_dev.virtual_functions[i]->domain, - data->pci_dev.virtual_functions[i]->bus, - data->pci_dev.virtual_functions[i]->slot, - data->pci_dev.virtual_functions[i]->function); - } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</capability>\n"); - } - } - if (data->pci_dev.hdrType) { - virBufferAsprintf(&buf, "<capability type='%s'/>\n", - virPCIHeaderTypeToString(data->pci_dev.hdrType)); - } - if (data->pci_dev.nIommuGroupDevices) { - virBufferAsprintf(&buf, "<iommuGroup number='%d'>\n", - data->pci_dev.iommuGroupNumber); - virBufferAdjustIndent(&buf, 2); - for (i = 0; i < data->pci_dev.nIommuGroupDevices; i++) { - virBufferAsprintf(&buf, - "<address domain='0x%.4x' bus='0x%.2x' " - "slot='0x%.2x' function='0x%.1x'/>\n", - data->pci_dev.iommuGroupDevices[i]->domain, - data->pci_dev.iommuGroupDevices[i]->bus, - data->pci_dev.iommuGroupDevices[i]->slot, - data->pci_dev.iommuGroupDevices[i]->function); - } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</iommuGroup>\n"); - } - if (data->pci_dev.numa_node >= 0) - virBufferAsprintf(&buf, "<numa node='%d'/>\n", - data->pci_dev.numa_node); - - if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCIE) - virPCIEDeviceInfoFormat(&buf, data->pci_dev.pci_express); + virNodeDeviceCapPCIDefFormat(&buf, data); break; case VIR_NODE_DEV_CAP_USB_DEV: - virBufferAsprintf(&buf, "<bus>%d</bus>\n", data->usb_dev.bus); - virBufferAsprintf(&buf, "<device>%d</device>\n", - data->usb_dev.device); - virBufferAsprintf(&buf, "<product id='0x%04x'", - data->usb_dev.product); - if (data->usb_dev.product_name) - virBufferEscapeString(&buf, ">%s</product>\n", - data->usb_dev.product_name); - else - virBufferAddLit(&buf, " />\n"); - virBufferAsprintf(&buf, "<vendor id='0x%04x'", - data->usb_dev.vendor); - if (data->usb_dev.vendor_name) - virBufferEscapeString(&buf, ">%s</vendor>\n", - data->usb_dev.vendor_name); - else - virBufferAddLit(&buf, " />\n"); + virNodeDeviceCapUSBDevDefFormat(&buf, data); break; case VIR_NODE_DEV_CAP_USB_INTERFACE: - virBufferAsprintf(&buf, "<number>%d</number>\n", - data->usb_if.number); - virBufferAsprintf(&buf, "<class>%d</class>\n", - data->usb_if._class); - virBufferAsprintf(&buf, "<subclass>%d</subclass>\n", - data->usb_if.subclass); - virBufferAsprintf(&buf, "<protocol>%d</protocol>\n", - data->usb_if.protocol); - if (data->usb_if.description) - virBufferEscapeString(&buf, - "<description>%s</description>\n", - data->usb_if.description); + virNodeDeviceCapUSBInterfaceDefFormat(&buf, data); break; case VIR_NODE_DEV_CAP_NET: - virBufferEscapeString(&buf, "<interface>%s</interface>\n", - data->net.ifname); - if (data->net.address) - virBufferEscapeString(&buf, "<address>%s</address>\n", - data->net.address); - virInterfaceLinkFormat(&buf, &data->net.lnk); - if (data->net.features) { - for (i = 0; i < VIR_NET_DEV_FEAT_LAST; i++) { - if (virBitmapIsBitSet(data->net.features, i)) { - virBufferAsprintf(&buf, "<feature name='%s'/>\n", - virNetDevFeatureTypeToString(i)); - } - } - } - if (data->net.subtype != VIR_NODE_DEV_CAP_NET_LAST) { - const char *subtyp = - virNodeDevNetCapTypeToString(data->net.subtype); - virBufferEscapeString(&buf, "<capability type='%s'/>\n", - subtyp); - } + virNodeDeviceCapNetDefFormat(&buf, data); break; case VIR_NODE_DEV_CAP_SCSI_HOST: - virBufferAsprintf(&buf, "<host>%d</host>\n", - data->scsi_host.host); - if (data->scsi_host.unique_id != -1) - virBufferAsprintf(&buf, "<unique_id>%d</unique_id>\n", - data->scsi_host.unique_id); - if (data->scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { - virBufferAddLit(&buf, "<capability type='fc_host'>\n"); - virBufferAdjustIndent(&buf, 2); - virBufferEscapeString(&buf, "<wwnn>%s</wwnn>\n", - data->scsi_host.wwnn); - virBufferEscapeString(&buf, "<wwpn>%s</wwpn>\n", - data->scsi_host.wwpn); - virBufferEscapeString(&buf, "<fabric_wwn>%s</fabric_wwn>\n", - data->scsi_host.fabric_wwn); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</capability>\n"); - } - if (data->scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS) { - virBufferAddLit(&buf, "<capability type='vport_ops'>\n"); - virBufferAdjustIndent(&buf, 2); - virBufferAsprintf(&buf, "<max_vports>%d</max_vports>\n", - data->scsi_host.max_vports); - virBufferAsprintf(&buf, "<vports>%d</vports>\n", - data->scsi_host.vports); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</capability>\n"); - } - + virNodeDeviceCapSCSIHostDefFormat(&buf, data); break; - case VIR_NODE_DEV_CAP_SCSI_TARGET: virBufferEscapeString(&buf, "<target>%s</target>\n", data->scsi_target.name); break; - case VIR_NODE_DEV_CAP_SCSI: - virBufferAsprintf(&buf, "<host>%d</host>\n", data->scsi.host); - virBufferAsprintf(&buf, "<bus>%d</bus>\n", data->scsi.bus); - virBufferAsprintf(&buf, "<target>%d</target>\n", - data->scsi.target); - virBufferAsprintf(&buf, "<lun>%d</lun>\n", data->scsi.lun); - if (data->scsi.type) - virBufferEscapeString(&buf, "<type>%s</type>\n", - data->scsi.type); + virNodeDeviceCapSCSIDefFormat(&buf, data); break; case VIR_NODE_DEV_CAP_STORAGE: - virBufferEscapeString(&buf, "<block>%s</block>\n", - data->storage.block); - if (data->storage.bus) - virBufferEscapeString(&buf, "<bus>%s</bus>\n", - data->storage.bus); - if (data->storage.drive_type) - virBufferEscapeString(&buf, "<drive_type>%s</drive_type>\n", - data->storage.drive_type); - if (data->storage.model) - virBufferEscapeString(&buf, "<model>%s</model>\n", - data->storage.model); - if (data->storage.vendor) - virBufferEscapeString(&buf, "<vendor>%s</vendor>\n", - data->storage.vendor); - if (data->storage.serial) - virBufferEscapeString(&buf, "<serial>%s</serial>\n", - data->storage.serial); - if (data->storage.flags & VIR_NODE_DEV_CAP_STORAGE_REMOVABLE) { - int avl = data->storage.flags & - VIR_NODE_DEV_CAP_STORAGE_REMOVABLE_MEDIA_AVAILABLE; - virBufferAddLit(&buf, "<capability type='removable'>\n"); - virBufferAdjustIndent(&buf, 2); - virBufferAsprintf(&buf, "<media_available>%d" - "</media_available>\n", avl ? 1 : 0); - virBufferAsprintf(&buf, "<media_size>%llu</media_size>\n", - data->storage.removable_media_size); - if (data->storage.media_label) - virBufferEscapeString(&buf, - "<media_label>%s</media_label>\n", - data->storage.media_label); - if (data->storage.logical_block_size > 0) - virBufferAsprintf(&buf, "<logical_block_size>%llu" - "</logical_block_size>\n", - data->storage.logical_block_size); - if (data->storage.num_blocks > 0) - virBufferAsprintf(&buf, - "<num_blocks>%llu</num_blocks>\n", - data->storage.num_blocks); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</capability>\n"); - } else { - virBufferAsprintf(&buf, "<size>%llu</size>\n", - data->storage.size); - if (data->storage.logical_block_size > 0) - virBufferAsprintf(&buf, "<logical_block_size>%llu" - "</logical_block_size>\n", - data->storage.logical_block_size); - if (data->storage.num_blocks > 0) - virBufferAsprintf(&buf, "<num_blocks>%llu</num_blocks>\n", - data->storage.num_blocks); - } - if (data->storage.flags & VIR_NODE_DEV_CAP_STORAGE_HOTPLUGGABLE) - virBufferAddLit(&buf, "<capability type='hotpluggable'/>\n"); + virNodeDeviceCapStorageDefFormat(&buf, data); break; case VIR_NODE_DEV_CAP_SCSI_GENERIC: virBufferEscapeString(&buf, "<char>%s</char>\n", -- 2.12.2

On Wed, Mar 29, 2017 at 14:51:13 +0200, Erik Skultety wrote:
Make the code look cleaner by moving the capability specific bits into separate functions.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/node_device_conf.c | 578 ++++++++++++++++++++++++-------------------- 1 file changed, 322 insertions(+), 256 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 7d0baa9d1a..72fb9a5611 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -155,6 +155,320 @@ virPCIEDeviceInfoFormat(virBufferPtr buf, }
+static void +virNodeDeviceCapSystemDefFormat(virBufferPtr buf, + const virNodeDevCapData *data) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (data->system.product_name) + virBufferEscapeString(buf, "<product>%s</product>\n", + data->system.product_name); + virBufferAddLit(buf, "<hardware>\n"); + virBufferAdjustIndent(buf, 2); + if (data->system.hardware.vendor_name) + virBufferEscapeString(buf, "<vendor>%s</vendor>\n", + data->system.hardware.vendor_name); + if (data->system.hardware.version) + virBufferEscapeString(buf, "<version>%s</version>\n", + data->system.hardware.version);
virBufferEscapeString automatically skips formatting of the whole string if the argument is NULL. So the condition is not necessary.

On Wed, Mar 29, 2017 at 03:51:54PM +0200, Peter Krempa wrote:
On Wed, Mar 29, 2017 at 14:51:13 +0200, Erik Skultety wrote:
Make the code look cleaner by moving the capability specific bits into separate functions.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/node_device_conf.c | 578 ++++++++++++++++++++++++-------------------- 1 file changed, 322 insertions(+), 256 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 7d0baa9d1a..72fb9a5611 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -155,6 +155,320 @@ virPCIEDeviceInfoFormat(virBufferPtr buf, }
+static void +virNodeDeviceCapSystemDefFormat(virBufferPtr buf, + const virNodeDevCapData *data) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (data->system.product_name) + virBufferEscapeString(buf, "<product>%s</product>\n", + data->system.product_name); + virBufferAddLit(buf, "<hardware>\n"); + virBufferAdjustIndent(buf, 2); + if (data->system.hardware.vendor_name) + virBufferEscapeString(buf, "<vendor>%s</vendor>\n", + data->system.hardware.vendor_name); + if (data->system.hardware.version) + virBufferEscapeString(buf, "<version>%s</version>\n", + data->system.hardware.version);
virBufferEscapeString automatically skips formatting of the whole string if the argument is NULL. So the condition is not necessary.
Again, you're right, but in this case, I was merely moving bits to separate functions without reviewing the code. Also, it's a fair amount of occurrences, so for the sake of the future reviewer, aka non-shredder :D, I will adjust this in a separate patch. Erik

Since we have that information provided by @def which is not a private object, there is really no need for the variable. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_udev.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 976ccad710..36ed92f712 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -328,7 +328,6 @@ static int udevProcessPCI(struct udev_device *device, virNodeDeviceDefPtr def) { - const char *syspath = NULL; virNodeDevCapPCIDevPtr pci_dev = &def->caps->data.pci_dev; virPCIEDeviceInfoPtr pci_express = NULL; virPCIDevicePtr pciDev = NULL; @@ -336,19 +335,17 @@ udevProcessPCI(struct udev_device *device, int ret = -1; char *p; - syspath = udev_device_get_syspath(device); - if (udevGetUintProperty(device, "PCI_CLASS", &pci_dev->class, 16) < 0) goto cleanup; - if ((p = strrchr(syspath, '/')) == NULL || + if ((p = strrchr(def->sysfs_path, '/')) == NULL || virStrToLong_ui(p + 1, &p, 16, &pci_dev->domain) < 0 || p == NULL || virStrToLong_ui(p + 1, &p, 16, &pci_dev->bus) < 0 || p == NULL || virStrToLong_ui(p + 1, &p, 16, &pci_dev->slot) < 0 || p == NULL || virStrToLong_ui(p + 1, &p, 16, &pci_dev->function) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse the PCI address from sysfs path: '%s'"), - syspath); + def->sysfs_path); goto cleanup; } @@ -375,8 +372,7 @@ udevProcessPCI(struct udev_device *device, &pci_dev->numa_node, 10) < 0) goto cleanup; - if (nodeDeviceSysfsGetPCIRelatedDevCaps(syspath, - &def->caps->data.pci_dev) < 0) + if (nodeDeviceSysfsGetPCIRelatedDevCaps(def->sysfs_path, pci_dev) < 0) goto cleanup; if (!(pciDev = virPCIDeviceNew(pci_dev->domain, -- 2.12.2

Since we do have this template at hand, why not using it wherever possible. Also, just to be grammatically correct, let's use singular, aka 'pool' instead of plural in the enumerated list of supported types. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/storage.html.in | 62 ++++++++++------------------------------------------ 1 file changed, 11 insertions(+), 51 deletions(-) diff --git a/docs/storage.html.in b/docs/storage.html.in index 5e18f02c58..2487ede67b 100644 --- a/docs/storage.html.in +++ b/docs/storage.html.in @@ -83,47 +83,7 @@ <p> Libvirt supports the following storage pool types: </p> - <ul> - <li> - <a href="#StorageBackendDir">Directory backend</a> - </li> - <li> - <a href="#StorageBackendFS">Local filesystem backend</a> - </li> - <li> - <a href="#StorageBackendNetFS">Network filesystem backend</a> - </li> - <li> - <a href="#StorageBackendLogical">Logical backend</a> - </li> - <li> - <a href="#StorageBackendDisk">Disk backend</a> - </li> - <li> - <a href="#StorageBackendISCSI">iSCSI backend</a> - </li> - <li> - <a href="#StorageBackendSCSI">SCSI backend</a> - </li> - <li> - <a href="#StorageBackendMultipath">Multipath backend</a> - </li> - <li> - <a href="#StorageBackendRBD">RBD (RADOS Block Device) backend</a> - </li> - <li> - <a href="#StorageBackendSheepdog">Sheepdog backend</a> - </li> - <li> - <a href="#StorageBackendGluster">Gluster backend</a> - </li> - <li> - <a href="#StorageBackendZFS">ZFS backend</a> - </li> - <li> - <a href="#StorageBackendVstorage">Virtuozzo storage backend</a> - </li> - </ul> + <ul id="toc"></ul> <h2><a name="StorageBackendDir">Directory pool</a></h2> <p> @@ -306,7 +266,7 @@ </p> - <h2><a name="StorageBackendLogical">Logical volume pools</a></h2> + <h2><a name="StorageBackendLogical">Logical volume pool</a></h2> <p> This provides a pool based on an LVM volume group. For a pre-defined LVM volume group, simply providing the group @@ -343,7 +303,7 @@ </p> - <h2><a name="StorageBackendDisk">Disk volume pools</a></h2> + <h2><a name="StorageBackendDisk">Disk volume pool</a></h2> <p> This provides a pool based on a physical disk. Volumes are created by adding partitions to the disk. Disk pools have constraints @@ -434,7 +394,7 @@ </ul> - <h2><a name="StorageBackendISCSI">iSCSI volume pools</a></h2> + <h2><a name="StorageBackendISCSI">iSCSI volume pool</a></h2> <p> This provides a pool based on an iSCSI target. Volumes must be pre-allocated on the iSCSI server, and cannot be created via @@ -473,7 +433,7 @@ The iSCSI volume pool does not use the volume format type element. </p> - <h2><a name="StorageBackendSCSI">SCSI volume pools</a></h2> + <h2><a name="StorageBackendSCSI">SCSI volume pool</a></h2> <p> This provides a pool based on a SCSI HBA. Volumes are preexisting SCSI LUNs, and cannot be created via the libvirt APIs. Since /dev/XXX names @@ -505,7 +465,7 @@ The SCSI volume pool does not use the volume format type element. </p> - <h2><a name="StorageBackendMultipath">Multipath pools</a></h2> + <h2><a name="StorageBackendMultipath">Multipath pool</a></h2> <p> This provides a pool that contains all the multipath devices on the host. Therefore, only one Multipath pool may be configured per host. @@ -538,7 +498,7 @@ The Multipath volume pool does not use the volume format type element. </p> - <h2><a name="StorageBackendRBD">RBD pools</a></h2> + <h2><a name="StorageBackendRBD">RBD pool</a></h2> <p> This storage driver provides a pool which contains all RBD images in a RADOS pool. RBD (RADOS Block Device) is part @@ -611,7 +571,7 @@ The RBD pool does not use the volume format type element. </p> - <h2><a name="StorageBackendSheepdog">Sheepdog pools</a></h2> + <h2><a name="StorageBackendSheepdog">Sheepdog pool</a></h2> <p> This provides a pool based on a Sheepdog Cluster. Sheepdog is a distributed storage system for QEMU/KVM. @@ -670,7 +630,7 @@ The Sheepdog pool does not use the volume format type element. </p> - <h2><a name="StorageBackendGluster">Gluster pools</a></h2> + <h2><a name="StorageBackendGluster">Gluster pool</a></h2> <p> This provides a pool based on native Gluster access. Gluster is a distributed file system that can be exposed to the user via @@ -756,7 +716,7 @@ pool type. </p> - <h2><a name="StorageBackendZFS">ZFS pools</a></h2> + <h2><a name="StorageBackendZFS">ZFS pool</a></h2> <p> This provides a pool based on the ZFS filesystem. Initially it was developed for FreeBSD, and <span class="since">since 1.3.2</span> experimental support @@ -794,7 +754,7 @@ <p> The ZFS volume pool does not use the volume format type element. </p> - <h2><a name="StorageBackendVstorage">Vstorage pools</a></h2> + <h2><a name="StorageBackendVstorage">Vstorage pool</a></h2> <p> This provides a pool based on Virtuozzo storage. Virtuozzo Storage is a highly available distributed software-defined storage with built-in -- 2.12.2

On Wed, Mar 29, 2017 at 02:51:15PM +0200, Erik Skultety wrote:
Since we do have this template at hand, why not using it wherever possible. Also, just to be grammatically correct, let's use singular, aka 'pool' instead of plural in the enumerated list of supported types.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/storage.html.in | 62 ++++++++++------------------------------------------ 1 file changed, 11 insertions(+), 51 deletions(-)
diff --git a/docs/storage.html.in b/docs/storage.html.in index 5e18f02c58..2487ede67b 100644 --- a/docs/storage.html.in +++ b/docs/storage.html.in @@ -83,47 +83,7 @@ <p> Libvirt supports the following storage pool types: </p> - <ul> - <li> - <a href="#StorageBackendDir">Directory backend</a> - </li> - <li> - <a href="#StorageBackendFS">Local filesystem backend</a> - </li> - <li> - <a href="#StorageBackendNetFS">Network filesystem backend</a> - </li> - <li> - <a href="#StorageBackendLogical">Logical backend</a> - </li> - <li> - <a href="#StorageBackendDisk">Disk backend</a> - </li> - <li> - <a href="#StorageBackendISCSI">iSCSI backend</a> - </li> - <li> - <a href="#StorageBackendSCSI">SCSI backend</a> - </li> - <li> - <a href="#StorageBackendMultipath">Multipath backend</a> - </li> - <li> - <a href="#StorageBackendRBD">RBD (RADOS Block Device) backend</a> - </li> - <li> - <a href="#StorageBackendSheepdog">Sheepdog backend</a> - </li> - <li> - <a href="#StorageBackendGluster">Gluster backend</a> - </li> - <li> - <a href="#StorageBackendZFS">ZFS backend</a> - </li> - <li> - <a href="#StorageBackendVstorage">Virtuozzo storage backend</a> - </li> - </ul> + <ul id="toc"></ul>
This makes sense,
<h2><a name="StorageBackendDir">Directory pool</a></h2> <p> @@ -306,7 +266,7 @@ </p>
- <h2><a name="StorageBackendLogical">Logical volume pools</a></h2> + <h2><a name="StorageBackendLogical">Logical volume pool</a></h2> <p> This provides a pool based on an LVM volume group. For a pre-defined LVM volume group, simply providing the group @@ -343,7 +303,7 @@ </p>
- <h2><a name="StorageBackendDisk">Disk volume pools</a></h2> + <h2><a name="StorageBackendDisk">Disk volume pool</a></h2>
but since the list of supported storage pools is generated from these heading elements I think that this patch should cleanup more than just changing plural to singular. I would also remove the "volume" from all storage pools except Logical volume pool. It also changes "backend" to "pool" but I guess that's OK. Pavel
<p> This provides a pool based on a physical disk. Volumes are created by adding partitions to the disk. Disk pools have constraints @@ -434,7 +394,7 @@ </ul>
- <h2><a name="StorageBackendISCSI">iSCSI volume pools</a></h2> + <h2><a name="StorageBackendISCSI">iSCSI volume pool</a></h2> <p> This provides a pool based on an iSCSI target. Volumes must be pre-allocated on the iSCSI server, and cannot be created via @@ -473,7 +433,7 @@ The iSCSI volume pool does not use the volume format type element. </p>
- <h2><a name="StorageBackendSCSI">SCSI volume pools</a></h2> + <h2><a name="StorageBackendSCSI">SCSI volume pool</a></h2> <p> This provides a pool based on a SCSI HBA. Volumes are preexisting SCSI LUNs, and cannot be created via the libvirt APIs. Since /dev/XXX names @@ -505,7 +465,7 @@ The SCSI volume pool does not use the volume format type element. </p>
- <h2><a name="StorageBackendMultipath">Multipath pools</a></h2> + <h2><a name="StorageBackendMultipath">Multipath pool</a></h2> <p> This provides a pool that contains all the multipath devices on the host. Therefore, only one Multipath pool may be configured per host. @@ -538,7 +498,7 @@ The Multipath volume pool does not use the volume format type element. </p>
- <h2><a name="StorageBackendRBD">RBD pools</a></h2> + <h2><a name="StorageBackendRBD">RBD pool</a></h2> <p> This storage driver provides a pool which contains all RBD images in a RADOS pool. RBD (RADOS Block Device) is part @@ -611,7 +571,7 @@ The RBD pool does not use the volume format type element. </p>
- <h2><a name="StorageBackendSheepdog">Sheepdog pools</a></h2> + <h2><a name="StorageBackendSheepdog">Sheepdog pool</a></h2> <p> This provides a pool based on a Sheepdog Cluster. Sheepdog is a distributed storage system for QEMU/KVM. @@ -670,7 +630,7 @@ The Sheepdog pool does not use the volume format type element. </p>
- <h2><a name="StorageBackendGluster">Gluster pools</a></h2> + <h2><a name="StorageBackendGluster">Gluster pool</a></h2> <p> This provides a pool based on native Gluster access. Gluster is a distributed file system that can be exposed to the user via @@ -756,7 +716,7 @@ pool type. </p>
- <h2><a name="StorageBackendZFS">ZFS pools</a></h2> + <h2><a name="StorageBackendZFS">ZFS pool</a></h2> <p> This provides a pool based on the ZFS filesystem. Initially it was developed for FreeBSD, and <span class="since">since 1.3.2</span> experimental support @@ -794,7 +754,7 @@ <p> The ZFS volume pool does not use the volume format type element. </p> - <h2><a name="StorageBackendVstorage">Vstorage pools</a></h2> + <h2><a name="StorageBackendVstorage">Vstorage pool</a></h2> <p> This provides a pool based on Virtuozzo storage. Virtuozzo Storage is a highly available distributed software-defined storage with built-in -- 2.12.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Besides updating the capability enum, this patch introduces 'virNodeDevCapMdev' structure which will store everything libvirt can gather from sysfs about a mediated device. Since we need to report the info for both the mediated child device it's parent mdev capabilities (merely the mdev types' attributes), this structure serves in both of these cases with the difference being that the amount of data it holds depends on the specific scenario (child vs parent). Signed-off-by: Erik Skultety <eskultet@redhat.com> --- include/libvirt/libvirt-nodedev.h | 1 + src/conf/node_device_conf.c | 19 ++++++++++++++++++- src/conf/node_device_conf.h | 19 ++++++++++++++++++- src/conf/virnodedeviceobj.c | 3 ++- src/libvirt-nodedev.c | 1 + src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 1 + src/node_device/node_device_udev.c | 8 +++++++- tools/virsh-nodedev.c | 2 ++ 9 files changed, 51 insertions(+), 4 deletions(-) diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 85003903d7..59edf4db02 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -79,6 +79,7 @@ typedef enum { VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPORTS = 1 << 10, /* Capable of vport */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_GENERIC = 1 << 11, /* Capable of scsi_generic */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM = 1 << 12, /* DRM device */ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV = 1 << 13, /* Mediated device */ } virConnectListAllNodeDeviceFlags; int virConnectListAllNodeDevices (virConnectPtr conn, diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 72fb9a5611..03d7993ab1 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -60,7 +60,8 @@ VIR_ENUM_IMPL(virNodeDevCap, VIR_NODE_DEV_CAP_LAST, "fc_host", "vports", "scsi_generic", - "drm") + "drm", + "mdev") VIR_ENUM_IMPL(virNodeDevNetCap, VIR_NODE_DEV_CAP_NET_LAST, "80203", @@ -542,6 +543,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def) break; case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_LAST: break; } @@ -1591,6 +1593,7 @@ virNodeDevCapsDefParseXML(xmlXPathContextPtr ctxt, case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown capability type '%d' for '%s'"), @@ -1905,6 +1908,7 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) case VIR_NODE_DEV_CAP_SCSI_GENERIC: VIR_FREE(data->sg.path); break; + case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_DRM: case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: @@ -1917,6 +1921,19 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) } +void virNodeDevCapMdevFree(virNodeDevCapMdevPtr mdev) +{ + if (!mdev) + return; + + VIR_FREE(mdev->type); + VIR_FREE(mdev->name); + VIR_FREE(mdev->description); + VIR_FREE(mdev->device_api); + VIR_FREE(mdev); +} + + /* virNodeDeviceGetParentName * @conn: Connection pointer * @nodedev_name: Node device to lookup diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index a5d5cdd2a5..b7db35dd50 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -64,6 +64,7 @@ typedef enum { VIR_NODE_DEV_CAP_VPORTS, /* HBA which is capable of vports */ VIR_NODE_DEV_CAP_SCSI_GENERIC, /* SCSI generic device */ VIR_NODE_DEV_CAP_DRM, /* DRM device */ + VIR_NODE_DEV_CAP_MDEV, /* Mediated device */ VIR_NODE_DEV_CAP_LAST } virNodeDevCapType; @@ -131,6 +132,17 @@ struct _virNodeDevCapSystem { virNodeDevCapSystemFirmware firmware; }; +typedef struct _virNodeDevCapMdev virNodeDevCapMdev; +typedef virNodeDevCapMdev *virNodeDevCapMdevPtr; +struct _virNodeDevCapMdev { + char *type; + char *name; + char *description; + char *device_api; + unsigned int available_instances; + unsigned int iommuGroupNumber; +}; + typedef struct _virNodeDevCapPCIDev virNodeDevCapPCIDev; typedef virNodeDevCapPCIDev *virNodeDevCapPCIDevPtr; struct _virNodeDevCapPCIDev { @@ -262,6 +274,7 @@ struct _virNodeDevCapData { virNodeDevCapStorage storage; virNodeDevCapSCSIGeneric sg; virNodeDevCapDRM drm; + virNodeDevCapMdev mdev; }; }; @@ -338,6 +351,9 @@ virNodeDeviceDefFree(virNodeDeviceDefPtr def); void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps); +void +virNodeDevCapMdevFree(virNodeDevCapMdevPtr mdev); + # define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP \ (VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV | \ @@ -351,7 +367,8 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps); VIR_CONNECT_LIST_NODE_DEVICES_CAP_FC_HOST | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPORTS | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_GENERIC | \ - VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM) + VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM | \ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV) char * virNodeDeviceGetParentName(virConnectPtr conn, diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 3fe3ae5fe8..1dfb7380c7 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -494,7 +494,8 @@ virNodeDeviceMatch(virNodeDeviceObjPtr devobj, MATCH(FC_HOST) || MATCH(VPORTS) || MATCH(SCSI_GENERIC) || - MATCH(DRM))) + MATCH(DRM) || + MATCH(MDEV))) return false; } diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index 83376b0d96..f9d8edf7e5 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -98,6 +98,7 @@ virNodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags) * VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPORTS * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_GENERIC * VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM + * VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV * * Returns the number of node devices found or -1 and sets @devices to NULL in * case of error. On success, the array stored into @devices is guaranteed to diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b551cb86a6..2a3e8cb705 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -690,6 +690,7 @@ virNetDevIPRouteParseXML; # conf/node_device_conf.h +virNodeDevCapMdevFree; virNodeDevCapsDefFree; virNodeDevCapTypeFromString; virNodeDevCapTypeToString; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 9b1c5bb5e9..bcf204c7b7 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -83,6 +83,7 @@ update_caps(virNodeDeviceObjPtr dev) case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_LAST: break; } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 36ed92f712..dbbd8e5d06 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -43,6 +43,7 @@ #include "virpci.h" #include "virstring.h" #include "virnetdev.h" +#include "virmdev.h" #define VIR_FROM_THIS VIR_FROM_NODEDEV @@ -1051,12 +1052,16 @@ udevGetDeviceType(struct udev_device *device, if (udevHasDeviceProperty(device, "INTERFACE")) *type = VIR_NODE_DEV_CAP_NET; - /* SCSI generic device doesn't set DEVTYPE property */ + /* Neither SCSI generic devices nor mediated devices set DEVTYPE + * property, but they so we need to rely on the SUBSYSTEM property */ if (udevGetStringProperty(device, "SUBSYSTEM", &subsystem) < 0) return -1; if (STREQ_NULLABLE(subsystem, "scsi_generic")) *type = VIR_NODE_DEV_CAP_SCSI_GENERIC; + else if (STREQ_NULLABLE(subsystem, "mdev")) + *type = VIR_NODE_DEV_CAP_MDEV; + VIR_FREE(subsystem); } @@ -1096,6 +1101,7 @@ udevGetDeviceDetails(struct udev_device *device, return udevProcessSCSIGeneric(device, def); case VIR_NODE_DEV_CAP_DRM: return udevProcessDRMDevice(device, def); + case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_SYSTEM: case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index c691440219..19dcfafa6e 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -454,6 +454,8 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) case VIR_NODE_DEV_CAP_DRM: flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM; break; + case VIR_NODE_DEV_CAP_MDEV: + flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV; case VIR_NODE_DEV_CAP_LAST: break; } -- 2.12.2

On Wed, Mar 29, 2017 at 02:51:16PM +0200, Erik Skultety wrote:
Besides updating the capability enum, this patch introduces 'virNodeDevCapMdev' structure which will store everything libvirt can gather from sysfs about a mediated device. Since we need to report the info for both the mediated child device it's parent mdev capabilities (merely the mdev types' attributes), this structure serves in both of these cases with the difference being that the amount of data it holds depends on the specific scenario (child vs parent).
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- include/libvirt/libvirt-nodedev.h | 1 + src/conf/node_device_conf.c | 19 ++++++++++++++++++- src/conf/node_device_conf.h | 19 ++++++++++++++++++- src/conf/virnodedeviceobj.c | 3 ++- src/libvirt-nodedev.c | 1 + src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 1 + src/node_device/node_device_udev.c | 8 +++++++- tools/virsh-nodedev.c | 2 ++ 9 files changed, 51 insertions(+), 4 deletions(-)
diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 85003903d7..59edf4db02 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -79,6 +79,7 @@ typedef enum { VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPORTS = 1 << 10, /* Capable of vport */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_GENERIC = 1 << 11, /* Capable of scsi_generic */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM = 1 << 12, /* DRM device */ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV = 1 << 13, /* Mediated device */ } virConnectListAllNodeDeviceFlags;
int virConnectListAllNodeDevices (virConnectPtr conn, diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 72fb9a5611..03d7993ab1 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -60,7 +60,8 @@ VIR_ENUM_IMPL(virNodeDevCap, VIR_NODE_DEV_CAP_LAST, "fc_host", "vports", "scsi_generic", - "drm") + "drm", + "mdev")
VIR_ENUM_IMPL(virNodeDevNetCap, VIR_NODE_DEV_CAP_NET_LAST, "80203", @@ -542,6 +543,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def) break; case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_LAST: break; } @@ -1591,6 +1593,7 @@ virNodeDevCapsDefParseXML(xmlXPathContextPtr ctxt, case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown capability type '%d' for '%s'"), @@ -1905,6 +1908,7 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) case VIR_NODE_DEV_CAP_SCSI_GENERIC: VIR_FREE(data->sg.path); break; + case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_DRM: case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: @@ -1917,6 +1921,19 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) }
+void virNodeDevCapMdevFree(virNodeDevCapMdevPtr mdev) +{ + if (!mdev) + return; + + VIR_FREE(mdev->type); + VIR_FREE(mdev->name); + VIR_FREE(mdev->description); + VIR_FREE(mdev->device_api); + VIR_FREE(mdev); +} + + /* virNodeDeviceGetParentName * @conn: Connection pointer * @nodedev_name: Node device to lookup diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index a5d5cdd2a5..b7db35dd50 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -64,6 +64,7 @@ typedef enum { VIR_NODE_DEV_CAP_VPORTS, /* HBA which is capable of vports */ VIR_NODE_DEV_CAP_SCSI_GENERIC, /* SCSI generic device */ VIR_NODE_DEV_CAP_DRM, /* DRM device */ + VIR_NODE_DEV_CAP_MDEV, /* Mediated device */
VIR_NODE_DEV_CAP_LAST } virNodeDevCapType; @@ -131,6 +132,17 @@ struct _virNodeDevCapSystem { virNodeDevCapSystemFirmware firmware; };
+typedef struct _virNodeDevCapMdev virNodeDevCapMdev; +typedef virNodeDevCapMdev *virNodeDevCapMdevPtr; +struct _virNodeDevCapMdev { + char *type; + char *name; + char *description; + char *device_api; + unsigned int available_instances; + unsigned int iommuGroupNumber; +}; +
Introducing this structure can be moved into the next patch, it's not used here.
typedef struct _virNodeDevCapPCIDev virNodeDevCapPCIDev; typedef virNodeDevCapPCIDev *virNodeDevCapPCIDevPtr; struct _virNodeDevCapPCIDev { @@ -262,6 +274,7 @@ struct _virNodeDevCapData { virNodeDevCapStorage storage; virNodeDevCapSCSIGeneric sg; virNodeDevCapDRM drm; + virNodeDevCapMdev mdev; }; };
@@ -338,6 +351,9 @@ virNodeDeviceDefFree(virNodeDeviceDefPtr def); void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps);
+void +virNodeDevCapMdevFree(virNodeDevCapMdevPtr mdev); + # define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP \ (VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV | \ @@ -351,7 +367,8 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps); VIR_CONNECT_LIST_NODE_DEVICES_CAP_FC_HOST | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPORTS | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_GENERIC | \ - VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM) + VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM | \ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV)
char * virNodeDeviceGetParentName(virConnectPtr conn, diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 3fe3ae5fe8..1dfb7380c7 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -494,7 +494,8 @@ virNodeDeviceMatch(virNodeDeviceObjPtr devobj, MATCH(FC_HOST) || MATCH(VPORTS) || MATCH(SCSI_GENERIC) || - MATCH(DRM))) + MATCH(DRM) || + MATCH(MDEV))) return false; }
diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index 83376b0d96..f9d8edf7e5 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -98,6 +98,7 @@ virNodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags) * VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPORTS * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_GENERIC * VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM + * VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV * * Returns the number of node devices found or -1 and sets @devices to NULL in * case of error. On success, the array stored into @devices is guaranteed to diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b551cb86a6..2a3e8cb705 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -690,6 +690,7 @@ virNetDevIPRouteParseXML;
# conf/node_device_conf.h +virNodeDevCapMdevFree; virNodeDevCapsDefFree; virNodeDevCapTypeFromString; virNodeDevCapTypeToString; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 9b1c5bb5e9..bcf204c7b7 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -83,6 +83,7 @@ update_caps(virNodeDeviceObjPtr dev) case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_LAST: break; } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 36ed92f712..dbbd8e5d06 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -43,6 +43,7 @@ #include "virpci.h" #include "virstring.h" #include "virnetdev.h" +#include "virmdev.h"
#define VIR_FROM_THIS VIR_FROM_NODEDEV
@@ -1051,12 +1052,16 @@ udevGetDeviceType(struct udev_device *device, if (udevHasDeviceProperty(device, "INTERFACE")) *type = VIR_NODE_DEV_CAP_NET;
- /* SCSI generic device doesn't set DEVTYPE property */ + /* Neither SCSI generic devices nor mediated devices set DEVTYPE + * property, but they so we need to rely on the SUBSYSTEM property */
s/by they so/therefore/
if (udevGetStringProperty(device, "SUBSYSTEM", &subsystem) < 0) return -1;
if (STREQ_NULLABLE(subsystem, "scsi_generic")) *type = VIR_NODE_DEV_CAP_SCSI_GENERIC; + else if (STREQ_NULLABLE(subsystem, "mdev")) + *type = VIR_NODE_DEV_CAP_MDEV; + VIR_FREE(subsystem); }
@@ -1096,6 +1101,7 @@ udevGetDeviceDetails(struct udev_device *device, return udevProcessSCSIGeneric(device, def); case VIR_NODE_DEV_CAP_DRM: return udevProcessDRMDevice(device, def); + case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_SYSTEM: case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index c691440219..19dcfafa6e 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -454,6 +454,8 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) case VIR_NODE_DEV_CAP_DRM: flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM; break; + case VIR_NODE_DEV_CAP_MDEV: + flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV; case VIR_NODE_DEV_CAP_LAST: break; } -- 2.12.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

+void virNodeDevCapMdevFree(virNodeDevCapMdevPtr mdev) +{ + if (!mdev) + return; + + VIR_FREE(mdev->type); + VIR_FREE(mdev->name); + VIR_FREE(mdev->description); + VIR_FREE(mdev->device_api); + VIR_FREE(mdev); +} +
[...]
+ +typedef struct _virNodeDevCapMdev virNodeDevCapMdev; +typedef virNodeDevCapMdev *virNodeDevCapMdevPtr; +struct _virNodeDevCapMdev { + char *type; + char *name; + char *description; + char *device_api; + unsigned int available_instances; + unsigned int iommuGroupNumber; +}; +
Introducing this structure can be moved into the next patch, it's not used here.
It actually is, virNodeDevCapMdevFree uses is, and it also used in the snippet below. Anyhow, I usually try to introduce concepts, data types and other symbols first to make it a tiny bit easier for the reviewer, since this can be easily missed when actually implementing function bodies, or logic in general. I have no problem with moving everything related to the structure to the following patch. Although in that case, if this patch would still be worth having as separate or more-or-less merge it with the next one completely. Let me know, what you find as the most plausible solution for you. Erik
typedef struct _virNodeDevCapPCIDev virNodeDevCapPCIDev; typedef virNodeDevCapPCIDev *virNodeDevCapPCIDevPtr; struct _virNodeDevCapPCIDev { @@ -262,6 +274,7 @@ struct _virNodeDevCapData { virNodeDevCapStorage storage; virNodeDevCapSCSIGeneric sg; virNodeDevCapDRM drm; + virNodeDevCapMdev mdev; }; };

On Mon, Apr 10, 2017 at 03:35:10PM +0200, Erik Skultety wrote:
+void virNodeDevCapMdevFree(virNodeDevCapMdevPtr mdev) +{ + if (!mdev) + return; + + VIR_FREE(mdev->type); + VIR_FREE(mdev->name); + VIR_FREE(mdev->description); + VIR_FREE(mdev->device_api); + VIR_FREE(mdev); +} +
[...]
+ +typedef struct _virNodeDevCapMdev virNodeDevCapMdev; +typedef virNodeDevCapMdev *virNodeDevCapMdevPtr; +struct _virNodeDevCapMdev { + char *type; + char *name; + char *description; + char *device_api; + unsigned int available_instances; + unsigned int iommuGroupNumber; +}; +
Introducing this structure can be moved into the next patch, it's not used here.
It actually is, virNodeDevCapMdevFree uses is, and it also used in the snippet below. Anyhow, I usually try to introduce concepts, data types and other
The free function itself isn't used anywhere in this patch and also the variable in struct is not used, that was my point.
symbols first to make it a tiny bit easier for the reviewer, since this can be easily missed when actually implementing function bodies, or logic in
I think that data structures should be grouped with the logic itself, they usually have no meaning without each other. This patch introduces the mdev capability which makes sense to keep it in a separate patch, but the capability itself doesn't require the structure, it would be better to place it into the next patch that actually implements the capability. Pavel
general. I have no problem with moving everything related to the structure to the following patch. Although in that case, if this patch would still be worth having as separate or more-or-less merge it with the next one completely. Let me know, what you find as the most plausible solution for you.
Erik
typedef struct _virNodeDevCapPCIDev virNodeDevCapPCIDev; typedef virNodeDevCapPCIDev *virNodeDevCapPCIDevPtr; struct _virNodeDevCapPCIDev { @@ -262,6 +274,7 @@ struct _virNodeDevCapData { virNodeDevCapStorage storage; virNodeDevCapSCSIGeneric sg; virNodeDevCapDRM drm; + virNodeDevCapMdev mdev; }; };

The parent device needs to report the generic stuff about the supported mediated devices types, like device API, available instances, and description which might hold some useful data about supported resolutions for the type given, framebuffer size, etc. Unfortunately, these are not standardized yet to be considered for separate elements. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/node_device_conf.h | 2 + src/node_device/node_device_udev.c | 115 +++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+) diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index b7db35dd50..d521dd9a4c 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -166,6 +166,8 @@ struct _virNodeDevCapPCIDev { int numa_node; virPCIEDeviceInfoPtr pci_express; int hdrType; /* enum virPCIHeaderType or -1 */ + virNodeDevCapMdevPtr *mdevs; + size_t nmdevs; }; typedef struct _virNodeDevCapUSBDev virNodeDevCapUSBDev; diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index dbbd8e5d06..be031fa5a8 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -324,6 +324,115 @@ udevTranslatePCIIds(unsigned int vendor, return 0; } +#define MDEV_GET_SYSFS_ATTR(attr, cb, ...) \ + do { \ + if (virAsprintf(&attrpath, "%s/%s", relpath, #attr) < 0) \ + goto cleanup; \ + \ + if (cb(device, attrpath, __VA_ARGS__) < 0) \ + goto cleanup; \ + } while (0) \ + + +static int +udevGetMdevCaps(struct udev_device *device, + const char *sysfspath, + virNodeDevCapMdevPtr mdev) +{ + int ret = -1; + char *attrpath = NULL; /* relative path to the actual sysfs attribute */ + const char *devpath = NULL; /* base sysfs path as reported by udev */ + const char *relpath = NULL; /* diff between @sysfspath and @devpath */ + char *tmp = NULL; + + + /* UDEV doesn't report attributes under subdirectories but can query them + * if specified as relative paths to the device's base path, + * e.g. /sys/devices/../0000:00:01.0/ is the device's base path as udev + * reports it, but we're interested in attributes under + * /sys/devices/../0000:00:01.0/mdev_supported_types/<type>/. So, let's + * strip the common part of the path and let udev chew the relative bit. + */ + devpath = udev_device_get_syspath(device); + relpath = sysfspath + strlen(devpath); + + /* When calling from the mdev child device, @sysfspath is a symbolic link + * to the actual mdev type (rather than a physical path), so we need to + * resolve it in order to get the type's name. + */ + if (virFileResolveLink(sysfspath, &tmp) < 0) + goto cleanup; + + if (VIR_STRDUP(mdev->type, last_component(tmp)) < 0) + goto cleanup; + + MDEV_GET_SYSFS_ATTR(name, udevGetStringSysfsAttr, + &mdev->name); + MDEV_GET_SYSFS_ATTR(description, udevGetStringSysfsAttr, + &mdev->description); + MDEV_GET_SYSFS_ATTR(device_api, udevGetStringSysfsAttr, + &mdev->device_api); + MDEV_GET_SYSFS_ATTR(available_instances, udevGetUintSysfsAttr, + &mdev->available_instances, 10); + + ret = 0; + cleanup: + VIR_FREE(attrpath); + VIR_FREE(tmp); + return ret; +} +#undef MDEV_GET_SYSFS_ATTR + + +static int +udevPCIGetMdevCaps(struct udev_device *device, + virNodeDevCapPCIDevPtr pcidata) +{ + int ret = -1; + DIR *dir = NULL; + struct dirent *entry; + char *path = NULL; + char *tmppath = NULL; + virNodeDevCapMdevPtr mdev = NULL; + + if (virAsprintf(&path, "%s/mdev_supported_types", + udev_device_get_syspath(device)) < 0) + return -1; + + if ((ret = virDirOpenIfExists(&dir, path)) <= 0) + goto cleanup; + + if (VIR_ALLOC(pcidata->mdevs) < 0) + goto cleanup; + + /* since udev doesn't provide means to list other than top-level + * attributes, we need to scan the subdirectories ourselves + */ + while ((ret = virDirRead(dir, &entry, path)) > 0) { + if (VIR_ALLOC(mdev) < 0) + goto cleanup; + + if (virAsprintf(&tmppath, "%s/%s", path, entry->d_name) < 0) + goto cleanup; + + if (udevGetMdevCaps(device, tmppath, mdev) < 0) + goto cleanup; + + if (VIR_APPEND_ELEMENT(pcidata->mdevs, pcidata->nmdevs, mdev) < 0) + goto cleanup; + + VIR_FREE(tmppath); + } + + ret = 0; + cleanup: + VIR_DIR_CLOSE(dir); + VIR_FREE(path); + VIR_FREE(tmppath); + virNodeDevCapMdevFree(mdev); + return ret; +} + static int udevProcessPCI(struct udev_device *device, @@ -412,6 +521,12 @@ udevProcessPCI(struct udev_device *device, } } + /* check whether the device is mediated devices framework capable, if so, + * process it + */ + if (udevPCIGetMdevCaps(device, pci_dev) < 0) + goto cleanup; + ret = 0; cleanup: -- 2.12.2

On Wed, Mar 29, 2017 at 02:51:17PM +0200, Erik Skultety wrote:
The parent device needs to report the generic stuff about the supported mediated devices types, like device API, available instances, and description which might hold some useful data about supported resolutions for the type given, framebuffer size, etc. Unfortunately, these are not standardized yet to be considered for separate elements.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/node_device_conf.h | 2 + src/node_device/node_device_udev.c | 115 +++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+)
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index b7db35dd50..d521dd9a4c 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -166,6 +166,8 @@ struct _virNodeDevCapPCIDev { int numa_node; virPCIEDeviceInfoPtr pci_express; int hdrType; /* enum virPCIHeaderType or -1 */ + virNodeDevCapMdevPtr *mdevs; + size_t nmdevs; };
typedef struct _virNodeDevCapUSBDev virNodeDevCapUSBDev; diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index dbbd8e5d06..be031fa5a8 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -324,6 +324,115 @@ udevTranslatePCIIds(unsigned int vendor, return 0; }
+#define MDEV_GET_SYSFS_ATTR(attr, cb, ...) \ + do { \ + if (virAsprintf(&attrpath, "%s/%s", relpath, #attr) < 0) \ + goto cleanup; \ + \ + if (cb(device, attrpath, __VA_ARGS__) < 0) \ + goto cleanup; \
You need to free *attrpath*, otherwise every next call of this macro will store new string into that variable and the previous one is leaked.
+ } while (0) \
I would move the macro into the function in order to make it clear that the macro supposed to be used only inside that function because it uses variables available only inside the function and jumps to *cleanup* label. Usually you define this type of macro, use it and undefine it.
+ + +static int +udevGetMdevCaps(struct udev_device *device, + const char *sysfspath, + virNodeDevCapMdevPtr mdev) +{ + int ret = -1; + char *attrpath = NULL; /* relative path to the actual sysfs attribute */ + const char *devpath = NULL; /* base sysfs path as reported by udev */ + const char *relpath = NULL; /* diff between @sysfspath and @devpath */ + char *tmp = NULL; + + + /* UDEV doesn't report attributes under subdirectories but can query them + * if specified as relative paths to the device's base path, + * e.g. /sys/devices/../0000:00:01.0/ is the device's base path as udev + * reports it, but we're interested in attributes under + * /sys/devices/../0000:00:01.0/mdev_supported_types/<type>/. So, let's + * strip the common part of the path and let udev chew the relative bit. + */ + devpath = udev_device_get_syspath(device); + relpath = sysfspath + strlen(devpath); + + /* When calling from the mdev child device, @sysfspath is a symbolic link + * to the actual mdev type (rather than a physical path), so we need to + * resolve it in order to get the type's name. + */ + if (virFileResolveLink(sysfspath, &tmp) < 0) + goto cleanup; + + if (VIR_STRDUP(mdev->type, last_component(tmp)) < 0) + goto cleanup; + + MDEV_GET_SYSFS_ATTR(name, udevGetStringSysfsAttr, + &mdev->name); + MDEV_GET_SYSFS_ATTR(description, udevGetStringSysfsAttr, + &mdev->description); + MDEV_GET_SYSFS_ATTR(device_api, udevGetStringSysfsAttr, + &mdev->device_api); + MDEV_GET_SYSFS_ATTR(available_instances, udevGetUintSysfsAttr, + &mdev->available_instances, 10); + + ret = 0; + cleanup: + VIR_FREE(attrpath); + VIR_FREE(tmp); + return ret; +} +#undef MDEV_GET_SYSFS_ATTR + + +static int +udevPCIGetMdevCaps(struct udev_device *device, + virNodeDevCapPCIDevPtr pcidata) +{ + int ret = -1; + DIR *dir = NULL; + struct dirent *entry; + char *path = NULL; + char *tmppath = NULL; + virNodeDevCapMdevPtr mdev = NULL; + + if (virAsprintf(&path, "%s/mdev_supported_types", + udev_device_get_syspath(device)) < 0) + return -1; + + if ((ret = virDirOpenIfExists(&dir, path)) <= 0) + goto cleanup;
This is usually a source of a lot of bugs. I would rather have another variable called *rc*: if ((rc = virDirOpenIfExists(&dir, path)) < 0) goto cleanup; if (rc == 0) { ret = 0; goto cleanup; } The reason for doing this is to make it clear what's happening, ...
+ + if (VIR_ALLOC(pcidata->mdevs) < 0) + goto cleanup;
This should be freed in virNodeDevCapsDefFree including all the mdev caps inside that array if there are any.
+ /* since udev doesn't provide means to list other than top-level + * attributes, we need to scan the subdirectories ourselves + */ + while ((ret = virDirRead(dir, &entry, path)) > 0) { + if (VIR_ALLOC(mdev) < 0) + goto cleanup;
because here for example if the allocation fails the *ret* contains *1* and you jump directly to *cleanup* and that would result into returning *1* instead of *-1*. So there you need to do: while ((rc = virDirRead(dir, &entry, path)) > 0) { ... } if (rc == 0) ret = 0;
+ if (virAsprintf(&tmppath, "%s/%s", path, entry->d_name) < 0) + goto cleanup; + + if (udevGetMdevCaps(device, tmppath, mdev) < 0) + goto cleanup; + + if (VIR_APPEND_ELEMENT(pcidata->mdevs, pcidata->nmdevs, mdev) < 0) + goto cleanup; + + VIR_FREE(tmppath); + } + + ret = 0; + cleanup: + VIR_DIR_CLOSE(dir); + VIR_FREE(path); + VIR_FREE(tmppath); + virNodeDevCapMdevFree(mdev); + return ret; +} +
static int udevProcessPCI(struct udev_device *device, @@ -412,6 +521,12 @@ udevProcessPCI(struct udev_device *device, } }
+ /* check whether the device is mediated devices framework capable, if so, + * process it + */ + if (udevPCIGetMdevCaps(device, pci_dev) < 0) + goto cleanup; + ret = 0;
cleanup: -- 2.12.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Start discovering the mediated devices on the host system and format the attributes for the child device into the XML. Compared to the parent device which reports generic information about the abstract mediated devices types, a child device only reports the type name it has been instantiated from and the iommu group number, since that's device specific compared to the rest of the info that can be gathered about mdevs at the moment. The resulting mdev child device XML: <device> <name>mdev_4b20d080_1b54_4048_85b3_a6a62d165c01</name> <path>/sys/devices/.../4b20d080-1b54-4048-85b3-a6a62d165c01</path> <parent>pci_0000_06_00_0</parent> <driver> <name>vfio_mdev</name> </driver> <capability type='mdev'> <type id='vendor-supplied-type-id'/> <iommuGroup number='NUM'/> <capability/> <device/> Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/node_device_conf.c | 6 ++- src/node_device/node_device_udev.c | 87 +++++++++++++++++++++++++++++--------- 2 files changed, 71 insertions(+), 22 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 03d7993ab1..1f13484d9b 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -541,9 +541,13 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def) case VIR_NODE_DEV_CAP_DRM: virBufferEscapeString(&buf, "<type>%s</type>\n", virNodeDevDRMTypeToString(data->drm.type)); break; + case VIR_NODE_DEV_CAP_MDEV: + virBufferEscapeString(&buf, "<type id='%s'/>\n", data->mdev.type); + virBufferAsprintf(&buf, "<iommuGroup number='%u'/>\n", + data->mdev.iommuGroupNumber); + break; case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: - case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_LAST: break; } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index be031fa5a8..092d9456b2 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -324,9 +324,9 @@ udevTranslatePCIIds(unsigned int vendor, return 0; } -#define MDEV_GET_SYSFS_ATTR(attr, cb, ...) \ +#define MDEV_GET_SYSFS_ATTR(attr_name, dir, cb, ...) \ do { \ - if (virAsprintf(&attrpath, "%s/%s", relpath, #attr) < 0) \ + if (virAsprintf(&attrpath, "%s/%s", dir, #attr_name) < 0) \ goto cleanup; \ \ if (cb(device, attrpath, __VA_ARGS__) < 0) \ @@ -345,17 +345,6 @@ udevGetMdevCaps(struct udev_device *device, const char *relpath = NULL; /* diff between @sysfspath and @devpath */ char *tmp = NULL; - - /* UDEV doesn't report attributes under subdirectories but can query them - * if specified as relative paths to the device's base path, - * e.g. /sys/devices/../0000:00:01.0/ is the device's base path as udev - * reports it, but we're interested in attributes under - * /sys/devices/../0000:00:01.0/mdev_supported_types/<type>/. So, let's - * strip the common part of the path and let udev chew the relative bit. - */ - devpath = udev_device_get_syspath(device); - relpath = sysfspath + strlen(devpath); - /* When calling from the mdev child device, @sysfspath is a symbolic link * to the actual mdev type (rather than a physical path), so we need to * resolve it in order to get the type's name. @@ -366,14 +355,24 @@ udevGetMdevCaps(struct udev_device *device, if (VIR_STRDUP(mdev->type, last_component(tmp)) < 0) goto cleanup; - MDEV_GET_SYSFS_ATTR(name, udevGetStringSysfsAttr, - &mdev->name); - MDEV_GET_SYSFS_ATTR(description, udevGetStringSysfsAttr, - &mdev->description); - MDEV_GET_SYSFS_ATTR(device_api, udevGetStringSysfsAttr, - &mdev->device_api); - MDEV_GET_SYSFS_ATTR(available_instances, udevGetUintSysfsAttr, - &mdev->available_instances, 10); + /* UDEV doesn't report attributes under subdirectories by default but is + * able to query them if the path to the attribute is relative paths to the + * device's base path, e.g. /sys/devices/../0000:00:01.0/ is the device's + * base path as udev reports it, but we're interested in attributes under + * /sys/devices/../0000:00:01.0/mdev_supported_types/<type>/. So, let's + * strip the common part of the path and let udev chew the relative bit. + */ + devpath = udev_device_get_syspath(device); + relpath = sysfspath + strlen(devpath); + + MDEV_GET_SYSFS_ATTR(name, relpath, + udevGetStringSysfsAttr, &mdev->name); + MDEV_GET_SYSFS_ATTR(description, relpath, + udevGetStringSysfsAttr, &mdev->description); + MDEV_GET_SYSFS_ATTR(device_api, relpath, + udevGetStringSysfsAttr, &mdev->device_api); + MDEV_GET_SYSFS_ATTR(available_instances, relpath, + udevGetUintSysfsAttr, &mdev->available_instances, 10); ret = 0; cleanup: @@ -1099,6 +1098,51 @@ udevProcessSCSIGeneric(struct udev_device *dev, static int +udevProcessMediatedDevice(struct udev_device *dev, + virNodeDeviceDefPtr def) +{ + int ret = -1; + const char *uuidstr = NULL; + int iommugrp = -1; + int model = -1; + char *path = NULL; + virMediatedDevicePtr mdev = NULL; + virNodeDevCapMdevPtr data = &def->caps->data.mdev; + + if (virAsprintf(&path, "%s/mdev_type", udev_device_get_syspath(dev)) < 0) + goto cleanup; + + if (udevGetMdevCaps(dev, path, data) < 0) + goto cleanup; + + if ((model = virMediatedDeviceModelTypeFromString(data->device_api)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Device API '%s' not supported yet"), + data->device_api); + goto cleanup; + } + + uuidstr = udev_device_get_sysname(dev); + if (!(mdev = virMediatedDeviceNew(uuidstr, model))) + goto cleanup; + + if ((iommugrp = virMediatedDeviceGetIOMMUGroupNum(mdev)) < 0) + goto cleanup; + + if (udevGenerateDeviceName(dev, def, NULL) != 0) + goto cleanup; + + data->iommuGroupNumber = iommugrp; + + ret = 0; + cleanup: + VIR_FREE(path); + virMediatedDeviceFree(mdev); + return ret; + +} + +static int udevGetDeviceNodes(struct udev_device *device, virNodeDeviceDefPtr def) { @@ -1217,6 +1261,7 @@ udevGetDeviceDetails(struct udev_device *device, case VIR_NODE_DEV_CAP_DRM: return udevProcessDRMDevice(device, def); case VIR_NODE_DEV_CAP_MDEV: + return udevProcessMediatedDevice(device, def); case VIR_NODE_DEV_CAP_SYSTEM: case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: -- 2.12.2

On Wed, Mar 29, 2017 at 02:51:18PM +0200, Erik Skultety wrote:
Start discovering the mediated devices on the host system and format the attributes for the child device into the XML. Compared to the parent device which reports generic information about the abstract mediated devices types, a child device only reports the type name it has been instantiated from and the iommu group number, since that's device specific compared to the rest of the info that can be gathered about mdevs at the moment.
The resulting mdev child device XML: <device> <name>mdev_4b20d080_1b54_4048_85b3_a6a62d165c01</name> <path>/sys/devices/.../4b20d080-1b54-4048-85b3-a6a62d165c01</path> <parent>pci_0000_06_00_0</parent> <driver> <name>vfio_mdev</name> </driver> <capability type='mdev'> <type id='vendor-supplied-type-id'/> <iommuGroup number='NUM'/> <capability/> <device/>
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/node_device_conf.c | 6 ++- src/node_device/node_device_udev.c | 87 +++++++++++++++++++++++++++++--------- 2 files changed, 71 insertions(+), 22 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 03d7993ab1..1f13484d9b 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -541,9 +541,13 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def) case VIR_NODE_DEV_CAP_DRM: virBufferEscapeString(&buf, "<type>%s</type>\n", virNodeDevDRMTypeToString(data->drm.type)); break; + case VIR_NODE_DEV_CAP_MDEV: + virBufferEscapeString(&buf, "<type id='%s'/>\n", data->mdev.type); + virBufferAsprintf(&buf, "<iommuGroup number='%u'/>\n", + data->mdev.iommuGroupNumber); + break;
You should update nodedev.rng schema to reflect changes made by this patch and I've just noticed that the whole series misses code for parsing the new mdev device capabilities and some tests in nodedevxml2xml tests would be nice.
case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: - case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_LAST: break; } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index be031fa5a8..092d9456b2 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -324,9 +324,9 @@ udevTranslatePCIIds(unsigned int vendor, return 0; }
-#define MDEV_GET_SYSFS_ATTR(attr, cb, ...) \ +#define MDEV_GET_SYSFS_ATTR(attr_name, dir, cb, ...) \ do { \ - if (virAsprintf(&attrpath, "%s/%s", relpath, #attr) < 0) \ + if (virAsprintf(&attrpath, "%s/%s", dir, #attr_name) < 0) \ goto cleanup; \ \ if (cb(device, attrpath, __VA_ARGS__) < 0) \ @@ -345,17 +345,6 @@ udevGetMdevCaps(struct udev_device *device, const char *relpath = NULL; /* diff between @sysfspath and @devpath */ char *tmp = NULL;
- - /* UDEV doesn't report attributes under subdirectories but can query them - * if specified as relative paths to the device's base path, - * e.g. /sys/devices/../0000:00:01.0/ is the device's base path as udev - * reports it, but we're interested in attributes under - * /sys/devices/../0000:00:01.0/mdev_supported_types/<type>/. So, let's - * strip the common part of the path and let udev chew the relative bit. - */ - devpath = udev_device_get_syspath(device); - relpath = sysfspath + strlen(devpath); - /* When calling from the mdev child device, @sysfspath is a symbolic link * to the actual mdev type (rather than a physical path), so we need to * resolve it in order to get the type's name. @@ -366,14 +355,24 @@ udevGetMdevCaps(struct udev_device *device, if (VIR_STRDUP(mdev->type, last_component(tmp)) < 0) goto cleanup;
- MDEV_GET_SYSFS_ATTR(name, udevGetStringSysfsAttr, - &mdev->name); - MDEV_GET_SYSFS_ATTR(description, udevGetStringSysfsAttr, - &mdev->description); - MDEV_GET_SYSFS_ATTR(device_api, udevGetStringSysfsAttr, - &mdev->device_api); - MDEV_GET_SYSFS_ATTR(available_instances, udevGetUintSysfsAttr, - &mdev->available_instances, 10); + /* UDEV doesn't report attributes under subdirectories by default but is + * able to query them if the path to the attribute is relative paths to the + * device's base path, e.g. /sys/devices/../0000:00:01.0/ is the device's + * base path as udev reports it, but we're interested in attributes under + * /sys/devices/../0000:00:01.0/mdev_supported_types/<type>/. So, let's + * strip the common part of the path and let udev chew the relative bit. + */ + devpath = udev_device_get_syspath(device); + relpath = sysfspath + strlen(devpath); + + MDEV_GET_SYSFS_ATTR(name, relpath, + udevGetStringSysfsAttr, &mdev->name); + MDEV_GET_SYSFS_ATTR(description, relpath, + udevGetStringSysfsAttr, &mdev->description); + MDEV_GET_SYSFS_ATTR(device_api, relpath, + udevGetStringSysfsAttr, &mdev->device_api); + MDEV_GET_SYSFS_ATTR(available_instances, relpath, + udevGetUintSysfsAttr, &mdev->available_instances, 10);
These 3 hunks should be probably part of the previous patch.
ret = 0; cleanup: @@ -1099,6 +1098,51 @@ udevProcessSCSIGeneric(struct udev_device *dev,
static int +udevProcessMediatedDevice(struct udev_device *dev, + virNodeDeviceDefPtr def) +{ + int ret = -1; + const char *uuidstr = NULL; + int iommugrp = -1; + int model = -1; + char *path = NULL; + virMediatedDevicePtr mdev = NULL; + virNodeDevCapMdevPtr data = &def->caps->data.mdev; + + if (virAsprintf(&path, "%s/mdev_type", udev_device_get_syspath(dev)) < 0) + goto cleanup; + + if (udevGetMdevCaps(dev, path, data) < 0) + goto cleanup; + + if ((model = virMediatedDeviceModelTypeFromString(data->device_api)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Device API '%s' not supported yet"), + data->device_api); + goto cleanup; + } + + uuidstr = udev_device_get_sysname(dev); + if (!(mdev = virMediatedDeviceNew(uuidstr, model))) + goto cleanup; + + if ((iommugrp = virMediatedDeviceGetIOMMUGroupNum(mdev)) < 0) + goto cleanup; + + if (udevGenerateDeviceName(dev, def, NULL) != 0) + goto cleanup; + + data->iommuGroupNumber = iommugrp; + + ret = 0; + cleanup: + VIR_FREE(path); + virMediatedDeviceFree(mdev); + return ret; + +} + +static int udevGetDeviceNodes(struct udev_device *device, virNodeDeviceDefPtr def) { @@ -1217,6 +1261,7 @@ udevGetDeviceDetails(struct udev_device *device, case VIR_NODE_DEV_CAP_DRM: return udevProcessDRMDevice(device, def); case VIR_NODE_DEV_CAP_MDEV: + return udevProcessMediatedDevice(device, def); case VIR_NODE_DEV_CAP_SYSTEM: case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: -- 2.12.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

This introduces a new nested capability element of type 'mdev' with the resulting XML of the following format: <device> ... <capability type='pci'> ... <capability type='mdev'> <type id='vendor-supplied-id'> <description>optional, raw, unstructured resource allocation data </description> <device_api>vfio-pci</device_api> <available_instances>NUM</available_instances> </type> ... <type> ... </type> </capability> </capability> ... </device> Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/node_device_conf.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 1f13484d9b..99211de6f3 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -264,6 +264,30 @@ virNodeDeviceCapPCIDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<capability type='%s'/>\n", virPCIHeaderTypeToString(data->pci_dev.hdrType)); } + if (data->pci_dev.mdevs) { + virBufferAddLit(buf, "<capability type='mdev'>\n"); + virBufferAdjustIndent(buf, 2); + for (i = 0; i < data->pci_dev.nmdevs; i++) { + virNodeDevCapMdevPtr mdev = data->pci_dev.mdevs[i]; + virBufferEscapeString(buf, "<type id='%s'>\n", mdev->type); + virBufferAdjustIndent(buf, 2); + if (mdev->name) + virBufferAsprintf(buf, "<name>%s</name>\n", + mdev->name); + if (mdev->description) + virBufferAsprintf(buf, "<description>%s</description>\n", + mdev->description); + virBufferAsprintf(buf, "<device_api>%s</device_api>\n", + mdev->device_api); + virBufferAsprintf(buf, + "<available_instances>%u</available_instances>\n", + mdev->available_instances); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</type>\n"); + } + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</capability>\n"); + } if (data->pci_dev.nIommuGroupDevices) { virBufferAsprintf(buf, "<iommuGroup number='%d'>\n", data->pci_dev.iommuGroupNumber); -- 2.12.2

There's lot more to document about the nodedev driver, besides PCI and SR-IOV (even this might need to be extended), but let's start small-ish and at least have a page for it linked from the drivers.html. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/drivers.html.in | 6 +- docs/drvnodedev.html.in | 184 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 docs/drvnodedev.html.in diff --git a/docs/drivers.html.in b/docs/drivers.html.in index be7483b9bc..61993861ee 100644 --- a/docs/drivers.html.in +++ b/docs/drivers.html.in @@ -4,7 +4,11 @@ <body> <h1>Internal drivers</h1> - <ul id="toc"></ul> + <ul> + <li><a href="#hypervisor">Hypervisor drivers</a></li> + <li><a href="#storage">Storage drivers</a></li> + <li><a href="drvnodedev.html">Node device driver</a></li> + </ul> <p> The libvirt public API delegates its implementation to one or diff --git a/docs/drvnodedev.html.in b/docs/drvnodedev.html.in new file mode 100644 index 0000000000..ed185c3df3 --- /dev/null +++ b/docs/drvnodedev.html.in @@ -0,0 +1,184 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"> +<html xmlns="http://www.w3.org/1999/xhtml"> + <body> + <h1>Host device management</h1> + + <p> + Libvirt provides management of both physical and virtual host devices + (historically also referred to as node devices) like USB, PCI, SCSI, and + network devices. This also includes various virtualization capabilities + which the aforementioned devices provide for utilization, for example + SR-IOV, NPIV, MDEV, DRM, etc. <br/> + <br/> + The node device driver provides means to list and show details about host + devices (<code>virsh nodedev-list</code>, + <code>virsh nodedev-dumpxml</code>), which are generic and can be used + with all devices. It also provides means to create and destroy devices + (<code>virsh nodedev-create</code>, <code>virsh nodedev-destroy</code>) + which are meant to be used to create virtual devices, currently only + supported by NPIV + (<a href="http://wiki.libvirt.org/page/NPIV_in_libvirt">more info about NPIV)</a>). <br/> + <br/> + Devices on the host system are arranged in a tree-like hierarchy, with + the root node being called <code>computer</code>. The node device driver + supports two backends to manage the devices, HAL and udev, with the former + being deprecated in favour of the latter.<br/> + The generic format of a host device XML can be seen below. + To identify a device both within the host and the device tree hierarchy, + the following elements are used: + </p> + <dl> + <dt><code>name</code></dt> + <dd> + The device's name will be generated by libvirt using the subsystem, + like pci and the device's sysfs basename. + </dd> + <dt><code>path</code></dt> + <dd> + Fully qualified sysfs path to the device. + </dd> + <dt><code>parent</code></dt> + <dd> + This element identifies the parent node in the device hierarchy. The + value of the element will correspond with the device parent's + <code>name</code> element or <code>computer</code> if the device does + not have any parent. + </dd> + <dt><code>driver</code></dt> + <dd> + This elements reports the driver in use for this device. The presence + of this element in the output XML depends on whether the underlying + device manager (most likely udev) exposes information about the + driver. + </dd> + <dt><code>capability</code></dt> + <dd> + Describes the device in terms of feature support. The element has one + mandatory attribute <code>type</code> the value of which determines + the type of the device. Currently recognized values for the attribute + are: + <code>system</code>, + <code>pci</code>, + <code>usb</code>, + <code>usb_device</code>, + <code>net</code>, + <code>scsi</code>, + <code>scsi_host</code> (<span class="since">Since 0.4.7</span>), + <code>fc_host</code>, + <code>vports</code>, + <code>scsi_target</code> (<span class="since">Since 0.7.3</span>), + <code>storage</code> (<span class="since">Since 1.0.4</span>), + <code>scsi_generic</code> (<span class="since">Since 1.0.7</span>), + <code>drm</code> (<span class="since">Since 3.1.0</span>), and + <code>mdev</code> (<span class="since">Since 3.2.0</span>). + This element can be nested in which case it further specifies a + device's capability. Refer to specific device types to see more values + for the <code>type</code> attribute which are exclusive. + </dd> + </dl> + + <h2>Basic structure of a node device</h2> + <pre> +<device> + <name>pci_0000_00_17_0</name> + <path>/sys/devices/pci0000:00/0000:00:17.0</path> + <parent>computer</parent> + <driver> + <name>ahci</name> + </driver> + <capability type='pci'> +... + </capability> +</device></pre> + + <ul id="toc"/> + + <h2><a name="PCI">PCI host devices</a></h2> + <dl> + <dt><code>capability</code></dt> + <dd> + When used as top level element, the supported values for the + <code>type</code> attribute are <code>pci</code> and + <code>phys_function</code> (see <a href="#SRIOVCap">SR-IOV below</a>). + </dd> + </dl> + <pre> +<device> + <name>pci_0000_04_00_1</name> + <path>/sys/devices/pci0000:00/0000:00:06.0/0000:04:00.1</path> + <parent>pci_0000_00_06_0</parent> + <driver> + <name>igb</name> + </driver> + <capability type='pci'> + <domain>0</domain> + <bus>4</bus> + <slot>0</slot> + <function>1</function> + <product id='0x10c9'>82576 Gigabit Network Connection</product> + <vendor id='0x8086'>Intel Corporation</vendor> + <iommuGroup number='15'> + <address domain='0x0000' bus='0x04' slot='0x00' function='0x1'/> + </iommuGroup> + <numa node='0'/> + <pci-express> + <link validity='cap' port='1' speed='2.5' width='2'/> + <link validity='sta' speed='2.5' width='2'/> + </pci-express> + </capability> +</device></pre> + + <p> + The XML format for a PCI device stays the same for any further + capabilities it supports, a single nested <code><capability></code> + element will be included for each capability the device supports. + </p> + + <h3><a name="SRIOVCap">SR-IOV capability</a></h3> + <p> + Single root input/output virtualization (SR-IOV) allows sharing of the + PCIe resources by multiple virtual environments. That is achieved by + slicing up a single full-featured physical resource called physical + function (PF) into multiple devices called virtual functions (VFs) sharing + their configuration with the underlying PF. Despite the SR-IOV + specification, the amount of VFs that can be created on a PF varies among + manufacturers.<br/> + <br/> + Suppose the NIC <a href="#PCI">above</a> was also SR-IOV capable, it would + also include a nested + <code><capability></code> element enumerating all virtual + functions available on the physical device (physical port) like in the + example below. + </p> + + <pre> +<capability type='pci'> +... + <capability type='virt_functions' maxCount='7'> + <address domain='0x0000' bus='0x04' slot='0x10' function='0x1'/> + <address domain='0x0000' bus='0x04' slot='0x10' function='0x3'/> + <address domain='0x0000' bus='0x04' slot='0x10' function='0x5'/> + <address domain='0x0000' bus='0x04' slot='0x10' function='0x7'/> + <address domain='0x0000' bus='0x04' slot='0x11' function='0x1'/> + <address domain='0x0000' bus='0x04' slot='0x11' function='0x3'/> + <address domain='0x0000' bus='0x04' slot='0x11' function='0x5'/> + </capability> +... +</capability></pre> + <p> + A SR-IOV child device on the other hand, would then report its top level + capability type as a physical function instead: + </p> + + <pre> +<device> +... + <capability type='phys_function'> + <address domain='0x0000' bus='0x04' slot='0x00' function='0x0'/> + </capability> +... +<device></pre> + + </body> +</html> -- 2.12.2

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/drvnodedev.html.in | 91 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/docs/drvnodedev.html.in b/docs/drvnodedev.html.in index ed185c3df3..776c88cc87 100644 --- a/docs/drvnodedev.html.in +++ b/docs/drvnodedev.html.in @@ -180,5 +180,96 @@ ... <device></pre> + <h3><a name="MDEVCap">MDEV capability</a></h3> + <p> + A PCI device capable of creating mediated devices will include a nested + capability mdev which enumerates all the supported mdev types on the + physical device, along with the type attributes available through sysfs. + For a more info about mediated devices, refer to the + <a href="#MDEV">paragraph below</a>. + </p> + +<pre> +<device> +... + <driver> + <name>nvidia</name> + </driver> + <capability type='pci'> +... + <capability type='mdev'> + <type id='nvidia-11'> + <name>GRID M60-0B</name> + <description>num_heads=2, frl_config=45, framebuffer=512M, + max_resolution=2560x1600, max_instance=16</description> + <device_api>vfio-pci</device_api> + <available_instances>16</available_instances> + </type> + <!-- Here would come the rest of the available mdev types --> + </capability> +... + </capability> +</device></pre> + + <h2><a name="MDEV">Mediated devices (MDEVs)</a></h2> + <p> + Mediated devices (<span class="since">Since 3.2.0</span>) are software + devices defining resource allocation on the backing physical device which + in turn allows the parent physical device's resources to be divided into + several mediated devices, thus sharing the physical device's performance + among multiple guests. Unlike SR-IOV however, where a PCIe device appears + as multiple separate PCIe devices on the host's PCI bus, mediated devices + only appear on the mdev virtual bus. Therefore, no detach/reattach + procedure from/to the host driver procedure is involved even though + mediated devices are used in a direct device assignment manner.<br/> + <br/> + At the moment, libvirt can only list the available mediated devices on the + host and display all information libvirt can gather about them, see the + examples below. Because mediated devices are instantiated from vendor + specific templates, simply called 'types', information about the resource + allocation for a specific type is contained within the backing physical + parent device (see <a href="#PCI">PCI host devices</a> for an example). + </p> + + <h3>Example of a mediated device</h3> + <pre> +<device> + <name>mdev_4b20d080_1b54_4048_85b3_a6a62d165c01</name> + <path>/sys/devices/pci0000:00/0000:00:02.0/4b20d080-1b54-4048-85b3-a6a62d165c01</path> + <parent>pci_0000_06_00_0</parent> + <driver> + <name>vfio_mdev</name> + </driver> + <capability type='mdev'> + <type id='nvidia-11'/> + <iommuGroup number='12'/> + <capability/> +<device/></pre> + + <p> + To see the supported mediated device types on a specific physical device + use the following: + </p> + + <pre> +$ ls /sys/class/mdev_bus/<device>/mdev_supported_types</pre> + + <p> + To manually instantiate a mediated device, use one of the following as a + reference: + </p> + + <pre> +$ uuidgen > /sys/class/mdev_bus/<device>/mdev_supported_types/<type>/create +... +$ echo <UUID> > /sys/class/mdev_bus/<device>/mdev_supported_types/<type>/create</pre> + + <p> + Manual removal of a mediated device is then performed as follows: + </p> + + <pre> +$ echo 1 > /sys/bus/mdev/devices/<uuid>/remove</pre> + </body> </html> -- 2.12.2

On Wed, Mar 29, 2017 at 02:51:10PM +0200, Erik Skultety wrote:
This series enables the node device driver to report information about the existing mediated devices on the host. There is no device creation involved yet. The information reported by the node device driver is split into two parts, one that is reported within the physical parent's capabilities (the generic stuff that comes from the mdev types' sysfs attributes, note the 'description' attribute which is verbatim - raw,unstructured string) and the other that is reported within the mdev child device and merely contains the mdev type id, which the device was instantiated from, and the iommu group number.
Basically, the format of the XML I went for is as follows:
PCI parent: <device> <name>pci_0000_06_00_0</name> <path>/sys/devices/.../0000:06:00.0</path> <parent>pci_0000_05_08_0</parent> ... <capability type='pci'> ... <capability type='mdev'> <type id='nvidia-11'> <name>GRID M60-0B</name> <description>num_heads=2, frl_config=45, framebuffer=512M, max_resolution=2560x1600, max_instance=16</description>
This 'description' field is pretty horrific. We were quite clear that we were not going to expose arbitrary attributes in the XML without modelling them explicitly as XML elements. Using the description field in this way is just doing arbitrary attribute passthrough via the backdoor - it is clear that applications are doing to end up parsing this 'description' data and will them complain if we later change it. So, NACK to including a description element with this kind of content. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

On Tue, Apr 04, 2017 at 04:23:18PM +0100, Daniel P. Berrange wrote:
On Wed, Mar 29, 2017 at 02:51:10PM +0200, Erik Skultety wrote:
This series enables the node device driver to report information about the existing mediated devices on the host. There is no device creation involved yet. The information reported by the node device driver is split into two parts, one that is reported within the physical parent's capabilities (the generic stuff that comes from the mdev types' sysfs attributes, note the 'description' attribute which is verbatim - raw,unstructured string) and the other that is reported within the mdev child device and merely contains the mdev type id, which the device was instantiated from, and the iommu group number.
Basically, the format of the XML I went for is as follows:
PCI parent: <device> <name>pci_0000_06_00_0</name> <path>/sys/devices/.../0000:06:00.0</path> <parent>pci_0000_05_08_0</parent> ... <capability type='pci'> ... <capability type='mdev'> <type id='nvidia-11'> <name>GRID M60-0B</name> <description>num_heads=2, frl_config=45, framebuffer=512M, max_resolution=2560x1600, max_instance=16</description>
This 'description' field is pretty horrific.
We were quite clear that we were not going to expose arbitrary attributes in the XML without modelling them explicitly as XML elements. Using the description field in this way is just doing arbitrary attribute passthrough via the backdoor - it is clear that applications are doing to end up parsing this 'description' data and will them complain if we later change it.
I remember us stating that, but this is the case with NVIDIA (who at least reports some useful information in it) and Intel - what if other vendor comes and creates a valid, human readable description of a device without specifying any attributes like in the case above? Just because some vendors "abused" the attribute doesn't mean we should stop reporting it completely. IMHO the name "description" should mean that it's something raw, possibly unstructured, and thus the applications should treat it that way. Now, this is my bad and I need to revisit the last patch with documentation and add a paragraph for the <description> element as an optional element with raw data. Until all the attributes are properly unified/standardized under sysfs ABI and respected by vendors, I think we should report everything we're able to gather about a device, description included. If properly documented, nobody can complain about us breaking anything, since how do you guarantee format-less raw string anyway? After all, applications will end up parsing it anyway, be it from our XML or not. Erik
So, NACK to including a description element with this kind of content.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

On Wed, Apr 05, 2017 at 08:12:31AM +0200, Erik Skultety wrote:
On Tue, Apr 04, 2017 at 04:23:18PM +0100, Daniel P. Berrange wrote:
On Wed, Mar 29, 2017 at 02:51:10PM +0200, Erik Skultety wrote:
This series enables the node device driver to report information about the existing mediated devices on the host. There is no device creation involved yet. The information reported by the node device driver is split into two parts, one that is reported within the physical parent's capabilities (the generic stuff that comes from the mdev types' sysfs attributes, note the 'description' attribute which is verbatim - raw,unstructured string) and the other that is reported within the mdev child device and merely contains the mdev type id, which the device was instantiated from, and the iommu group number.
Basically, the format of the XML I went for is as follows:
PCI parent: <device> <name>pci_0000_06_00_0</name> <path>/sys/devices/.../0000:06:00.0</path> <parent>pci_0000_05_08_0</parent> ... <capability type='pci'> ... <capability type='mdev'> <type id='nvidia-11'> <name>GRID M60-0B</name> <description>num_heads=2, frl_config=45, framebuffer=512M, max_resolution=2560x1600, max_instance=16</description>
This 'description' field is pretty horrific.
We were quite clear that we were not going to expose arbitrary attributes in the XML without modelling them explicitly as XML elements. Using the description field in this way is just doing arbitrary attribute passthrough via the backdoor - it is clear that applications are doing to end up parsing this 'description' data and will them complain if we later change it.
I remember us stating that, but this is the case with NVIDIA (who at least reports some useful information in it) and Intel - what if other vendor comes and creates a valid, human readable description of a device without specifying any attributes like in the case above? Just because some vendors "abused" the attribute doesn't mean we should stop reporting it completely. IMHO the name "description" should mean that it's something raw, possibly unstructured, and thus the applications should treat it that way. Now, this is my bad and I need to revisit the last patch with documentation and add a paragraph for the <description> element as an optional element with raw data.
Until all the attributes are properly unified/standardized under sysfs ABI and respected by vendors, I think we should report everything we're able to gather about a device, description included. If properly documented, nobody can complain about us breaking anything, since how do you guarantee format-less raw string anyway? After all, applications will end up parsing it anyway, be it from our XML or not.
I understand your point, but I'm still not in favour of exposing this info because it is a clear cut attempt to do arbitrary attribute passthrough to libvirt. All the attributes show there can be determined by a simple lookup based on the name field "GRID M60-0B". So if apps want to know the number of heads, framebuffer size, etc, etc I think they should just maintain a lookup map based on name in their own code, until we explicitly model this stuff in the XML. Once we model the attributes in the XML, this description adds no useful info that we wouldn't already be reported, and before we model it in the XML, this is just clearly an abuse of our design statement that we are not doing generic attribute passthrough.
So, NACK to including a description element with this kind of content.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

On Wed, 5 Apr 2017 09:21:17 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Apr 05, 2017 at 08:12:31AM +0200, Erik Skultety wrote:
On Tue, Apr 04, 2017 at 04:23:18PM +0100, Daniel P. Berrange wrote:
On Wed, Mar 29, 2017 at 02:51:10PM +0200, Erik Skultety wrote:
This series enables the node device driver to report information about the existing mediated devices on the host. There is no device creation involved yet. The information reported by the node device driver is split into two parts, one that is reported within the physical parent's capabilities (the generic stuff that comes from the mdev types' sysfs attributes, note the 'description' attribute which is verbatim - raw,unstructured string) and the other that is reported within the mdev child device and merely contains the mdev type id, which the device was instantiated from, and the iommu group number.
Basically, the format of the XML I went for is as follows:
PCI parent: <device> <name>pci_0000_06_00_0</name> <path>/sys/devices/.../0000:06:00.0</path> <parent>pci_0000_05_08_0</parent> ... <capability type='pci'> ... <capability type='mdev'> <type id='nvidia-11'> <name>GRID M60-0B</name> <description>num_heads=2, frl_config=45, framebuffer=512M, max_resolution=2560x1600, max_instance=16</description>
This 'description' field is pretty horrific.
We were quite clear that we were not going to expose arbitrary attributes in the XML without modelling them explicitly as XML elements. Using the description field in this way is just doing arbitrary attribute passthrough via the backdoor - it is clear that applications are doing to end up parsing this 'description' data and will them complain if we later change it.
I remember us stating that, but this is the case with NVIDIA (who at least reports some useful information in it) and Intel - what if other vendor comes and creates a valid, human readable description of a device without specifying any attributes like in the case above? Just because some vendors "abused" the attribute doesn't mean we should stop reporting it completely. IMHO the name "description" should mean that it's something raw, possibly unstructured, and thus the applications should treat it that way. Now, this is my bad and I need to revisit the last patch with documentation and add a paragraph for the <description> element as an optional element with raw data.
Until all the attributes are properly unified/standardized under sysfs ABI and respected by vendors, I think we should report everything we're able to gather about a device, description included. If properly documented, nobody can complain about us breaking anything, since how do you guarantee format-less raw string anyway? After all, applications will end up parsing it anyway, be it from our XML or not.
I understand your point, but I'm still not in favour of exposing this info because it is a clear cut attempt to do arbitrary attribute passthrough to libvirt.
All the attributes show there can be determined by a simple lookup based on the name field "GRID M60-0B". So if apps want to know the number of heads, framebuffer size, etc, etc I think they should just maintain a lookup map based on name in their own code, until we explicitly model this stuff in the XML.
Once we model the attributes in the XML, this description adds no useful info that we wouldn't already be reported, and before we model it in the XML, this is just clearly an abuse of our design statement that we are not doing generic attribute passthrough.
I told Erik I'd jump in here, but I think I might agree with Dan. On the kernel side, the description attribute was an attempt to pull ourselves out of a rat hole of trying to figure out how to classify devices and then figure out what attributes for each class might be common across vendors. Clearly it'd be great to somehow know that a device is a GPU and has a resolution and graphics memory attribute, but getting there is hard. A free-form description field is sort of a catch-all where the vendor can provide a stop-gap and it's useful for human consumption. It would be inadvisable to parse it with machine code though. Including it in the XML sort of invites that possibility. A given mdev type is supposed to be stable. It should always point to a software equivalent device, including capabilities and resources. It should therefore be valid for upper level software to use the type to lookup from a human generated table the properties for that device. Of course there's about zero chance that a type will be perfectly stable, but hopefully it's a rare event and they stabilize pretty quickly. It'd be nice to one day revisit that classification and per class attributes problem in the kernel, but maybe we can let userspace figure out a common, useful set of attribute per class first. Thanks, Alex

On 12/04/17 16:19 -0600, Alex Williamson wrote:
On Wed, 5 Apr 2017 09:21:17 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Apr 05, 2017 at 08:12:31AM +0200, Erik Skultety wrote:
On Tue, Apr 04, 2017 at 04:23:18PM +0100, Daniel P. Berrange wrote:
On Wed, Mar 29, 2017 at 02:51:10PM +0200, Erik Skultety wrote:
This series enables the node device driver to report information about the existing mediated devices on the host. There is no device creation involved yet. The information reported by the node device driver is split into two parts, one that is reported within the physical parent's capabilities (the generic stuff that comes from the mdev types' sysfs attributes, note the 'description' attribute which is verbatim - raw,unstructured string) and the other that is reported within the mdev child device and merely contains the mdev type id, which the device was instantiated from, and the iommu group number.
Basically, the format of the XML I went for is as follows:
PCI parent: <device> <name>pci_0000_06_00_0</name> <path>/sys/devices/.../0000:06:00.0</path> <parent>pci_0000_05_08_0</parent> ... <capability type='pci'> ... <capability type='mdev'> <type id='nvidia-11'> <name>GRID M60-0B</name> <description>num_heads=2, frl_config=45, framebuffer=512M, max_resolution=2560x1600, max_instance=16</description>
This 'description' field is pretty horrific.
We were quite clear that we were not going to expose arbitrary attributes in the XML without modelling them explicitly as XML elements. Using the description field in this way is just doing arbitrary attribute passthrough via the backdoor - it is clear that applications are doing to end up parsing this 'description' data and will them complain if we later change it.
I remember us stating that, but this is the case with NVIDIA (who at least reports some useful information in it) and Intel - what if other vendor comes and creates a valid, human readable description of a device without specifying any attributes like in the case above? Just because some vendors "abused" the attribute doesn't mean we should stop reporting it completely. IMHO the name "description" should mean that it's something raw, possibly unstructured, and thus the applications should treat it that way. Now, this is my bad and I need to revisit the last patch with documentation and add a paragraph for the <description> element as an optional element with raw data.
Until all the attributes are properly unified/standardized under sysfs ABI and respected by vendors, I think we should report everything we're able to gather about a device, description included. If properly documented, nobody can complain about us breaking anything, since how do you guarantee format-less raw string anyway? After all, applications will end up parsing it anyway, be it from our XML or not.
I understand your point, but I'm still not in favour of exposing this info because it is a clear cut attempt to do arbitrary attribute passthrough to libvirt.
All the attributes show there can be determined by a simple lookup based on the name field "GRID M60-0B". So if apps want to know the number of heads, framebuffer size, etc, etc I think they should just maintain a lookup map based on name in their own code, until we explicitly model this stuff in the XML.
Once we model the attributes in the XML, this description adds no useful info that we wouldn't already be reported, and before we model it in the XML, this is just clearly an abuse of our design statement that we are not doing generic attribute passthrough.
I told Erik I'd jump in here, but I think I might agree with Dan. On the kernel side, the description attribute was an attempt to pull ourselves out of a rat hole of trying to figure out how to classify devices and then figure out what attributes for each class might be common across vendors. Clearly it'd be great to somehow know that a device is a GPU and has a resolution and graphics memory attribute, but getting there is hard. A free-form description field is sort of a catch-all where the vendor can provide a stop-gap and it's useful for human consumption. It would be inadvisable to parse it with machine code though. Including it in the XML sort of invites that possibility.
A given mdev type is supposed to be stable. It should always point to a software equivalent device, including capabilities and resources. It should therefore be valid for upper level software to use the type to lookup from a human generated table the properties for that device. Of
From an upper level software's perspective, this is super inefficient unless database like pci IDs exist. Each management software maintaining their possibly outdated lookup table may lead to inconsistence in the user experience and data.
If such database existed, there is no reason for libvirt not to use it and report the data in some sane format.
course there's about zero chance that a type will be perfectly stable, but hopefully it's a rare event and they stabilize pretty quickly. It'd be nice to one day revisit that classification and per class attributes problem in the kernel, but maybe we can let userspace figure out a common, useful set of attribute per class first. Thanks,
Alex
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Apr 18, 2017 at 10:49:40AM +0200, Martin Polednik wrote:
On 12/04/17 16:19 -0600, Alex Williamson wrote:
On Wed, 5 Apr 2017 09:21:17 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Apr 05, 2017 at 08:12:31AM +0200, Erik Skultety wrote:
On Tue, Apr 04, 2017 at 04:23:18PM +0100, Daniel P. Berrange wrote:
On Wed, Mar 29, 2017 at 02:51:10PM +0200, Erik Skultety wrote:
This series enables the node device driver to report information about the existing mediated devices on the host. There is no device creation involved yet. The information reported by the node device driver is split into two parts, one that is reported within the physical parent's capabilities (the generic stuff that comes from the mdev types' sysfs attributes, note the 'description' attribute which is verbatim - raw,unstructured string) and the other that is reported within the mdev child device and merely contains the mdev type id, which the device was instantiated from, and the iommu group number.
Basically, the format of the XML I went for is as follows:
PCI parent: <device> <name>pci_0000_06_00_0</name> <path>/sys/devices/.../0000:06:00.0</path> <parent>pci_0000_05_08_0</parent> ... <capability type='pci'> ... <capability type='mdev'> <type id='nvidia-11'> <name>GRID M60-0B</name> <description>num_heads=2, frl_config=45, framebuffer=512M, max_resolution=2560x1600, max_instance=16</description>
This 'description' field is pretty horrific.
We were quite clear that we were not going to expose arbitrary attributes in the XML without modelling them explicitly as XML elements. Using the description field in this way is just doing arbitrary attribute passthrough via the backdoor - it is clear that applications are doing to end up parsing this 'description' data and will them complain if we later change it.
I remember us stating that, but this is the case with NVIDIA (who at least reports some useful information in it) and Intel - what if other vendor comes and creates a valid, human readable description of a device without specifying any attributes like in the case above? Just because some vendors "abused" the attribute doesn't mean we should stop reporting it completely. IMHO the name "description" should mean that it's something raw, possibly unstructured, and thus the applications should treat it that way. Now, this is my bad and I need to revisit the last patch with documentation and add a paragraph for the <description> element as an optional element with raw data.
Until all the attributes are properly unified/standardized under sysfs ABI and respected by vendors, I think we should report everything we're able to gather about a device, description included. If properly documented, nobody can complain about us breaking anything, since how do you guarantee format-less raw string anyway? After all, applications will end up parsing it anyway, be it from our XML or not.
I understand your point, but I'm still not in favour of exposing this info because it is a clear cut attempt to do arbitrary attribute passthrough to libvirt.
All the attributes show there can be determined by a simple lookup based on the name field "GRID M60-0B". So if apps want to know the number of heads, framebuffer size, etc, etc I think they should just maintain a lookup map based on name in their own code, until we explicitly model this stuff in the XML.
Once we model the attributes in the XML, this description adds no useful info that we wouldn't already be reported, and before we model it in the XML, this is just clearly an abuse of our design statement that we are not doing generic attribute passthrough.
I told Erik I'd jump in here, but I think I might agree with Dan. On the kernel side, the description attribute was an attempt to pull ourselves out of a rat hole of trying to figure out how to classify devices and then figure out what attributes for each class might be common across vendors. Clearly it'd be great to somehow know that a device is a GPU and has a resolution and graphics memory attribute, but getting there is hard. A free-form description field is sort of a catch-all where the vendor can provide a stop-gap and it's useful for human consumption. It would be inadvisable to parse it with machine code though. Including it in the XML sort of invites that possibility.
A given mdev type is supposed to be stable. It should always point to a software equivalent device, including capabilities and resources. It should therefore be valid for upper level software to use the type to lookup from a human generated table the properties for that device. Of
From an upper level software's perspective, this is super inefficient unless database like pci IDs exist. Each management software maintaining their possibly outdated lookup table may lead to inconsistence in the user experience and data.
If such database existed, there is no reason for libvirt not to use it and report the data in some sane format.
IMHO it would be a waste of time for libvirt to do this. Effort would be better spent implementing the solution we requested right at the start, which is for the kernel to export the information in a stable format via dedicated sysfs attributes. Any userspace DB in applications is just a short term lack to workaround this missing kernel feature. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

On Tue, Apr 18, 2017 at 09:52:38AM +0100, Daniel P. Berrange wrote:
On Tue, Apr 18, 2017 at 10:49:40AM +0200, Martin Polednik wrote:
On 12/04/17 16:19 -0600, Alex Williamson wrote:
On Wed, 5 Apr 2017 09:21:17 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Apr 05, 2017 at 08:12:31AM +0200, Erik Skultety wrote:
On Tue, Apr 04, 2017 at 04:23:18PM +0100, Daniel P. Berrange wrote:
On Wed, Mar 29, 2017 at 02:51:10PM +0200, Erik Skultety wrote: > This series enables the node device driver to report information about the > existing mediated devices on the host. There is no device creation involved > yet. The information reported by the node device driver is split into two > parts, one that is reported within the physical parent's capabilities > (the generic stuff that comes from the mdev types' sysfs attributes, note the > 'description' attribute which is verbatim - raw,unstructured string) and the > other that is reported within the mdev child device and merely contains the > mdev type id, which the device was instantiated from, and the iommu group > number. > > Basically, the format of the XML I went for is as follows: > > PCI parent: > <device> > <name>pci_0000_06_00_0</name> > <path>/sys/devices/.../0000:06:00.0</path> > <parent>pci_0000_05_08_0</parent> > ... > <capability type='pci'> > ... > <capability type='mdev'> > <type id='nvidia-11'> > <name>GRID M60-0B</name> > <description>num_heads=2, frl_config=45, framebuffer=512M, max_resolution=2560x1600, max_instance=16</description>
This 'description' field is pretty horrific.
We were quite clear that we were not going to expose arbitrary attributes in the XML without modelling them explicitly as XML elements. Using the description field in this way is just doing arbitrary attribute passthrough via the backdoor - it is clear that applications are doing to end up parsing this 'description' data and will them complain if we later change it.
I remember us stating that, but this is the case with NVIDIA (who at least reports some useful information in it) and Intel - what if other vendor comes and creates a valid, human readable description of a device without specifying any attributes like in the case above? Just because some vendors "abused" the attribute doesn't mean we should stop reporting it completely. IMHO the name "description" should mean that it's something raw, possibly unstructured, and thus the applications should treat it that way. Now, this is my bad and I need to revisit the last patch with documentation and add a paragraph for the <description> element as an optional element with raw data.
Until all the attributes are properly unified/standardized under sysfs ABI and respected by vendors, I think we should report everything we're able to gather about a device, description included. If properly documented, nobody can complain about us breaking anything, since how do you guarantee format-less raw string anyway? After all, applications will end up parsing it anyway, be it from our XML or not.
I understand your point, but I'm still not in favour of exposing this info because it is a clear cut attempt to do arbitrary attribute passthrough to libvirt.
All the attributes show there can be determined by a simple lookup based on the name field "GRID M60-0B". So if apps want to know the number of heads, framebuffer size, etc, etc I think they should just maintain a lookup map based on name in their own code, until we explicitly model this stuff in the XML.
Once we model the attributes in the XML, this description adds no useful info that we wouldn't already be reported, and before we model it in the XML, this is just clearly an abuse of our design statement that we are not doing generic attribute passthrough.
I told Erik I'd jump in here, but I think I might agree with Dan. On the kernel side, the description attribute was an attempt to pull ourselves out of a rat hole of trying to figure out how to classify devices and then figure out what attributes for each class might be common across vendors. Clearly it'd be great to somehow know that a device is a GPU and has a resolution and graphics memory attribute, but getting there is hard. A free-form description field is sort of a catch-all where the vendor can provide a stop-gap and it's useful for human consumption. It would be inadvisable to parse it with machine code though. Including it in the XML sort of invites that possibility.
A given mdev type is supposed to be stable. It should always point to a software equivalent device, including capabilities and resources. It should therefore be valid for upper level software to use the type to lookup from a human generated table the properties for that device. Of
From an upper level software's perspective, this is super inefficient unless database like pci IDs exist. Each management software maintaining their possibly outdated lookup table may lead to inconsistence in the user experience and data.
I agree, but the same stays true for any software in the application stack just as Dan pointed that out below. Erik
If such database existed, there is no reason for libvirt not to use it and report the data in some sane format.
IMHO it would be a waste of time for libvirt to do this. Effort would be better spent implementing the solution we requested right at the start, which is for the kernel to export the information in a stable format via dedicated sysfs attributes. Any userspace DB in applications is just a short term lack to workaround this missing kernel feature.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

On Tue, 18 Apr 2017 09:52:38 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, Apr 18, 2017 at 10:49:40AM +0200, Martin Polednik wrote:
On 12/04/17 16:19 -0600, Alex Williamson wrote:
On Wed, 5 Apr 2017 09:21:17 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Apr 05, 2017 at 08:12:31AM +0200, Erik Skultety wrote:
On Tue, Apr 04, 2017 at 04:23:18PM +0100, Daniel P. Berrange wrote:
On Wed, Mar 29, 2017 at 02:51:10PM +0200, Erik Skultety wrote: > This series enables the node device driver to report information about the > existing mediated devices on the host. There is no device creation involved > yet. The information reported by the node device driver is split into two > parts, one that is reported within the physical parent's capabilities > (the generic stuff that comes from the mdev types' sysfs attributes, note the > 'description' attribute which is verbatim - raw,unstructured string) and the > other that is reported within the mdev child device and merely contains the > mdev type id, which the device was instantiated from, and the iommu group > number. > > Basically, the format of the XML I went for is as follows: > > PCI parent: > <device> > <name>pci_0000_06_00_0</name> > <path>/sys/devices/.../0000:06:00.0</path> > <parent>pci_0000_05_08_0</parent> > ... > <capability type='pci'> > ... > <capability type='mdev'> > <type id='nvidia-11'> > <name>GRID M60-0B</name> > <description>num_heads=2, frl_config=45, framebuffer=512M, max_resolution=2560x1600, max_instance=16</description>
This 'description' field is pretty horrific.
We were quite clear that we were not going to expose arbitrary attributes in the XML without modelling them explicitly as XML elements. Using the description field in this way is just doing arbitrary attribute passthrough via the backdoor - it is clear that applications are doing to end up parsing this 'description' data and will them complain if we later change it.
I remember us stating that, but this is the case with NVIDIA (who at least reports some useful information in it) and Intel - what if other vendor comes and creates a valid, human readable description of a device without specifying any attributes like in the case above? Just because some vendors "abused" the attribute doesn't mean we should stop reporting it completely. IMHO the name "description" should mean that it's something raw, possibly unstructured, and thus the applications should treat it that way. Now, this is my bad and I need to revisit the last patch with documentation and add a paragraph for the <description> element as an optional element with raw data.
Until all the attributes are properly unified/standardized under sysfs ABI and respected by vendors, I think we should report everything we're able to gather about a device, description included. If properly documented, nobody can complain about us breaking anything, since how do you guarantee format-less raw string anyway? After all, applications will end up parsing it anyway, be it from our XML or not.
I understand your point, but I'm still not in favour of exposing this info because it is a clear cut attempt to do arbitrary attribute passthrough to libvirt.
All the attributes show there can be determined by a simple lookup based on the name field "GRID M60-0B". So if apps want to know the number of heads, framebuffer size, etc, etc I think they should just maintain a lookup map based on name in their own code, until we explicitly model this stuff in the XML.
Once we model the attributes in the XML, this description adds no useful info that we wouldn't already be reported, and before we model it in the XML, this is just clearly an abuse of our design statement that we are not doing generic attribute passthrough.
I told Erik I'd jump in here, but I think I might agree with Dan. On the kernel side, the description attribute was an attempt to pull ourselves out of a rat hole of trying to figure out how to classify devices and then figure out what attributes for each class might be common across vendors. Clearly it'd be great to somehow know that a device is a GPU and has a resolution and graphics memory attribute, but getting there is hard. A free-form description field is sort of a catch-all where the vendor can provide a stop-gap and it's useful for human consumption. It would be inadvisable to parse it with machine code though. Including it in the XML sort of invites that possibility.
A given mdev type is supposed to be stable. It should always point to a software equivalent device, including capabilities and resources. It should therefore be valid for upper level software to use the type to lookup from a human generated table the properties for that device. Of
From an upper level software's perspective, this is super inefficient unless database like pci IDs exist. Each management software maintaining their possibly outdated lookup table may lead to inconsistence in the user experience and data.
If such database existed, there is no reason for libvirt not to use it and report the data in some sane format.
IMHO it would be a waste of time for libvirt to do this. Effort would be better spent implementing the solution we requested right at the start, which is for the kernel to export the information in a stable format via dedicated sysfs attributes. Any userspace DB in applications is just a short term lack to workaround this missing kernel feature.
...and a pony too! Seriously, "requesting" this feature isn't necessarily going to make it happen. How do we classify devices? How do we define attributes per class that multiple vendors can agree to and are extensible to future vendors? This seems exactly like the sort of thing that needs to be flushed out in userspace before committing to poorly defined ABIs in the kernel. Thanks, Alex
participants (6)
-
Alex Williamson
-
Daniel P. Berrange
-
Erik Skultety
-
Martin Polednik
-
Pavel Hrdina
-
Peter Krempa