On Thu, 12 Nov 2020 13:15:14 +0100
Shalini Chellathurai Saroja <shalini@xxxxxxxxxxxxx> wrote:
> Add support for AP matrix device in libvirt node device driver.
>
> Signed-off-by: Shalini Chellathurai Saroja <shalini@xxxxxxxxxxxxx>
> Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxx>
> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
> ---
> docs/schemas/nodedev.rng | 7 +++++++
> src/conf/node_device_conf.c | 11 ++++++++++-
> src/conf/node_device_conf.h | 8 ++++++++
> src/conf/virnodedeviceobj.c | 1 +
> src/node_device/node_device_udev.c | 23 +++++++++++++++++++++++
> tools/virsh-nodedev.c | 1 +
> 6 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
> index 51cb8854..5be21145 100644
> --- a/docs/schemas/nodedev.rng
> +++ b/docs/schemas/nodedev.rng
> @@ -89,6 +89,7 @@
> <ref name="capvdpa"/>
> <ref name="capapcard"/>
> <ref name="capapqueue"/>
> + <ref name="capapmatrix"/>
> </choice>
> </element>
> </define>
> @@ -691,6 +692,12 @@
> </element>
> </define>
>
> + <define name='capapmatrix'>
> + <attribute name='type'>
> + <value>ap_matrix</value>
> + </attribute>
> + </define>
> +
> <define name='address'>
> <element name='address'>
> <attribute name='domain'><ref name='hexuint'/></attribute>
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index fa3b823f..f3d146a5 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -69,6 +69,7 @@ VIR_ENUM_IMPL(virNodeDevCap,
> "vdpa",
> "ap_card",
> "ap_queue",
> + "ap_matrix",
> );
>
> VIR_ENUM_IMPL(virNodeDevNetCap,
> @@ -665,6 +666,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef
> *def) case VIR_NODE_DEV_CAP_MDEV_TYPES:
> case VIR_NODE_DEV_CAP_FC_HOST:
> case VIR_NODE_DEV_CAP_VPORTS:
> + case VIR_NODE_DEV_CAP_AP_MATRIX:
> case VIR_NODE_DEV_CAP_LAST:
> break;
> }
> @@ -2075,6 +2077,9 @@ virNodeDevCapsDefParseXML(xmlXPathContextPtr
> ctxt, ret = virNodeDevCapAPQueueParseXML(ctxt, def, node,
> &caps->data.ap_queue);
> break;
> + case VIR_NODE_DEV_CAP_AP_MATRIX:
> + ret = 0;
> + break;
> case VIR_NODE_DEV_CAP_MDEV_TYPES:
> case VIR_NODE_DEV_CAP_FC_HOST:
> case VIR_NODE_DEV_CAP_VPORTS:
> @@ -2396,6 +2401,9 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
> virMediatedDeviceTypeFree(data->ccw_dev.mdev_types[i]);
> VIR_FREE(data->ccw_dev.mdev_types);
> break;
> + case VIR_NODE_DEV_CAP_AP_MATRIX:
> + VIR_FREE(data->ap_matrix.addr);
> + break;
> case VIR_NODE_DEV_CAP_MDEV_TYPES:
> case VIR_NODE_DEV_CAP_DRM:
> case VIR_NODE_DEV_CAP_FC_HOST:
> @@ -2465,6 +2473,7 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def)
> case VIR_NODE_DEV_CAP_VDPA:
> case VIR_NODE_DEV_CAP_AP_CARD:
> case VIR_NODE_DEV_CAP_AP_QUEUE:
> + case VIR_NODE_DEV_CAP_AP_MATRIX:
> case VIR_NODE_DEV_CAP_LAST:
> break;
> }
> @@ -2806,6 +2815,7 @@ virNodeDeviceGetPCIDynamicCaps(const char
> *sysfsPath, &pci_dev->mdev_types,
> &pci_dev->nmdev_types) < 0)
> return -1;
> +
unrelated change
> if (pci_dev->nmdev_types > 0)
> pci_dev->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_MDEV;
>
> @@ -2849,7 +2859,6 @@ virNodeDeviceGetPCIDynamicCaps(const char
> *sysfsPath G_GNUC_UNUSED, return -1;
> }
>
> -
unrelated change
> int virNodeDeviceGetSCSITargetCaps(const char *sysfsPath
> G_GNUC_UNUSED, virNodeDevCapSCSITargetPtr scsi_target G_GNUC_UNUSED)
> {
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index b580d6cf..b863653b 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -68,6 +68,7 @@ typedef enum {
> VIR_NODE_DEV_CAP_VDPA, /* vDPA device */
> VIR_NODE_DEV_CAP_AP_CARD, /* s390 AP Card device */
> VIR_NODE_DEV_CAP_AP_QUEUE, /* s390 AP Queue */
> + VIR_NODE_DEV_CAP_AP_MATRIX, /* s390 AP Matrix device */
>
> VIR_NODE_DEV_CAP_LAST
> } virNodeDevCapType;
> @@ -304,6 +305,12 @@ struct _virNodeDevCapAPQueue {
> unsigned int ap_domain;
> };
>
> +typedef struct _virNodeDevCapAPMatrix virNodeDevCapAPMatrix;
> +typedef virNodeDevCapAPMatrix *virNodeDevCapAPMatrixPtr;
> +struct _virNodeDevCapAPMatrix {
> + char *addr;
> +};
> +
> typedef struct _virNodeDevCapData virNodeDevCapData;
> typedef virNodeDevCapData *virNodeDevCapDataPtr;
> struct _virNodeDevCapData {
> @@ -325,6 +332,7 @@ struct _virNodeDevCapData {
> virNodeDevCapVDPA vdpa;
> virNodeDevCapAPCard ap_card;
> virNodeDevCapAPQueue ap_queue;
> + virNodeDevCapAPMatrix ap_matrix;
> };
> };
>
> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> index 691632c6..8fbef5f5 100644
> --- a/src/conf/virnodedeviceobj.c
> +++ b/src/conf/virnodedeviceobj.c
> @@ -719,6 +719,7 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj
> *obj, case VIR_NODE_DEV_CAP_VDPA:
> case VIR_NODE_DEV_CAP_AP_CARD:
> case VIR_NODE_DEV_CAP_AP_QUEUE:
> + case VIR_NODE_DEV_CAP_AP_MATRIX:
> 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 6bbff571..5f57000e 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1241,6 +1241,25 @@ udevProcessAPQueue(struct udev_device *device,
> }
>
>
> +static int
> +udevProcessAPMatrix(struct udev_device *device,
> + virNodeDeviceDefPtr def)
> +{
> + size_t i;
> + virNodeDevCapDataPtr data = &def->caps->data;
> +
> + data->ap_matrix.addr =
> g_strdup(udev_device_get_sysname(device));
> + def->name = g_strdup_printf("ap_%s", data->ap_matrix.addr);
So you're setting this 'addr' field, but it is not used anywhere else in
this patch. Perhaps you'll use it in upcoming patches. But it is not
formatted into the node device xml or anything like that. Is that
intentional?
> +
> + for (i = 0; i < strlen(def->name); i++) {
> + if (!(g_ascii_isalnum(*(def->name + i))))
> + *(def->name + i) = '_';
> + }
> +
> + return 0;
> +}
Out of curiosity, what's the reason that you're
hard-coding an "ap_" prefix to the nodedev name rather than just using
udevGenerateDeviceName() like all of the other device types?
> +
> +
> static int
> udevGetDeviceNodes(struct udev_device *device,
> virNodeDeviceDefPtr def)
> @@ -1326,6 +1345,8 @@ udevGetDeviceType(struct udev_device *device,
> *type = VIR_NODE_DEV_CAP_CSS_DEV;
> else if (STREQ_NULLABLE(subsystem, "vdpa"))
> *type = VIR_NODE_DEV_CAP_VDPA;
> + else if (STREQ_NULLABLE(subsystem, "matrix"))
> + *type = VIR_NODE_DEV_CAP_AP_MATRIX;
>
> VIR_FREE(subsystem);
> }
> @@ -1378,6 +1399,8 @@ udevGetDeviceDetails(struct udev_device *device,
> return udevProcessAPCard(device, def);
> case VIR_NODE_DEV_CAP_AP_QUEUE:
> return udevProcessAPQueue(device, def);
> + case VIR_NODE_DEV_CAP_AP_MATRIX:
> + return udevProcessAPMatrix(device, def);
> case VIR_NODE_DEV_CAP_MDEV_TYPES:
> case VIR_NODE_DEV_CAP_SYSTEM:
> case VIR_NODE_DEV_CAP_FC_HOST:
> diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
> index f5ffe525..f69030d2 100644
> --- a/tools/virsh-nodedev.c
> +++ b/tools/virsh-nodedev.c
> @@ -473,6 +473,7 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd
> *cmd G_GNUC_UNUSED) case VIR_NODE_DEV_CAP_AP_QUEUE:
> flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE;
> break;
> + case VIR_NODE_DEV_CAP_AP_MATRIX:
> case VIR_NODE_DEV_CAP_LAST:
> break;
> }
