[PATCH v2] s390/vfio-ap: Driver feature advertisement

[PATCH v2] s390/vfio-ap: Driver feature advertisement

[Jason J. Herne]

Advertise features of the driver for the benefit of automated tooling
like Libvirt and mdevctl.

Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
---
 Documentation/arch/s390/vfio-ap.rst | 34 +++++++++++++++++++++++++++++
 drivers/s390/crypto/vfio_ap_drv.c   | 13 +++++++++++
 2 files changed, 47 insertions(+)

diff --git a/Documentation/arch/s390/vfio-ap.rst b/Documentation/arch/s390/vfio-ap.rst
index ea744cbc8687..22f1965af500 100644
--- a/Documentation/arch/s390/vfio-ap.rst
+++ b/Documentation/arch/s390/vfio-ap.rst
@@ -999,6 +999,40 @@ the vfio_ap mediated device to which it is assigned as long as each new APQN
 resulting from plugging it in references a queue device bound to the vfio_ap
 device driver.
 
+Driver Features
+===============
+The vfio_ap driver exposes a sysfs file containing supported features.
+This exists so third party tools (like Libvirt and mdevctl) can query the
+availability of specific features.
+
+The features list can be found here: /sys/bus/matrix/devices/matrix/features
+
+Entries are \n delimited. Each entry contains a key value pair. The key is made
+up of a combination of alphanumeric and underscore characters. The separator
+consists of a space, a colon and then another space. The value consists of
+alphanumeric, space, and underscore characters.
+
+Example:
+cat /sys/bus/matrix/devices/matrix/features
+flags : guest_matrix dyn ap_config
+
+Presently only a single field named flags is defined. It is meant to advertise a
+list of features the driver provides. The flags fields advertises the following
+features:
+
+---------------+---------------------------------------------------------------+
+| Flag         | Description                                                   |
++==============+===============================================================+
+| guest_matrix | guest_matrix attribute exists. It reports the matrix of       |
+|              | adapters and domains that are or will be passed through to a  |
+|              | guest when the mdev is attached to it.                        |
++--------------+---------------------------------------------------------------+
+| hotplug      | Indicates hot plug/unplug of AP adapters, domains and control |
+|              | domains for a guest to which the mdev is attached.            |
++------------+-----------------------------------------------------------------+
+| ap_config    | ap_config interface for one-shot modifications to mdev config |
++--------------+---------------------------------------------------------------+
+
 Limitations
 ===========
 Live guest migration is not supported for guests using AP devices without
diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 4aeb3e1213c7..e3fc444ff404 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -26,6 +26,18 @@ MODULE_LICENSE("GPL v2");
 struct ap_matrix_dev *matrix_dev;
 debug_info_t *vfio_ap_dbf_info;
 
+static ssize_t features_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "flags : guest_matrix hotplug ap_config\n");
+}
+static DEVICE_ATTR_RO(features);
+
+static struct attribute *matrix_dev_attrs[] = {
+	&dev_attr_features.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(matrix_dev);
+
 /* Only type 10 adapters (CEX4 and later) are supported
  * by the AP matrix device driver
  */
@@ -68,6 +80,7 @@ static struct device_driver matrix_driver = {
 	.name = "vfio_ap",
 	.bus = &matrix_bus,
 	.suppress_bind_attrs = true,
+	.dev_groups = matrix_dev_groups,
 };
 
 static int vfio_ap_matrix_dev_create(void)
-- 
2.46.0

Re: [PATCH v2] s390/vfio-ap: Driver feature advertisement

[Heiko Carstens]

On Tue, Sep 10, 2024 at 07:34:40AM -0400, Jason J. Herne wrote:
> Advertise features of the driver for the benefit of automated tooling
> like Libvirt and mdevctl.
> 
> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> ---
>  Documentation/arch/s390/vfio-ap.rst | 34 +++++++++++++++++++++++++++++
>  drivers/s390/crypto/vfio_ap_drv.c   | 13 +++++++++++
>  2 files changed, 47 insertions(+)

...

> +Driver Features
> +===============
> +The vfio_ap driver exposes a sysfs file containing supported features.
> +This exists so third party tools (like Libvirt and mdevctl) can query the
> +availability of specific features.
> +
> +The features list can be found here: /sys/bus/matrix/devices/matrix/features
> +
> +Entries are \n delimited. Each entry contains a key value pair. The key is made
> +up of a combination of alphanumeric and underscore characters. The separator
> +consists of a space, a colon and then another space. The value consists of
> +alphanumeric, space, and underscore characters.
> +
> +Example:
> +cat /sys/bus/matrix/devices/matrix/features
> +flags : guest_matrix dyn ap_config
> +
> +Presently only a single field named flags is defined. It is meant to advertise a
> +list of features the driver provides. The flags fields advertises the following
> +features:

I stumbled across this only now: sysfs files are not supposed to have
several key value pairs. Actually the file(name) itself is supposed to
be the key and its contents are the value. So I would expect:

cat /sys/bus/matrix/devices/matrix/flags
guest_matrix dyn ap_config

Which is also easier to parse. Is there a good reason why this does
not follow the general approach for sysfs files?

Re: [PATCH v2] s390/vfio-ap: Driver feature advertisement

[Jason J. Herne]

On 9/11/24 2:57 AM, Heiko Carstens wrote:
> On Tue, Sep 10, 2024 at 07:34:40AM -0400, Jason J. Herne wrote:
>> Advertise features of the driver for the benefit of automated tooling
>> like Libvirt and mdevctl.
>>
>> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
>> Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> ---
>>   Documentation/arch/s390/vfio-ap.rst | 34 +++++++++++++++++++++++++++++
>>   drivers/s390/crypto/vfio_ap_drv.c   | 13 +++++++++++
>>   2 files changed, 47 insertions(+)
> 
> ...
> 
>> +Driver Features
>> +===============
>> +The vfio_ap driver exposes a sysfs file containing supported features.
>> +This exists so third party tools (like Libvirt and mdevctl) can query the
>> +availability of specific features.
>> +
>> +The features list can be found here: /sys/bus/matrix/devices/matrix/features
>> +
>> +Entries are \n delimited. Each entry contains a key value pair. The key is made
>> +up of a combination of alphanumeric and underscore characters. The separator
>> +consists of a space, a colon and then another space. The value consists of
>> +alphanumeric, space, and underscore characters.
>> +
>> +Example:
>> +cat /sys/bus/matrix/devices/matrix/features
>> +flags : guest_matrix dyn ap_config
>> +
>> +Presently only a single field named flags is defined. It is meant to advertise a
>> +list of features the driver provides. The flags fields advertises the following
>> +features:
> 
> I stumbled across this only now: sysfs files are not supposed to have
> several key value pairs. Actually the file(name) itself is supposed to
> be the key and its contents are the value. So I would expect:
> 
> cat /sys/bus/matrix/devices/matrix/flags
> guest_matrix dyn ap_config
> 
> Which is also easier to parse. Is there a good reason why this does
> not follow the general approach for sysfs files?


We talked about a few designs for this feature. This patch represents 
where we landed. I'm happy to make the change to the way you suggest but 
I'll give some time for Boris or Tony to speak up if they disagree.

Re: [PATCH v2] s390/vfio-ap: Driver feature advertisement

[Boris Fiuczynski]

On 9/12/24 2:24 PM, Jason J. Herne wrote:
> On 9/11/24 2:57 AM, Heiko Carstens wrote:
>> On Tue, Sep 10, 2024 at 07:34:40AM -0400, Jason J. Herne wrote:
>>> Advertise features of the driver for the benefit of automated tooling
>>> like Libvirt and mdevctl.
>>>
>>> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
>>> Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>>> ---
>>>   Documentation/arch/s390/vfio-ap.rst | 34 +++++++++++++++++++++++++++++
>>>   drivers/s390/crypto/vfio_ap_drv.c   | 13 +++++++++++
>>>   2 files changed, 47 insertions(+)
>>
>> ...
>>
>>> +Driver Features
>>> +===============
>>> +The vfio_ap driver exposes a sysfs file containing supported features.
>>> +This exists so third party tools (like Libvirt and mdevctl) can 
>>> query the
>>> +availability of specific features.
>>> +
>>> +The features list can be found here: 
>>> /sys/bus/matrix/devices/matrix/features
>>> +
>>> +Entries are \n delimited. Each entry contains a key value pair. The 
>>> key is made
>>> +up of a combination of alphanumeric and underscore characters. The 
>>> separator
>>> +consists of a space, a colon and then another space. The value 
>>> consists of
>>> +alphanumeric, space, and underscore characters.
>>> +
>>> +Example:
>>> +cat /sys/bus/matrix/devices/matrix/features
>>> +flags : guest_matrix dyn ap_config
>>> +
>>> +Presently only a single field named flags is defined. It is meant to 
>>> advertise a
>>> +list of features the driver provides. The flags fields advertises 
>>> the following
>>> +features:
>>
>> I stumbled across this only now: sysfs files are not supposed to have
>> several key value pairs. Actually the file(name) itself is supposed to
>> be the key and its contents are the value. So I would expect:
>>
>> cat /sys/bus/matrix/devices/matrix/flags
>> guest_matrix dyn ap_config
>>
>> Which is also easier to parse. Is there a good reason why this does
>> not follow the general approach for sysfs files?
> 
> 
> We talked about a few designs for this feature. This patch represents 
> where we landed. I'm happy to make the change to the way you suggest but 
> I'll give some time for Boris or Tony to speak up if they disagree.

Heiko's expectation is fine by me.

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v2] s390/vfio-ap: Driver feature advertisement

[Anthony Krowiak]

On 9/11/24 2:57 AM, Heiko Carstens wrote:
> On Tue, Sep 10, 2024 at 07:34:40AM -0400, Jason J. Herne wrote:
>> Advertise features of the driver for the benefit of automated tooling
>> like Libvirt and mdevctl.
>>
>> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
>> Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> ---
>>   Documentation/arch/s390/vfio-ap.rst | 34 +++++++++++++++++++++++++++++
>>   drivers/s390/crypto/vfio_ap_drv.c   | 13 +++++++++++
>>   2 files changed, 47 insertions(+)
> ...
>
>> +Driver Features
>> +===============
>> +The vfio_ap driver exposes a sysfs file containing supported features.
>> +This exists so third party tools (like Libvirt and mdevctl) can query the
>> +availability of specific features.
>> +
>> +The features list can be found here: /sys/bus/matrix/devices/matrix/features
>> +
>> +Entries are \n delimited. Each entry contains a key value pair. The key is made
>> +up of a combination of alphanumeric and underscore characters. The separator
>> +consists of a space, a colon and then another space. The value consists of
>> +alphanumeric, space, and underscore characters.
>> +
>> +Example:
>> +cat /sys/bus/matrix/devices/matrix/features
>> +flags : guest_matrix dyn ap_config
>> +
>> +Presently only a single field named flags is defined. It is meant to advertise a
>> +list of features the driver provides. The flags fields advertises the following
>> +features:
> I stumbled across this only now: sysfs files are not supposed to have
> several key value pairs. Actually the file(name) itself is supposed to
> be the key and its contents are the value. So I would expect:
>
> cat /sys/bus/matrix/devices/matrix/flags
> guest_matrix dyn ap_config
>
> Which is also easier to parse. Is there a good reason why this does
> not follow the general approach for sysfs files?

I am okay with this, but I would keep the sysfs filename  'features'.

cat /sys/bus/matrix/devices/matrix/features
guest_matrix dyn ap_config

[PATCH v2] s390/vfio-ap: Driver feature advertisement

[Jason J. Herne]

Advertise features of the driver for the benefit of automated tooling
like Libvirt and mdevctl.

Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
---
 Documentation/arch/s390/vfio-ap.rst | 34 +++++++++++++++++++++++++++++
 drivers/s390/crypto/vfio_ap_drv.c   | 13 +++++++++++
 2 files changed, 47 insertions(+)

diff --git a/Documentation/arch/s390/vfio-ap.rst b/Documentation/arch/s390/vfio-ap.rst
index ea744cbc8687..22f1965af500 100644
--- a/Documentation/arch/s390/vfio-ap.rst
+++ b/Documentation/arch/s390/vfio-ap.rst
@@ -999,6 +999,40 @@ the vfio_ap mediated device to which it is assigned as long as each new APQN
 resulting from plugging it in references a queue device bound to the vfio_ap
 device driver.
 
+Driver Features
+===============
+The vfio_ap driver exposes a sysfs file containing supported features.
+This exists so third party tools (like Libvirt and mdevctl) can query the
+availability of specific features.
+
+The features list can be found here: /sys/bus/matrix/devices/matrix/features
+
+Entries are \n delimited. Each entry contains a key value pair. The key is made
+up of a combination of alphanumeric and underscore characters. The separator
+consists of a space, a colon and then another space. The value consists of
+alphanumeric, space, and underscore characters.
+
+Example:
+cat /sys/bus/matrix/devices/matrix/features
+flags : guest_matrix dyn ap_config
+
+Presently only a single field named flags is defined. It is meant to advertise a
+list of features the driver provides. The flags fields advertises the following
+features:
+
+---------------+---------------------------------------------------------------+
+| Flag         | Description                                                   |
++==============+===============================================================+
+| guest_matrix | guest_matrix attribute exists. It reports the matrix of       |
+|              | adapters and domains that are or will be passed through to a  |
+|              | guest when the mdev is attached to it.                        |
++--------------+---------------------------------------------------------------+
+| hotplug      | Indicates hot plug/unplug of AP adapters, domains and control |
+|              | domains for a guest to which the mdev is attached.            |
++------------+-----------------------------------------------------------------+
+| ap_config    | ap_config interface for one-shot modifications to mdev config |
++--------------+---------------------------------------------------------------+
+
 Limitations
 ===========
 Live guest migration is not supported for guests using AP devices without
diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 4aeb3e1213c7..e3fc444ff404 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -26,6 +26,18 @@ MODULE_LICENSE("GPL v2");
 struct ap_matrix_dev *matrix_dev;
 debug_info_t *vfio_ap_dbf_info;
 
+static ssize_t features_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "flags : guest_matrix hotplug ap_config\n");
+}
+static DEVICE_ATTR_RO(features);
+
+static struct attribute *matrix_dev_attrs[] = {
+	&dev_attr_features.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(matrix_dev);
+
 /* Only type 10 adapters (CEX4 and later) are supported
  * by the AP matrix device driver
  */
@@ -68,6 +80,7 @@ static struct device_driver matrix_driver = {
 	.name = "vfio_ap",
 	.bus = &matrix_bus,
 	.suppress_bind_attrs = true,
+	.dev_groups = matrix_dev_groups,
 };
 
 static int vfio_ap_matrix_dev_create(void)
-- 
2.46.0