[3/4] cdx: create sysfs resource files
Commit Message
Resource files provides the basic MMIO regions info to the
user-space. Also, resources<x> devices can be used to mmap the
MMIO regions in the user-space.
Co-developed-by: Puneet Gupta <puneet.gupta@amd.com>
Signed-off-by: Puneet Gupta <puneet.gupta@amd.com>
Co-developed-by: Nipun Gupta <nipun.gupta@amd.com>
Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
---
Documentation/ABI/testing/sysfs-bus-cdx | 15 +++
drivers/cdx/cdx.c | 139 +++++++++++++++++++++++-
include/linux/cdx/cdx_bus.h | 10 ++
3 files changed, 163 insertions(+), 1 deletion(-)
Comments
On Tue, Jul 11, 2023 at 05:40:26PM +0530, Abhijit Gangurde wrote:
> Resource files provides the basic MMIO regions info to the
> user-space. Also, resources<x> devices can be used to mmap the
> MMIO regions in the user-space.
>
> Co-developed-by: Puneet Gupta <puneet.gupta@amd.com>
> Signed-off-by: Puneet Gupta <puneet.gupta@amd.com>
> Co-developed-by: Nipun Gupta <nipun.gupta@amd.com>
> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
> Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> ---
> Documentation/ABI/testing/sysfs-bus-cdx | 15 +++
> drivers/cdx/cdx.c | 139 +++++++++++++++++++++++-
> include/linux/cdx/cdx_bus.h | 10 ++
> 3 files changed, 163 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-cdx b/Documentation/ABI/testing/sysfs-bus-cdx
> index d9e00058471d..6ca47b6442ce 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cdx
> +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> @@ -76,3 +76,18 @@ Description:
> For example::
>
> # echo 1 > /sys/bus/cdx/devices/.../remove
> +
> +What: /sys/bus/cdx/devices/.../resource
> +Date: July 2023
> +Contact: puneet.gupta@amd.com
> +Description:
> + The resource file contains host addresses of CDX device
> + resources. Each line of the resource file describes a region
> + with start, end, and flag fields.
If you documented what this file looked like here, it would be obvious
that this is not an acceptable sysfs file in any sense of the word.
Please do so, and then fix the patch to not do that at all.
thanks,
greg k-h
[AMD Official Use Only - General]
> > Resource files provides the basic MMIO regions info to the
> > user-space. Also, resources<x> devices can be used to mmap the
> > MMIO regions in the user-space.
> >
> > Co-developed-by: Puneet Gupta <puneet.gupta@amd.com>
> > Signed-off-by: Puneet Gupta <puneet.gupta@amd.com>
> > Co-developed-by: Nipun Gupta <nipun.gupta@amd.com>
> > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> vuuren@amd.com>
> > Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> > ---
> > Documentation/ABI/testing/sysfs-bus-cdx | 15 +++
> > drivers/cdx/cdx.c | 139 +++++++++++++++++++++++-
> > include/linux/cdx/cdx_bus.h | 10 ++
> > 3 files changed, 163 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx
> b/Documentation/ABI/testing/sysfs-bus-cdx
> > index d9e00058471d..6ca47b6442ce 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cdx
> > +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> > @@ -76,3 +76,18 @@ Description:
> > For example::
> >
> > # echo 1 > /sys/bus/cdx/devices/.../remove
> > +
> > +What: /sys/bus/cdx/devices/.../resource
> > +Date: July 2023
> > +Contact: puneet.gupta@amd.com
> > +Description:
> > + The resource file contains host addresses of CDX device
> > + resources. Each line of the resource file describes a region
> > + with start, end, and flag fields.
>
> If you documented what this file looked like here, it would be obvious
> that this is not an acceptable sysfs file in any sense of the word.
>
> Please do so, and then fix the patch to not do that at all.
Similar interface exist for pci and we intended to keep it same way. Could you please elaborate on this.
# cat /sys/bus/pci/devices/0000\:01\:00.0/resource
0x0000000092100000 0x00000000921fffff 0x000000000014220c
0x0000000000000000 0x0000000000000000 0x0000000000000000
Thanks,
Abhijit
On Wed, Jul 12, 2023 at 01:23:28PM +0000, Gangurde, Abhijit wrote:
> [AMD Official Use Only - General]
>
> > > Resource files provides the basic MMIO regions info to the
> > > user-space. Also, resources<x> devices can be used to mmap the
> > > MMIO regions in the user-space.
> > >
> > > Co-developed-by: Puneet Gupta <puneet.gupta@amd.com>
> > > Signed-off-by: Puneet Gupta <puneet.gupta@amd.com>
> > > Co-developed-by: Nipun Gupta <nipun.gupta@amd.com>
> > > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> > > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> > > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> > vuuren@amd.com>
> > > Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> > > ---
> > > Documentation/ABI/testing/sysfs-bus-cdx | 15 +++
> > > drivers/cdx/cdx.c | 139 +++++++++++++++++++++++-
> > > include/linux/cdx/cdx_bus.h | 10 ++
> > > 3 files changed, 163 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx
> > b/Documentation/ABI/testing/sysfs-bus-cdx
> > > index d9e00058471d..6ca47b6442ce 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-cdx
> > > +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> > > @@ -76,3 +76,18 @@ Description:
> > > For example::
> > >
> > > # echo 1 > /sys/bus/cdx/devices/.../remove
> > > +
> > > +What: /sys/bus/cdx/devices/.../resource
> > > +Date: July 2023
> > > +Contact: puneet.gupta@amd.com
> > > +Description:
> > > + The resource file contains host addresses of CDX device
> > > + resources. Each line of the resource file describes a region
> > > + with start, end, and flag fields.
> >
> > If you documented what this file looked like here, it would be obvious
> > that this is not an acceptable sysfs file in any sense of the word.
> >
> > Please do so, and then fix the patch to not do that at all.
>
> Similar interface exist for pci and we intended to keep it same way. Could you please elaborate on this.
>
> # cat /sys/bus/pci/devices/0000\:01\:00.0/resource
> 0x0000000092100000 0x00000000921fffff 0x000000000014220c
> 0x0000000000000000 0x0000000000000000 0x0000000000000000
Please don't propagate incorrect decisions in the past.
Why do you need all of these "resources" in userspace? What tool is
going to read and parse them and do something with them?
This really violates the "one value per file" sysfs rule, you are going
to have to have a huge reason why this is not applicable here, AND you
are going to have to document it very very well and get everyone to
agree with it.
thanks,
greg k-h
[AMD Official Use Only - General]
> > > > Resource files provides the basic MMIO regions info to the
> > > > user-space. Also, resources<x> devices can be used to mmap the
> > > > MMIO regions in the user-space.
> > > >
> > > > Co-developed-by: Puneet Gupta <puneet.gupta@amd.com>
> > > > Signed-off-by: Puneet Gupta <puneet.gupta@amd.com>
> > > > Co-developed-by: Nipun Gupta <nipun.gupta@amd.com>
> > > > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> > > > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> > > > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> > > vuuren@amd.com>
> > > > Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> > > > ---
> > > > Documentation/ABI/testing/sysfs-bus-cdx | 15 +++
> > > > drivers/cdx/cdx.c | 139 +++++++++++++++++++++++-
> > > > include/linux/cdx/cdx_bus.h | 10 ++
> > > > 3 files changed, 163 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx
> > > b/Documentation/ABI/testing/sysfs-bus-cdx
> > > > index d9e00058471d..6ca47b6442ce 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-cdx
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> > > > @@ -76,3 +76,18 @@ Description:
> > > > For example::
> > > >
> > > > # echo 1 > /sys/bus/cdx/devices/.../remove
> > > > +
> > > > +What: /sys/bus/cdx/devices/.../resource
> > > > +Date: July 2023
> > > > +Contact: puneet.gupta@amd.com
> > > > +Description:
> > > > + The resource file contains host addresses of CDX device
> > > > + resources. Each line of the resource file describes a region
> > > > + with start, end, and flag fields.
> > >
> > > If you documented what this file looked like here, it would be obvious
> > > that this is not an acceptable sysfs file in any sense of the word.
> > >
> > > Please do so, and then fix the patch to not do that at all.
> >
> > Similar interface exist for pci and we intended to keep it same way. Could you
> please elaborate on this.
> >
> > # cat /sys/bus/pci/devices/0000\:01\:00.0/resource
> > 0x0000000092100000 0x00000000921fffff 0x000000000014220c
> > 0x0000000000000000 0x0000000000000000 0x0000000000000000
>
> Please don't propagate incorrect decisions in the past.
>
> Why do you need all of these "resources" in userspace? What tool is
> going to read and parse them and do something with them?
>
> This really violates the "one value per file" sysfs rule, you are going
> to have to have a huge reason why this is not applicable here, AND you
> are going to have to document it very very well and get everyone to
> agree with it.
We don't have any strong reason apart from that this is being used by some
test and debug applications. Will drop this one now and revisit later by
complying to the specifications.
Thanks,
Abhijit
On Thu, Jul 13, 2023 at 05:36:25AM +0000, Gangurde, Abhijit wrote:
> [AMD Official Use Only - General]
>
> > > > > Resource files provides the basic MMIO regions info to the
> > > > > user-space. Also, resources<x> devices can be used to mmap the
> > > > > MMIO regions in the user-space.
> > > > >
> > > > > Co-developed-by: Puneet Gupta <puneet.gupta@amd.com>
> > > > > Signed-off-by: Puneet Gupta <puneet.gupta@amd.com>
> > > > > Co-developed-by: Nipun Gupta <nipun.gupta@amd.com>
> > > > > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> > > > > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> > > > > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> > > > vuuren@amd.com>
> > > > > Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> > > > > ---
> > > > > Documentation/ABI/testing/sysfs-bus-cdx | 15 +++
> > > > > drivers/cdx/cdx.c | 139 +++++++++++++++++++++++-
> > > > > include/linux/cdx/cdx_bus.h | 10 ++
> > > > > 3 files changed, 163 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx
> > > > b/Documentation/ABI/testing/sysfs-bus-cdx
> > > > > index d9e00058471d..6ca47b6442ce 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-bus-cdx
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> > > > > @@ -76,3 +76,18 @@ Description:
> > > > > For example::
> > > > >
> > > > > # echo 1 > /sys/bus/cdx/devices/.../remove
> > > > > +
> > > > > +What: /sys/bus/cdx/devices/.../resource
> > > > > +Date: July 2023
> > > > > +Contact: puneet.gupta@amd.com
> > > > > +Description:
> > > > > + The resource file contains host addresses of CDX device
> > > > > + resources. Each line of the resource file describes a region
> > > > > + with start, end, and flag fields.
> > > >
> > > > If you documented what this file looked like here, it would be obvious
> > > > that this is not an acceptable sysfs file in any sense of the word.
> > > >
> > > > Please do so, and then fix the patch to not do that at all.
> > >
> > > Similar interface exist for pci and we intended to keep it same way. Could you
> > please elaborate on this.
> > >
> > > # cat /sys/bus/pci/devices/0000\:01\:00.0/resource
> > > 0x0000000092100000 0x00000000921fffff 0x000000000014220c
> > > 0x0000000000000000 0x0000000000000000 0x0000000000000000
> >
> > Please don't propagate incorrect decisions in the past.
> >
> > Why do you need all of these "resources" in userspace? What tool is
> > going to read and parse them and do something with them?
> >
> > This really violates the "one value per file" sysfs rule, you are going
> > to have to have a huge reason why this is not applicable here, AND you
> > are going to have to document it very very well and get everyone to
> > agree with it.
>
> We don't have any strong reason apart from that this is being used by some
> test and debug applications. Will drop this one now and revisit later by
> complying to the specifications.
If it's only for debug stuff, then use debugfs, that's what it is there
for.
thanks,
greg k-h
@@ -76,3 +76,18 @@ Description:
For example::
# echo 1 > /sys/bus/cdx/devices/.../remove
+
+What: /sys/bus/cdx/devices/.../resource
+Date: July 2023
+Contact: puneet.gupta@amd.com
+Description:
+ The resource file contains host addresses of CDX device
+ resources. Each line of the resource file describes a region
+ with start, end, and flag fields.
+
+What: /sys/bus/cdx/devices/.../resource<N>
+Date: July 2023
+Contact: puneet.gupta@amd.com
+Description:
+ The resource binary file contains the content of the memory
+ regions. These files can be m'maped from userspace.
@@ -73,6 +73,8 @@
/* CDX controllers registered with the CDX bus */
static DEFINE_XARRAY_ALLOC(cdx_controllers);
+static void cdx_destroy_res_attr(struct cdx_device *cdx_dev, int num);
+
/**
* cdx_dev_reset - Reset a CDX device
* @dev: CDX device
@@ -126,6 +128,8 @@ static int cdx_unregister_device(struct device *dev,
{
struct cdx_device *cdx_dev = to_cdx_device(dev);
+ cdx_destroy_res_attr(cdx_dev, MAX_CDX_DEV_RESOURCES);
+
kfree(cdx_dev->driver_override);
cdx_dev->driver_override = NULL;
/*
@@ -375,12 +379,32 @@ static ssize_t driver_override_show(struct device *dev,
}
static DEVICE_ATTR_RW(driver_override);
+static ssize_t resource_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct cdx_device *cdx_dev = to_cdx_device(dev);
+ size_t len = 0;
+ int i;
+
+ for (i = 0; i < MAX_CDX_DEV_RESOURCES; i++) {
+ struct resource *res = &cdx_dev->res[i];
+
+ len += sysfs_emit_at(buf, len, "0x%016llx 0x%016llx 0x%016llx\n",
+ (unsigned long long)res->start,
+ (unsigned long long)res->end,
+ (unsigned long long)res->flags);
+ }
+
+ return len;
+}
+static DEVICE_ATTR_RO(resource);
+
static struct attribute *cdx_dev_attrs[] = {
&dev_attr_remove.attr,
&dev_attr_reset.attr,
&dev_attr_vendor.attr,
&dev_attr_device.attr,
&dev_attr_driver_override.attr,
+ &dev_attr_resource.attr,
NULL,
};
ATTRIBUTE_GROUPS(cdx_dev);
@@ -514,12 +538,106 @@ static void cdx_device_release(struct device *dev)
kfree(cdx_dev);
}
+static const struct vm_operations_struct cdx_phys_vm_ops = {
+#ifdef CONFIG_HAVE_IOREMAP_PROT
+ .access = generic_access_phys,
+#endif
+};
+
+/**
+ * cdx_mmap_resource - map a CDX resource into user memory space
+ * @fp: File pointer. Not used in this function, but required where
+ * this API is registered as a callback.
+ * @kobj: kobject for mapping
+ * @attr: struct bin_attribute for the file being mapped
+ * @vma: struct vm_area_struct passed into the mmap
+ *
+ * Use the regular CDX mapping routines to map a CDX resource into userspace.
+ *
+ * Return: true on success, false otherwise.
+ */
+static int cdx_mmap_resource(struct file *fp, struct kobject *kobj,
+ struct bin_attribute *attr,
+ struct vm_area_struct *vma)
+{
+ struct cdx_device *cdx_dev = to_cdx_device(kobj_to_dev(kobj));
+ int num = (unsigned long)attr->private;
+ struct resource *res;
+ unsigned long size;
+
+ res = &cdx_dev->res[num];
+ if (iomem_is_exclusive(res->start))
+ return -EINVAL;
+
+ /* Make sure the caller is mapping a valid resource for this device */
+ size = ((cdx_resource_len(cdx_dev, num) - 1) >> PAGE_SHIFT) + 1;
+ if (vma->vm_pgoff + vma_pages(vma) > size)
+ return -EINVAL;
+
+ /*
+ * Map memory region and vm->vm_pgoff is expected to be an
+ * offset within that region.
+ */
+ vma->vm_page_prot = pgprot_device(vma->vm_page_prot);
+ vma->vm_pgoff += (cdx_resource_start(cdx_dev, num) >> PAGE_SHIFT);
+ vma->vm_ops = &cdx_phys_vm_ops;
+ return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
+ vma->vm_end - vma->vm_start,
+ vma->vm_page_prot);
+}
+
+static void cdx_destroy_res_attr(struct cdx_device *cdx_dev, int num)
+{
+ int i;
+
+ /* removing the bin attributes */
+ for (i = 0; i < num; i++) {
+ struct bin_attribute *res_attr;
+
+ res_attr = cdx_dev->res_attr[i];
+ if (res_attr) {
+ sysfs_remove_bin_file(&cdx_dev->dev.kobj, res_attr);
+ kfree(res_attr);
+ }
+ }
+}
+
+#define CDX_RES_ATTR_NAME_LEN 10
+static int cdx_create_res_attr(struct cdx_device *cdx_dev, int num)
+{
+ struct bin_attribute *res_attr;
+ char *res_attr_name;
+ int ret;
+
+ res_attr = kzalloc(sizeof(*res_attr) + CDX_RES_ATTR_NAME_LEN, GFP_ATOMIC);
+ if (!res_attr)
+ return -ENOMEM;
+
+ res_attr_name = (char *)(res_attr + 1);
+
+ sysfs_bin_attr_init(res_attr);
+
+ cdx_dev->res_attr[num] = res_attr;
+ sprintf(res_attr_name, "resource%d", num);
+
+ res_attr->mmap = cdx_mmap_resource;
+ res_attr->attr.name = res_attr_name;
+ res_attr->attr.mode = 0600;
+ res_attr->size = cdx_resource_len(cdx_dev, num);
+ res_attr->private = (void *)(unsigned long)num;
+ ret = sysfs_create_bin_file(&cdx_dev->dev.kobj, res_attr);
+ if (ret)
+ kfree(res_attr);
+
+ return ret;
+}
+
int cdx_device_add(struct cdx_dev_params *dev_params)
{
struct cdx_controller *cdx = dev_params->cdx;
struct device *parent = cdx->dev;
struct cdx_device *cdx_dev;
- int ret;
+ int ret, i;
cdx_dev = kzalloc(sizeof(*cdx_dev), GFP_KERNEL);
if (!cdx_dev)
@@ -558,7 +676,26 @@ int cdx_device_add(struct cdx_dev_params *dev_params)
goto fail;
}
+ /* Create resource<N> attributes */
+ for (i = 0; i < MAX_CDX_DEV_RESOURCES; i++) {
+ if (cdx_resource_flags(cdx_dev, i) & IORESOURCE_MEM) {
+ /* skip empty resources */
+ if (!cdx_resource_len(cdx_dev, i))
+ continue;
+
+ ret = cdx_create_res_attr(cdx_dev, i);
+ if (ret != 0) {
+ dev_err(&cdx_dev->dev,
+ "cdx device resource<%d> file creation failed: %d", i, ret);
+ goto fail1;
+ }
+ }
+ }
+
return 0;
+fail1:
+ cdx_destroy_res_attr(cdx_dev, i);
+ device_del(&cdx_dev->dev);
fail:
/*
* Do not free cdx_dev here as it would be freed in
@@ -104,6 +104,7 @@ struct cdx_device {
u8 bus_num;
u8 dev_num;
struct resource res[MAX_CDX_DEV_RESOURCES];
+ struct bin_attribute *res_attr[MAX_CDX_DEV_RESOURCES];
u8 res_count;
u64 dma_mask;
u16 flags;
@@ -114,6 +115,15 @@ struct cdx_device {
#define to_cdx_device(_dev) \
container_of(_dev, struct cdx_device, dev)
+#define cdx_resource_start(dev, num) ((dev)->res[(num)].start)
+#define cdx_resource_end(dev, num) ((dev)->res[(num)].end)
+#define cdx_resource_flags(dev, num) ((dev)->res[(num)].flags)
+#define cdx_resource_len(dev, num) \
+ ((cdx_resource_start((dev), (num)) == 0 && \
+ cdx_resource_end((dev), (num)) == \
+ cdx_resource_start((dev), (num))) ? 0 : \
+ (cdx_resource_end((dev), (num)) - \
+ cdx_resource_start((dev), (num)) + 1))
/**
* struct cdx_driver - CDX device driver
* @driver: Generic device driver