[PATCH] PCI: export pci device assignment info as read only via sysfs

[PATCH] PCI: export pci device assignment info as read only via sysfs

[Ethan Zhao]

This patch exports assignment flag of PCI device via sysfs, so user
space application could know if PCI device was assigned to guest OS
or not via interface as following from viewpoint of host device
management:

 PCI device was assigned to guest OS
 $cat /sys/devices/pci0000:00/0000:00:1f.3/assigned
 1

 PCI device isn't assigned to guest OS
 $cat /sys/devices/pci0000:00/0000:00:1f.3/assigned
 0

Compiled and tested with v3.18.1

Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
---
 drivers/pci/pci-sysfs.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 2c6643f..0ac3780 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -547,6 +547,15 @@ static ssize_t driver_override_show(struct device *dev,
 }
 static DEVICE_ATTR_RW(driver_override);
 
+static ssize_t assigned_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%d\n", pci_is_dev_assigned(pdev) ? 1 : 0);
+}
+static DEVICE_ATTR_RO(assigned);
+
 static struct attribute *pci_dev_attrs[] = {
 	&dev_attr_resource.attr,
 	&dev_attr_vendor.attr,
@@ -573,6 +582,7 @@ static struct attribute *pci_dev_attrs[] = {
 	&dev_attr_devspec.attr,
 #endif
 	&dev_attr_driver_override.attr,
+	&dev_attr_assigned.attr,
 	NULL,
 };
 
-- 
1.8.3.1

Re: [PATCH] PCI: export pci device assignment info as read only via sysfs

[Andreas Mohr]

Hi,

Ethan Zhao wrote:
> This patch exports assignment flag of PCI device via sysfs, so user
> space application could know if PCI device was assigned to guest OS
> or not via interface as following from viewpoint of host device
> management:
> 
>  PCI device was assigned to guest OS
>  $cat /sys/devices/pci0000:00/0000:00:1f.3/assigned
>  1
>  PCI device isn't assigned to guest OS
>  $cat /sys/devices/pci0000:00/0000:00:1f.3/assigned
>  0

For such a sysfs API (and similar cases in general!),
do we actually want this relatively simple interface of
"assigned" false/true?
Or should it rather be something like an "ownership" attribute
which could then indicate interface-fixed(!) string values
such as "system"/"vm_guest"/FUTURE_EXTENSION_POSSIBILITY?

IOW, "assigned" is quite a bit less flexible (and quite possibly even less
usability-suitable from a naming POV - "assigned"...??)
but of course simpler (which may or may not pose an advantage from an
interface versioning/evolution POV).

To further clarify things, this "assigned" interface as-is
can merely tell apart
the "device is occupied" vs. "device is not occupied" cases
yet it could easily provide exact ownership details instead.
And I'm also wondering whether or not it's good
to have it directly/symmetrically named "assigned"
due to it currently(!!) being based internally
on an equally-named pci_is_dev_assigned() API.


I should probably read up on sysfs item design hints/guidelines
which I'd trust to be accurately explained in a Documentation/ file ;)
Hmm, seems Documentation/sysfs-rules.txt has only one closing remark
which seems relevant:
====
  Userspace applications can, however, expect the format and contents of
  the attribute files to remain consistent in the absence of a version
  attribute change in the context of a given attribute.
====
This would be fulfilled by both the current and my suggested variant
of this interface, AFAICS.

Thanks,

Andreas Mohr

Re: [PATCH] PCI: export pci device assignment info as read only via sysfs

[Alex Williamson]

----- Original Message -----
> This patch exports assignment flag of PCI device via sysfs, so user
> space application could know if PCI device was assigned to guest OS
> or not via interface as following from viewpoint of host device
> management:
> 
>  PCI device was assigned to guest OS
>  $cat /sys/devices/pci0000:00/0000:00:1f.3/assigned
>  1
> 
>  PCI device isn't assigned to guest OS
>  $cat /sys/devices/pci0000:00/0000:00:1f.3/assigned
>  0
> 
> Compiled and tested with v3.18.1
> 
> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
> ---
>  drivers/pci/pci-sysfs.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 2c6643f..0ac3780 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -547,6 +547,15 @@ static ssize_t driver_override_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(driver_override);
>  
> +static ssize_t assigned_show(struct device *dev, struct device_attribute
> *attr,
> +			 char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return sprintf(buf, "%d\n", pci_is_dev_assigned(pdev) ? 1 : 0);
> +}
> +static DEVICE_ATTR_RO(assigned);
> +
>  static struct attribute *pci_dev_attrs[] = {
>  	&dev_attr_resource.attr,
>  	&dev_attr_vendor.attr,
> @@ -573,6 +582,7 @@ static struct attribute *pci_dev_attrs[] = {
>  	&dev_attr_devspec.attr,
>  #endif
>  	&dev_attr_driver_override.attr,
> +	&dev_attr_assigned.attr,
>  	NULL,
>  };
>  

NAK.  The assigned attribute of a device is already in contention, it's racy and not universally used.  If we can't even make it a consistent and reliable internal interface, we certainly shouldn't be exposing it to userspace.  Thanks,

Alex