Re: [PATCH libvirt v2 06/11] nodedev: detect AP matrix device
On 11/30/20 2:46 PM, Erik Skultety wrote:
On Fri, Nov 13, 2020 at 03:45:33PM +0100, Shalini Chellathurai Saroja wrote:
On 11/12/20 9:29 PM, Jonathon Jongsma wrote:
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 = >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?
Yes, it is not formatted into the node device xml.
I will include this change in the patch 10.
+
+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?
The name generated with udevGenerateDeviceName() is matrix_matrix (as both
udev_device_get_subsystem and udev_device_get_sysname returns matrix), which
is not very helpful, so we decided to go with ap_matrix instead.
But if that is the case, I'm wondering why are you trying to generate it
dynamically in the first place, why not hardcoding it to "ap_matrix"? I must be
missing something here.
Regards,
Erik
ok, I will hard code it. Thank you for the review.
--
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH libvirt v2 06/11] nodedev: detect AP matrix device
On Fri, Nov 13, 2020 at 03:45:33PM +0100, Shalini Chellathurai Saroja wrote:
>
> On 11/12/20 9:29 PM, Jonathon Jongsma wrote:
> > > 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 = >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?
> >
> Yes, it is not formatted into the node device xml.
>
> I will include this change in the patch 10.
>
> >
> > > +
> > > +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?
> The name generated with udevGenerateDeviceName() is matrix_matrix (as both
> udev_device_get_subsystem and udev_device_get_sysname returns matrix), which
> is not very helpful, so we decided to go with ap_matrix instead.
But if that is the case, I'm wondering why are you trying to generate it
dynamically in the first place, why not hardcoding it to "ap_matrix"? I must be
missing something here.
Regards,
Erik
Re: [PATCH libvirt v2 06/11] nodedev: detect AP matrix device
On 11/12/20 9:29 PM, Jonathon Jongsma wrote:
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 = >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?
Yes, it is not formatted into the node device xml.
I will include this change in the patch 10.
+
+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?
The name generated with udevGenerateDeviceName() is matrix_matrix (as
both udev_device_get_subsystem and udev_device_get_sysname returns
matrix), which is not very helpful, so we decided to go with ap_matrix
instead.
--
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH libvirt v2 06/11] nodedev: detect AP matrix device
On Thu, 12 Nov 2020 13:15:14 +0100
Shalini Chellathurai Saroja wrote:
> Add support for AP matrix device in libvirt node device driver.
>
> Signed-off-by: Shalini Chellathurai Saroja
> Reviewed-by: Bjoern Walk
> Reviewed-by: Boris Fiuczynski
> ---
> 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 @@
>
>
>
> +
>
>
>
> @@ -691,6 +692,12 @@
>
>
>
> +
> +
> + ap_matrix
> +
> +
> +
>
>
>
> 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,
> >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, _dev->mdev_types,
>_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
