[PATCH 0/3] cxl: Provide user-space r/o access to the AFU descriptor

[PATCH 0/3] cxl: Provide user-space r/o access to the AFU descriptor

[Vaibhav Jain]

Hi,

This patch-set provides a fix for libcxl github issue#20 
"libcxl doesn't handle a mmio read of 0xfffffff... (all 1's) correctly" at
https://github.com/ibm-capi/libcxl/issues/20.

The issue arises as libcxl uses mmio read values from problem-state-area(PSA) to
determine if the card had fenced or not. When it reads a value of 0xFFs (all 1)
it assumes that the card has fenced and returns an error for the mmio read.
Unfortunately 0xFFs can be a valid value in PSA of some some AFUs and in such
cases libcxl incorrectly assumes the card to be fenced and indicate and error to
the caller.

To fix this issue, the patch-set provides direct and read-only access of the afu
descriptor to libcxl via a binary sysfs attribute named 'afu_desc'. Libcxl can
mmap this attribute to process address space and if an mmio read returns 0xFFs
it tries reading contents of afu_desc at an offset whose contents != ~0x0ULL. If
the card is fenced read of the same offset will have contents == ~0x0ULL in
which case libcxl can indicate an error to the caller about the fenced card else
the read of 0xFFs is a valid read.


There are three patchset in this series:

- First patch refactors the function cxl_pci_afu_err_buffer() and moves its
  implementation to 'native.c'. It also moves the core logic of the function
  to a new function __aligned_memcpy().

- Second patch introduces a new sysfs attribute named afu_desc to be used by
  libcxl to read raw contents of afu descriptor.

- Third patch provides native implementations for new cxl backend-ops introduced
  in the previous patch. Most importantly it implements the afu_desc_mmap 
  backend-op thats mmaps the afu descriptor to a given vma.

Additional related changes apart from this patch-set:

- Corresponding libcxl changes are posted as a pull request#25 
  "libcxl: Check afu link when read from PSA mmio return all FFs" at
  https://github.com/ibm-capi/libcxl/pull/25

- Patch "kernfs: Check KERNFS_HAS_RELEASE before calling kernfs_release_file()"
  which fixes a kernel oops occurring while removing a bin_attribute from sysfs
  at https://patchwork.ozlabs.org/patch/738536/.

Vaibhav Jain (3):
  cxl: Re-factor cxl_pci_afu_read_err_buffer()
  cxl: Introduce afu_desc sysfs attribute
  cxl: Provide user-space access to afu descriptor on bare metal

 Documentation/ABI/testing/sysfs-class-cxl |  9 ++++
 drivers/misc/cxl/cxl.h                    | 15 ++++--
 drivers/misc/cxl/native.c                 | 89 ++++++++++++++++++++++++++++++-
 drivers/misc/cxl/pci.c                    | 47 ++--------------
 drivers/misc/cxl/sysfs.c                  | 45 ++++++++++++++++
 5 files changed, 157 insertions(+), 48 deletions(-)

-- 
2.9.3

[PATCH 1/3] cxl: Re-factor cxl_pci_afu_read_err_buffer()

[Vaibhav Jain]

This patch moves,renames and re-factors the function
afu_pci_afu_err_buffer(). The function is now moved to native.c from
pci.c and renamed as native_afu_read_err_buffer().

Also the ability of copying data from h/w enforcing 4/8 byte aligned
access is useful and better shared across other functions. So this
patch moves the core logic of existing cxl_pci_afu_read_err_buffer()
to a new function named __aligned_memcpy().The new implementation of
native_afu_read_err_buffer() is simply a call to __aligned_memcpy()
with appropriate actual parameters.

Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
---
 drivers/misc/cxl/cxl.h    |  3 ---
 drivers/misc/cxl/native.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/misc/cxl/pci.c    | 44 -------------------------------------
 3 files changed, 55 insertions(+), 48 deletions(-)

diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 79e60ec..ef683b7 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -739,9 +739,6 @@ static inline u64 cxl_p2n_read(struct cxl_afu *afu, cxl_p2n_reg_t reg)
 		return ~0ULL;
 }
 
-ssize_t cxl_pci_afu_read_err_buffer(struct cxl_afu *afu, char *buf,
-				loff_t off, size_t count);
-
 /* Internal functions wrapped in cxl_base to allow PHB to call them */
 bool _cxl_pci_associate_default_context(struct pci_dev *dev, struct cxl_afu *afu);
 void _cxl_pci_disable_device(struct pci_dev *dev);
diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
index 7ae7105..20d3df6 100644
--- a/drivers/misc/cxl/native.c
+++ b/drivers/misc/cxl/native.c
@@ -1276,6 +1276,60 @@ static int native_afu_cr_write8(struct cxl_afu *afu, int cr, u64 off, u8 in)
 	return rc;
 }
 
+#define ERR_BUFF_MAX_COPY_SIZE PAGE_SIZE
+
+/*
+ * __aligned_memcpy:
+ * Copies count or max_read bytes (whichever is smaller) from src to dst buffer
+ * starting at offset off in src buffer. This specialized implementation of
+ * memcpy_fromio is needed as capi h/w only supports 4/8 bytes aligned access.
+ * So in case the requested offset/count arent 8 byte aligned the function uses
+ * a bounce buffer which can be max ERR_BUFF_MAX_COPY_SIZE == PAGE_SIZE
+ */
+static ssize_t __aligned_memcpy(void *dst, void __iomem *src, loff_t off,
+			       size_t count, size_t max_read)
+{
+	loff_t aligned_start, aligned_end;
+	size_t aligned_length;
+	void *tbuf;
+
+	if (count == 0 || off < 0 || (size_t)off >= max_read)
+		return 0;
+
+	/* calculate aligned read window */
+	count = min((size_t)(max_read - off), count);
+	aligned_start = round_down(off, 8);
+	aligned_end = round_up(off + count, 8);
+	aligned_length = aligned_end - aligned_start;
+
+	/* max we can copy in one read is PAGE_SIZE */
+	if (aligned_length > ERR_BUFF_MAX_COPY_SIZE) {
+		aligned_length = ERR_BUFF_MAX_COPY_SIZE;
+		count = ERR_BUFF_MAX_COPY_SIZE - (off & 0x7);
+	}
+
+	/* use bounce buffer for copy */
+	tbuf = (void *)__get_free_page(GFP_TEMPORARY);
+	if (!tbuf)
+		return -ENOMEM;
+
+	/* perform aligned read from the mmio region */
+	memcpy_fromio(tbuf, src + aligned_start, aligned_length);
+	memcpy(dst, tbuf + (off & 0x7), count);
+
+	free_page((unsigned long)tbuf);
+
+	return count;
+}
+
+static ssize_t native_afu_read_err_buffer(struct cxl_afu *afu, char *buf,
+				loff_t off, size_t count)
+{
+	void __iomem *ebuf = afu->native->afu_desc_mmio + afu->eb_offset;
+
+	return __aligned_memcpy(buf, ebuf, off, count, afu->eb_len);
+}
+
 const struct cxl_backend_ops cxl_native_ops = {
 	.module = THIS_MODULE,
 	.adapter_reset = cxl_pci_reset,
@@ -1294,7 +1348,7 @@ const struct cxl_backend_ops cxl_native_ops = {
 	.support_attributes = native_support_attributes,
 	.link_ok = cxl_adapter_link_ok,
 	.release_afu = cxl_pci_release_afu,
-	.afu_read_err_buffer = cxl_pci_afu_read_err_buffer,
+	.afu_read_err_buffer = native_afu_read_err_buffer,
 	.afu_check_and_enable = native_afu_check_and_enable,
 	.afu_activate_mode = native_afu_activate_mode,
 	.afu_deactivate_mode = native_afu_deactivate_mode,
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 91f6459..541dc9a 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -1051,50 +1051,6 @@ static int sanitise_afu_regs(struct cxl_afu *afu)
 	return 0;
 }
 
-#define ERR_BUFF_MAX_COPY_SIZE PAGE_SIZE
-/*
- * afu_eb_read:
- * Called from sysfs and reads the afu error info buffer. The h/w only supports
- * 4/8 bytes aligned access. So in case the requested offset/count arent 8 byte
- * aligned the function uses a bounce buffer which can be max PAGE_SIZE.
- */
-ssize_t cxl_pci_afu_read_err_buffer(struct cxl_afu *afu, char *buf,
-				loff_t off, size_t count)
-{
-	loff_t aligned_start, aligned_end;
-	size_t aligned_length;
-	void *tbuf;
-	const void __iomem *ebuf = afu->native->afu_desc_mmio + afu->eb_offset;
-
-	if (count == 0 || off < 0 || (size_t)off >= afu->eb_len)
-		return 0;
-
-	/* calculate aligned read window */
-	count = min((size_t)(afu->eb_len - off), count);
-	aligned_start = round_down(off, 8);
-	aligned_end = round_up(off + count, 8);
-	aligned_length = aligned_end - aligned_start;
-
-	/* max we can copy in one read is PAGE_SIZE */
-	if (aligned_length > ERR_BUFF_MAX_COPY_SIZE) {
-		aligned_length = ERR_BUFF_MAX_COPY_SIZE;
-		count = ERR_BUFF_MAX_COPY_SIZE - (off & 0x7);
-	}
-
-	/* use bounce buffer for copy */
-	tbuf = (void *)__get_free_page(GFP_TEMPORARY);
-	if (!tbuf)
-		return -ENOMEM;
-
-	/* perform aligned read from the mmio region */
-	memcpy_fromio(tbuf, ebuf + aligned_start, aligned_length);
-	memcpy(buf, tbuf + (off & 0x7), count);
-
-	free_page((unsigned long)tbuf);
-
-	return count;
-}
-
 static int pci_configure_afu(struct cxl_afu *afu, struct cxl *adapter, struct pci_dev *dev)
 {
 	int rc;
-- 
2.9.3

[PATCH 2/3] cxl: Introduce afu_desc sysfs attribute

[Vaibhav Jain]

This patch introduces a new afu sysfs attribute named afu_desc. This
binary attribute provides access to raw contents of the afu descriptor
to user-space. Direct access to afu descriptor is useful for libcxl
that can use it to determine if the CXL card has been fenced or
provide application access to afu attributes beyond one defined in
CAIA.

We introduce three new backend-ops:

* afu_desc_size(): Return the size in bytes of the afu descriptor.

* afu_desc_read(): Copy into a provided buffer contents of afu
  descriptor starting at specific offset.

* afu_desc_mmap(): Memory map the afu descriptor to the given
  vm_area_struct.

If afu_desc_size() > 0 the afu_desc attribute gets created for the AFU.
The bin_attribute callbacks route the calls to corresponding cxl backend
implementation.

Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
---
 Documentation/ABI/testing/sysfs-class-cxl |  9 +++++++
 drivers/misc/cxl/cxl.h                    |  9 +++++++
 drivers/misc/cxl/sysfs.c                  | 45 +++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-cxl b/Documentation/ABI/testing/sysfs-class-cxl
index 640f65e..9ac84c4 100644
--- a/Documentation/ABI/testing/sysfs-class-cxl
+++ b/Documentation/ABI/testing/sysfs-class-cxl
@@ -6,6 +6,15 @@ Example: The real path of the attribute /sys/class/cxl/afu0.0s/irqs_max is
 
 Slave contexts (eg. /sys/class/cxl/afu0.0s):
 
+What:           /sys/class/cxl/<afu>/afu_desc
+Date:           March 2016
+Contact:        linuxppc-dev@lists.ozlabs.org
+Description:    read only
+                AFU Descriptor contents. The contents of this file are
+		binary contents of the AFU descriptor. LIBCXL library can
+		use this file to read afu descriptor and in some special cases
+		determine if the cxl card has been fenced.
+
 What:           /sys/class/cxl/<afu>/afu_err_buf
 Date:           September 2014
 Contact:        linuxppc-dev@lists.ozlabs.org
diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index ef683b7..1c43d06 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -426,6 +426,9 @@ struct cxl_afu {
 	u64 eb_len, eb_offset;
 	struct bin_attribute attr_eb;
 
+	/* Afu descriptor */
+	struct bin_attribute attr_afud;
+
 	/* pointer to the vphb */
 	struct pci_controller *phb;
 
@@ -995,6 +998,12 @@ struct cxl_backend_ops {
 	int (*afu_cr_write16)(struct cxl_afu *afu, int cr_idx, u64 offset, u16 val);
 	int (*afu_cr_write32)(struct cxl_afu *afu, int cr_idx, u64 offset, u32 val);
 	ssize_t (*read_adapter_vpd)(struct cxl *adapter, void *buf, size_t count);
+	/* Access to AFU descriptor */
+	ssize_t (*afu_desc_size)(struct cxl_afu *afu);
+	ssize_t (*afu_desc_read)(struct cxl_afu *afu, char *buf, loff_t off,
+				 size_t count);
+	int (*afu_desc_mmap)(struct cxl_afu *afu, struct file *filp,
+			     struct vm_area_struct *vma);
 };
 extern const struct cxl_backend_ops cxl_native_ops;
 extern const struct cxl_backend_ops cxl_guest_ops;
diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c
index a8b6d6a..fff3468 100644
--- a/drivers/misc/cxl/sysfs.c
+++ b/drivers/misc/cxl/sysfs.c
@@ -426,6 +426,26 @@ static ssize_t afu_eb_read(struct file *filp, struct kobject *kobj,
 	return cxl_ops->afu_read_err_buffer(afu, buf, off, count);
 }
 
+static ssize_t afu_desc_read(struct file *filp, struct kobject *kobj,
+			     struct bin_attribute *bin_attr, char *buf,
+			     loff_t off, size_t count)
+{
+	struct cxl_afu *afu = to_cxl_afu(kobj_to_dev(kobj));
+
+	return cxl_ops->afu_desc_read ?
+		cxl_ops->afu_desc_read(afu, buf, off, count) : -EIO;
+}
+
+static int afu_desc_mmap(struct file *filp, struct kobject *kobj,
+			 struct bin_attribute *attr, struct vm_area_struct *vma)
+{
+	struct cxl_afu *afu = to_cxl_afu(kobj_to_dev(kobj));
+
+	return cxl_ops->afu_desc_mmap ?
+		cxl_ops->afu_desc_mmap(afu, filp, vma) : -EINVAL;
+}
+
+
 static struct device_attribute afu_attrs[] = {
 	__ATTR_RO(mmio_size),
 	__ATTR_RO(irqs_min),
@@ -625,6 +645,9 @@ void cxl_sysfs_afu_remove(struct cxl_afu *afu)
 	struct afu_config_record *cr, *tmp;
 	int i;
 
+	if (afu->attr_afud.size > 0)
+		device_remove_bin_file(&afu->dev, &afu->attr_afud);
+
 	/* remove the err buffer bin attribute */
 	if (afu->eb_len)
 		device_remove_bin_file(&afu->dev, &afu->attr_eb);
@@ -686,6 +709,28 @@ int cxl_sysfs_afu_add(struct cxl_afu *afu)
 		list_add(&cr->list, &afu->crs);
 	}
 
+	/* Create the sysfs binattr for afu-descriptor */
+	afu->attr_afud.size = cxl_ops->afu_desc_size ?
+		cxl_ops->afu_desc_size(afu) : 0;
+
+	if (afu->attr_afud.size > 0) {
+		sysfs_attr_init(&afu->attr_afud.attr);
+		afu->attr_afud.attr.name = "afu_desc";
+		afu->attr_afud.attr.mode = 0444;
+		afu->attr_afud.read = afu_desc_read;
+		afu->attr_afud.mmap = afu_desc_mmap;
+
+		rc = device_create_bin_file(&afu->dev, &afu->attr_afud);
+		if (rc) {
+			/* indicate that we did'nt create the sysfs attr */
+			afu->attr_afud.size = 0;
+			dev_err(&afu->dev,
+				"Unable to create afu_desc attr for the afu. Err(%d)\n",
+				rc);
+			goto err1;
+		}
+	}
+
 	return 0;
 
 err1:
-- 
2.9.3

[PATCH 3/3] cxl: Provide user-space access to afu descriptor on bare metal

[Vaibhav Jain]

This patch implements cxl backend to provide user-space access to
binary afu descriptor contents via sysfs. We add a new member to
struct cxl_afu_native named phy_desc that caches the physical base
address of afu descriptor, which is then used in implementation of new
native cxl backend ops namel:

* native_afu_desc_size()
* native_afu_desc_read()
* native_afu_desc_mmap()

The implementations of all these callbacks is mostly trivial except
native_afu_desc_mmap() which maps the PFNs pointing to afu descriptor
in i/o memory, to user-space vm_area_struct.

Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
---
 drivers/misc/cxl/cxl.h    |  3 +++
 drivers/misc/cxl/native.c | 33 +++++++++++++++++++++++++++++++++
 drivers/misc/cxl/pci.c    |  3 +++
 3 files changed, 39 insertions(+)

diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 1c43d06..c6db1fa 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -386,6 +386,9 @@ struct cxl_afu_native {
 	int spa_order;
 	int spa_max_procs;
 	u64 pp_offset;
+
+	/* Afu descriptor physical address */
+	u64 phy_desc;
 };
 
 struct cxl_afu_guest {
diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
index 20d3df6..44e3e84 100644
--- a/drivers/misc/cxl/native.c
+++ b/drivers/misc/cxl/native.c
@@ -1330,6 +1330,36 @@ static ssize_t native_afu_read_err_buffer(struct cxl_afu *afu, char *buf,
 	return __aligned_memcpy(buf, ebuf, off, count, afu->eb_len);
 }
 
+static ssize_t native_afu_desc_size(struct cxl_afu *afu)
+{
+	return afu->adapter->native->afu_desc_size;
+}
+
+static ssize_t native_afu_desc_read(struct cxl_afu *afu, char *buf, loff_t off,
+				     size_t count)
+{
+	return __aligned_memcpy(buf, afu->native->afu_desc_mmio, off, count,
+				afu->adapter->native->afu_desc_size);
+}
+
+static int native_afu_desc_mmap(struct cxl_afu *afu, struct file *filp,
+		     struct vm_area_struct *vma)
+{
+	u64 len = vma->vm_end - vma->vm_start;
+
+	/* Check the size vma so that it doesn't go beyond afud size */
+	if (len > native_afu_desc_size(afu)) {
+		pr_err("Requested VMA too large. Requested=%lld, Available=%ld\n",
+		       len, native_afu_desc_size(afu));
+		return -EINVAL;
+	}
+
+	vma->vm_flags |= VM_IO | VM_PFNMAP;
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+
+	return vm_iomap_memory(vma, afu->native->phy_desc, len);
+}
+
 const struct cxl_backend_ops cxl_native_ops = {
 	.module = THIS_MODULE,
 	.adapter_reset = cxl_pci_reset,
@@ -1361,4 +1391,7 @@ const struct cxl_backend_ops cxl_native_ops = {
 	.afu_cr_write16 = native_afu_cr_write16,
 	.afu_cr_write32 = native_afu_cr_write32,
 	.read_adapter_vpd = cxl_pci_read_adapter_vpd,
+	.afu_desc_read = native_afu_desc_read,
+	.afu_desc_mmap = native_afu_desc_mmap,
+	.afu_desc_size =  native_afu_desc_size
 };
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 541dc9a..a6166e0 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -869,6 +869,9 @@ static int pci_map_slice_regs(struct cxl_afu *afu, struct cxl *adapter, struct p
 	if (afu_desc) {
 		if (!(afu->native->afu_desc_mmio = ioremap(afu_desc, adapter->native->afu_desc_size)))
 			goto err2;
+
+		/* Cache the afu descriptor physical address */
+		afu->native->phy_desc = afu_desc;
 	}
 
 	return 0;
-- 
2.9.3

Re: [PATCH 1/3] cxl: Re-factor cxl_pci_afu_read_err_buffer()

[Andrew Donnellan]

On 14/03/17 15:06, Vaibhav Jain wrote:
> This patch moves,renames and re-factors the function
> afu_pci_afu_err_buffer(). The function is now moved to native.c from
> pci.c and renamed as native_afu_read_err_buffer().
>
> Also the ability of copying data from h/w enforcing 4/8 byte aligned
> access is useful and better shared across other functions. So this
> patch moves the core logic of existing cxl_pci_afu_read_err_buffer()
> to a new function named __aligned_memcpy().The new implementation of
> native_afu_read_err_buffer() is simply a call to __aligned_memcpy()
> with appropriate actual parameters.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>

Comments below.

Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

> ---
>  drivers/misc/cxl/cxl.h    |  3 ---
>  drivers/misc/cxl/native.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/misc/cxl/pci.c    | 44 -------------------------------------
>  3 files changed, 55 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
> index 79e60ec..ef683b7 100644
> --- a/drivers/misc/cxl/cxl.h
> +++ b/drivers/misc/cxl/cxl.h
> @@ -739,9 +739,6 @@ static inline u64 cxl_p2n_read(struct cxl_afu *afu, cxl_p2n_reg_t reg)
>  		return ~0ULL;
>  }
>
> -ssize_t cxl_pci_afu_read_err_buffer(struct cxl_afu *afu, char *buf,
> -				loff_t off, size_t count);
> -
>  /* Internal functions wrapped in cxl_base to allow PHB to call them */
>  bool _cxl_pci_associate_default_context(struct pci_dev *dev, struct cxl_afu *afu);
>  void _cxl_pci_disable_device(struct pci_dev *dev);
> diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
> index 7ae7105..20d3df6 100644
> --- a/drivers/misc/cxl/native.c
> +++ b/drivers/misc/cxl/native.c
> @@ -1276,6 +1276,60 @@ static int native_afu_cr_write8(struct cxl_afu *afu, int cr, u64 off, u8 in)
>  	return rc;
>  }
>
> +#define ERR_BUFF_MAX_COPY_SIZE PAGE_SIZE
> +
> +/*
> + * __aligned_memcpy:
> + * Copies count or max_read bytes (whichever is smaller) from src to dst buffer
> + * starting at offset off in src buffer. This specialized implementation of
> + * memcpy_fromio is needed as capi h/w only supports 4/8 bytes aligned access.
> + * So in case the requested offset/count arent 8 byte aligned the function uses

aren't

> + * a bounce buffer which can be max ERR_BUFF_MAX_COPY_SIZE == PAGE_SIZE
> + */
> +static ssize_t __aligned_memcpy(void *dst, void __iomem *src, loff_t off,
> +			       size_t count, size_t max_read)
> +{
> +	loff_t aligned_start, aligned_end;
> +	size_t aligned_length;
> +	void *tbuf;
> +
> +	if (count == 0 || off < 0 || (size_t)off >= max_read)
> +		return 0;
> +
> +	/* calculate aligned read window */
> +	count = min((size_t)(max_read - off), count);
> +	aligned_start = round_down(off, 8);
> +	aligned_end = round_up(off + count, 8);
> +	aligned_length = aligned_end - aligned_start;
> +
> +	/* max we can copy in one read is PAGE_SIZE */
> +	if (aligned_length > ERR_BUFF_MAX_COPY_SIZE) {

I'm not sure if the name ERR_BUFF_MAX_COPY_SIZE makes sense here any more.

> +		aligned_length = ERR_BUFF_MAX_COPY_SIZE;
> +		count = ERR_BUFF_MAX_COPY_SIZE - (off & 0x7);
> +	}
> +
> +	/* use bounce buffer for copy */
> +	tbuf = (void *)__get_free_page(GFP_TEMPORARY);
> +	if (!tbuf)
> +		return -ENOMEM;
> +
> +	/* perform aligned read from the mmio region */
> +	memcpy_fromio(tbuf, src + aligned_start, aligned_length);
> +	memcpy(dst, tbuf + (off & 0x7), count);
> +
> +	free_page((unsigned long)tbuf);
> +
> +	return count;
> +}
> +
> +static ssize_t native_afu_read_err_buffer(struct cxl_afu *afu, char *buf,
> +				loff_t off, size_t count)
> +{
> +	void __iomem *ebuf = afu->native->afu_desc_mmio + afu->eb_offset;
> +
> +	return __aligned_memcpy(buf, ebuf, off, count, afu->eb_len);
> +}
> +
>  const struct cxl_backend_ops cxl_native_ops = {
>  	.module = THIS_MODULE,
>  	.adapter_reset = cxl_pci_reset,
> @@ -1294,7 +1348,7 @@ const struct cxl_backend_ops cxl_native_ops = {
>  	.support_attributes = native_support_attributes,
>  	.link_ok = cxl_adapter_link_ok,
>  	.release_afu = cxl_pci_release_afu,
> -	.afu_read_err_buffer = cxl_pci_afu_read_err_buffer,
> +	.afu_read_err_buffer = native_afu_read_err_buffer,
>  	.afu_check_and_enable = native_afu_check_and_enable,
>  	.afu_activate_mode = native_afu_activate_mode,
>  	.afu_deactivate_mode = native_afu_deactivate_mode,
> diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
> index 91f6459..541dc9a 100644
> --- a/drivers/misc/cxl/pci.c
> +++ b/drivers/misc/cxl/pci.c
> @@ -1051,50 +1051,6 @@ static int sanitise_afu_regs(struct cxl_afu *afu)
>  	return 0;
>  }
>
> -#define ERR_BUFF_MAX_COPY_SIZE PAGE_SIZE
> -/*
> - * afu_eb_read:
> - * Called from sysfs and reads the afu error info buffer. The h/w only supports
> - * 4/8 bytes aligned access. So in case the requested offset/count arent 8 byte
> - * aligned the function uses a bounce buffer which can be max PAGE_SIZE.
> - */
> -ssize_t cxl_pci_afu_read_err_buffer(struct cxl_afu *afu, char *buf,
> -				loff_t off, size_t count)
> -{
> -	loff_t aligned_start, aligned_end;
> -	size_t aligned_length;
> -	void *tbuf;
> -	const void __iomem *ebuf = afu->native->afu_desc_mmio + afu->eb_offset;
> -
> -	if (count == 0 || off < 0 || (size_t)off >= afu->eb_len)
> -		return 0;
> -
> -	/* calculate aligned read window */
> -	count = min((size_t)(afu->eb_len - off), count);
> -	aligned_start = round_down(off, 8);
> -	aligned_end = round_up(off + count, 8);
> -	aligned_length = aligned_end - aligned_start;
> -
> -	/* max we can copy in one read is PAGE_SIZE */
> -	if (aligned_length > ERR_BUFF_MAX_COPY_SIZE) {
> -		aligned_length = ERR_BUFF_MAX_COPY_SIZE;
> -		count = ERR_BUFF_MAX_COPY_SIZE - (off & 0x7);
> -	}
> -
> -	/* use bounce buffer for copy */
> -	tbuf = (void *)__get_free_page(GFP_TEMPORARY);
> -	if (!tbuf)
> -		return -ENOMEM;
> -
> -	/* perform aligned read from the mmio region */
> -	memcpy_fromio(tbuf, ebuf + aligned_start, aligned_length);
> -	memcpy(buf, tbuf + (off & 0x7), count);
> -
> -	free_page((unsigned long)tbuf);
> -
> -	return count;
> -}
> -
>  static int pci_configure_afu(struct cxl_afu *afu, struct cxl *adapter, struct pci_dev *dev)
>  {
>  	int rc;
>

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH 1/3] cxl: Re-factor cxl_pci_afu_read_err_buffer()

[Frederic Barrat]

Le 14/03/2017 à 05:06, Vaibhav Jain a écrit :
> This patch moves,renames and re-factors the function
> afu_pci_afu_err_buffer(). The function is now moved to native.c from
> pci.c and renamed as native_afu_read_err_buffer().
>
> Also the ability of copying data from h/w enforcing 4/8 byte aligned
> access is useful and better shared across other functions. So this
> patch moves the core logic of existing cxl_pci_afu_read_err_buffer()
> to a new function named __aligned_memcpy().The new implementation of
> native_afu_read_err_buffer() is simply a call to __aligned_memcpy()
> with appropriate actual parameters.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
> ---

Acked-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com>

>  drivers/misc/cxl/cxl.h    |  3 ---
>  drivers/misc/cxl/native.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/misc/cxl/pci.c    | 44 -------------------------------------
>  3 files changed, 55 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
> index 79e60ec..ef683b7 100644
> --- a/drivers/misc/cxl/cxl.h
> +++ b/drivers/misc/cxl/cxl.h
> @@ -739,9 +739,6 @@ static inline u64 cxl_p2n_read(struct cxl_afu *afu, cxl_p2n_reg_t reg)
>  		return ~0ULL;
>  }
>
> -ssize_t cxl_pci_afu_read_err_buffer(struct cxl_afu *afu, char *buf,
> -				loff_t off, size_t count);
> -
>  /* Internal functions wrapped in cxl_base to allow PHB to call them */
>  bool _cxl_pci_associate_default_context(struct pci_dev *dev, struct cxl_afu *afu);
>  void _cxl_pci_disable_device(struct pci_dev *dev);
> diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
> index 7ae7105..20d3df6 100644
> --- a/drivers/misc/cxl/native.c
> +++ b/drivers/misc/cxl/native.c
> @@ -1276,6 +1276,60 @@ static int native_afu_cr_write8(struct cxl_afu *afu, int cr, u64 off, u8 in)
>  	return rc;
>  }
>
> +#define ERR_BUFF_MAX_COPY_SIZE PAGE_SIZE
> +
> +/*
> + * __aligned_memcpy:
> + * Copies count or max_read bytes (whichever is smaller) from src to dst buffer
> + * starting at offset off in src buffer. This specialized implementation of
> + * memcpy_fromio is needed as capi h/w only supports 4/8 bytes aligned access.
> + * So in case the requested offset/count arent 8 byte aligned the function uses
> + * a bounce buffer which can be max ERR_BUFF_MAX_COPY_SIZE == PAGE_SIZE
> + */
> +static ssize_t __aligned_memcpy(void *dst, void __iomem *src, loff_t off,
> +			       size_t count, size_t max_read)
> +{
> +	loff_t aligned_start, aligned_end;
> +	size_t aligned_length;
> +	void *tbuf;
> +
> +	if (count == 0 || off < 0 || (size_t)off >= max_read)
> +		return 0;
> +
> +	/* calculate aligned read window */
> +	count = min((size_t)(max_read - off), count);
> +	aligned_start = round_down(off, 8);
> +	aligned_end = round_up(off + count, 8);
> +	aligned_length = aligned_end - aligned_start;
> +
> +	/* max we can copy in one read is PAGE_SIZE */
> +	if (aligned_length > ERR_BUFF_MAX_COPY_SIZE) {
> +		aligned_length = ERR_BUFF_MAX_COPY_SIZE;
> +		count = ERR_BUFF_MAX_COPY_SIZE - (off & 0x7);
> +	}
> +
> +	/* use bounce buffer for copy */
> +	tbuf = (void *)__get_free_page(GFP_TEMPORARY);
> +	if (!tbuf)
> +		return -ENOMEM;
> +
> +	/* perform aligned read from the mmio region */
> +	memcpy_fromio(tbuf, src + aligned_start, aligned_length);
> +	memcpy(dst, tbuf + (off & 0x7), count);
> +
> +	free_page((unsigned long)tbuf);
> +
> +	return count;
> +}
> +
> +static ssize_t native_afu_read_err_buffer(struct cxl_afu *afu, char *buf,
> +				loff_t off, size_t count)
> +{
> +	void __iomem *ebuf = afu->native->afu_desc_mmio + afu->eb_offset;
> +
> +	return __aligned_memcpy(buf, ebuf, off, count, afu->eb_len);
> +}
> +
>  const struct cxl_backend_ops cxl_native_ops = {
>  	.module = THIS_MODULE,
>  	.adapter_reset = cxl_pci_reset,
> @@ -1294,7 +1348,7 @@ const struct cxl_backend_ops cxl_native_ops = {
>  	.support_attributes = native_support_attributes,
>  	.link_ok = cxl_adapter_link_ok,
>  	.release_afu = cxl_pci_release_afu,
> -	.afu_read_err_buffer = cxl_pci_afu_read_err_buffer,
> +	.afu_read_err_buffer = native_afu_read_err_buffer,
>  	.afu_check_and_enable = native_afu_check_and_enable,
>  	.afu_activate_mode = native_afu_activate_mode,
>  	.afu_deactivate_mode = native_afu_deactivate_mode,
> diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
> index 91f6459..541dc9a 100644
> --- a/drivers/misc/cxl/pci.c
> +++ b/drivers/misc/cxl/pci.c
> @@ -1051,50 +1051,6 @@ static int sanitise_afu_regs(struct cxl_afu *afu)
>  	return 0;
>  }
>
> -#define ERR_BUFF_MAX_COPY_SIZE PAGE_SIZE
> -/*
> - * afu_eb_read:
> - * Called from sysfs and reads the afu error info buffer. The h/w only supports
> - * 4/8 bytes aligned access. So in case the requested offset/count arent 8 byte
> - * aligned the function uses a bounce buffer which can be max PAGE_SIZE.
> - */
> -ssize_t cxl_pci_afu_read_err_buffer(struct cxl_afu *afu, char *buf,
> -				loff_t off, size_t count)
> -{
> -	loff_t aligned_start, aligned_end;
> -	size_t aligned_length;
> -	void *tbuf;
> -	const void __iomem *ebuf = afu->native->afu_desc_mmio + afu->eb_offset;
> -
> -	if (count == 0 || off < 0 || (size_t)off >= afu->eb_len)
> -		return 0;
> -
> -	/* calculate aligned read window */
> -	count = min((size_t)(afu->eb_len - off), count);
> -	aligned_start = round_down(off, 8);
> -	aligned_end = round_up(off + count, 8);
> -	aligned_length = aligned_end - aligned_start;
> -
> -	/* max we can copy in one read is PAGE_SIZE */
> -	if (aligned_length > ERR_BUFF_MAX_COPY_SIZE) {
> -		aligned_length = ERR_BUFF_MAX_COPY_SIZE;
> -		count = ERR_BUFF_MAX_COPY_SIZE - (off & 0x7);
> -	}
> -
> -	/* use bounce buffer for copy */
> -	tbuf = (void *)__get_free_page(GFP_TEMPORARY);
> -	if (!tbuf)
> -		return -ENOMEM;
> -
> -	/* perform aligned read from the mmio region */
> -	memcpy_fromio(tbuf, ebuf + aligned_start, aligned_length);
> -	memcpy(buf, tbuf + (off & 0x7), count);
> -
> -	free_page((unsigned long)tbuf);
> -
> -	return count;
> -}
> -
>  static int pci_configure_afu(struct cxl_afu *afu, struct cxl *adapter, struct pci_dev *dev)
>  {
>  	int rc;
>

Re: [PATCH 2/3] cxl: Introduce afu_desc sysfs attribute

[Frederic Barrat]

Hi Vaibhav,

There's one thing bugging me here, see below


Le 14/03/2017 à 05:06, Vaibhav Jain a écrit :
> This patch introduces a new afu sysfs attribute named afu_desc. This
> binary attribute provides access to raw contents of the afu descriptor
> to user-space. Direct access to afu descriptor is useful for libcxl
> that can use it to determine if the CXL card has been fenced or
> provide application access to afu attributes beyond one defined in
> CAIA.
>
> We introduce three new backend-ops:
>
> * afu_desc_size(): Return the size in bytes of the afu descriptor.
>
> * afu_desc_read(): Copy into a provided buffer contents of afu
>   descriptor starting at specific offset.
>
> * afu_desc_mmap(): Memory map the afu descriptor to the given
>   vm_area_struct.
>
> If afu_desc_size() > 0 the afu_desc attribute gets created for the AFU.
> The bin_attribute callbacks route the calls to corresponding cxl backend
> implementation.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
> ---
>  Documentation/ABI/testing/sysfs-class-cxl |  9 +++++++
>  drivers/misc/cxl/cxl.h                    |  9 +++++++
>  drivers/misc/cxl/sysfs.c                  | 45 +++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-cxl b/Documentation/ABI/testing/sysfs-class-cxl
> index 640f65e..9ac84c4 100644
> --- a/Documentation/ABI/testing/sysfs-class-cxl
> +++ b/Documentation/ABI/testing/sysfs-class-cxl
> @@ -6,6 +6,15 @@ Example: The real path of the attribute /sys/class/cxl/afu0.0s/irqs_max is
>
>  Slave contexts (eg. /sys/class/cxl/afu0.0s):
>
> +What:           /sys/class/cxl/<afu>/afu_desc
> +Date:           March 2016
> +Contact:        linuxppc-dev@lists.ozlabs.org
> +Description:    read only
> +                AFU Descriptor contents. The contents of this file are
> +		binary contents of the AFU descriptor. LIBCXL library can
> +		use this file to read afu descriptor and in some special cases
> +		determine if the cxl card has been fenced.
> +
>  What:           /sys/class/cxl/<afu>/afu_err_buf
>  Date:           September 2014
>  Contact:        linuxppc-dev@lists.ozlabs.org
> diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
> index ef683b7..1c43d06 100644
> --- a/drivers/misc/cxl/cxl.h
> +++ b/drivers/misc/cxl/cxl.h
> @@ -426,6 +426,9 @@ struct cxl_afu {
>  	u64 eb_len, eb_offset;
>  	struct bin_attribute attr_eb;
>
> +	/* Afu descriptor */
> +	struct bin_attribute attr_afud;
> +
>  	/* pointer to the vphb */
>  	struct pci_controller *phb;
>
> @@ -995,6 +998,12 @@ struct cxl_backend_ops {
>  	int (*afu_cr_write16)(struct cxl_afu *afu, int cr_idx, u64 offset, u16 val);
>  	int (*afu_cr_write32)(struct cxl_afu *afu, int cr_idx, u64 offset, u32 val);
>  	ssize_t (*read_adapter_vpd)(struct cxl *adapter, void *buf, size_t count);
> +	/* Access to AFU descriptor */
> +	ssize_t (*afu_desc_size)(struct cxl_afu *afu);
> +	ssize_t (*afu_desc_read)(struct cxl_afu *afu, char *buf, loff_t off,
> +				 size_t count);
> +	int (*afu_desc_mmap)(struct cxl_afu *afu, struct file *filp,
> +			     struct vm_area_struct *vma);
>  };
>  extern const struct cxl_backend_ops cxl_native_ops;
>  extern const struct cxl_backend_ops cxl_guest_ops;
> diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c
> index a8b6d6a..fff3468 100644
> --- a/drivers/misc/cxl/sysfs.c
> +++ b/drivers/misc/cxl/sysfs.c
> @@ -426,6 +426,26 @@ static ssize_t afu_eb_read(struct file *filp, struct kobject *kobj,
>  	return cxl_ops->afu_read_err_buffer(afu, buf, off, count);
>  }
>
> +static ssize_t afu_desc_read(struct file *filp, struct kobject *kobj,
> +			     struct bin_attribute *bin_attr, char *buf,
> +			     loff_t off, size_t count)
> +{
> +	struct cxl_afu *afu = to_cxl_afu(kobj_to_dev(kobj));
> +
> +	return cxl_ops->afu_desc_read ?
> +		cxl_ops->afu_desc_read(afu, buf, off, count) : -EIO;
> +}
> +
> +static int afu_desc_mmap(struct file *filp, struct kobject *kobj,
> +			 struct bin_attribute *attr, struct vm_area_struct *vma)
> +{
> +	struct cxl_afu *afu = to_cxl_afu(kobj_to_dev(kobj));
> +
> +	return cxl_ops->afu_desc_mmap ?
> +		cxl_ops->afu_desc_mmap(afu, filp, vma) : -EINVAL;
> +}
> +
> +
>  static struct device_attribute afu_attrs[] = {
>  	__ATTR_RO(mmio_size),
>  	__ATTR_RO(irqs_min),
> @@ -625,6 +645,9 @@ void cxl_sysfs_afu_remove(struct cxl_afu *afu)
>  	struct afu_config_record *cr, *tmp;
>  	int i;
>
> +	if (afu->attr_afud.size > 0)
> +		device_remove_bin_file(&afu->dev, &afu->attr_afud);
> +
>  	/* remove the err buffer bin attribute */
>  	if (afu->eb_len)
>  		device_remove_bin_file(&afu->dev, &afu->attr_eb);
> @@ -686,6 +709,28 @@ int cxl_sysfs_afu_add(struct cxl_afu *afu)
>  		list_add(&cr->list, &afu->crs);
>  	}
>
> +	/* Create the sysfs binattr for afu-descriptor */
> +	afu->attr_afud.size = cxl_ops->afu_desc_size ?
> +		cxl_ops->afu_desc_size(afu) : 0;
> +
> +	if (afu->attr_afud.size > 0) {
> +		sysfs_attr_init(&afu->attr_afud.attr);
> +		afu->attr_afud.attr.name = "afu_desc";
> +		afu->attr_afud.attr.mode = 0444;


So I had a long pause here, wondering if we are showing too much 
information to ANY user. Most of the content of the AFU descriptor is 
already world-readable through other sysfs attributes, the only ones 
troubling me are offset 0x50 and 0x58, ie. the page resolution interrupt 
handling. We currently don't support it (yet), but we need to think 
about it. A user process, by monitoring offset x50 could get a glimpse 
of the layout of the address space of other processes. Not the actual 
content, but at least what addresses are valid. I think that's already 
too much.

Why not just exporting an area only covering 64 bits (and guaranteed not 
to be all 1's) and call it mmio_check? It would seem safer to me.

   Fred

Re: [PATCH 2/3] cxl: Introduce afu_desc sysfs attribute

[Andrew Donnellan]

On 14/03/17 15:06, Vaibhav Jain wrote:
> This patch introduces a new afu sysfs attribute named afu_desc. This
> binary attribute provides access to raw contents of the afu descriptor
> to user-space. Direct access to afu descriptor is useful for libcxl
> that can use it to determine if the CXL card has been fenced or
> provide application access to afu attributes beyond one defined in
> CAIA.
>
> We introduce three new backend-ops:
>
> * afu_desc_size(): Return the size in bytes of the afu descriptor.
>
> * afu_desc_read(): Copy into a provided buffer contents of afu
>   descriptor starting at specific offset.
>
> * afu_desc_mmap(): Memory map the afu descriptor to the given
>   vm_area_struct.
>
> If afu_desc_size() > 0 the afu_desc attribute gets created for the AFU.
> The bin_attribute callbacks route the calls to corresponding cxl backend
> implementation.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
> ---
>  Documentation/ABI/testing/sysfs-class-cxl |  9 +++++++
>  drivers/misc/cxl/cxl.h                    |  9 +++++++
>  drivers/misc/cxl/sysfs.c                  | 45 +++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-cxl b/Documentation/ABI/testing/sysfs-class-cxl
> index 640f65e..9ac84c4 100644
> --- a/Documentation/ABI/testing/sysfs-class-cxl
> +++ b/Documentation/ABI/testing/sysfs-class-cxl
> @@ -6,6 +6,15 @@ Example: The real path of the attribute /sys/class/cxl/afu0.0s/irqs_max is
>
>  Slave contexts (eg. /sys/class/cxl/afu0.0s):
>
> +What:           /sys/class/cxl/<afu>/afu_desc
> +Date:           March 2016
> +Contact:        linuxppc-dev@lists.ozlabs.org
> +Description:    read only
> +                AFU Descriptor contents. The contents of this file are
> +		binary contents of the AFU descriptor. LIBCXL library can

"The libcxl"

> +		use this file to read afu descriptor and in some special cases
> +		determine if the cxl card has been fenced.
> +
>  What:           /sys/class/cxl/<afu>/afu_err_buf
>  Date:           September 2014
>  Contact:        linuxppc-dev@lists.ozlabs.org
> diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
> index ef683b7..1c43d06 100644
> --- a/drivers/misc/cxl/cxl.h
> +++ b/drivers/misc/cxl/cxl.h
> @@ -426,6 +426,9 @@ struct cxl_afu {
>  	u64 eb_len, eb_offset;
>  	struct bin_attribute attr_eb;
>
> +	/* Afu descriptor */
> +	struct bin_attribute attr_afud;
> +
>  	/* pointer to the vphb */
>  	struct pci_controller *phb;
>
> @@ -995,6 +998,12 @@ struct cxl_backend_ops {
>  	int (*afu_cr_write16)(struct cxl_afu *afu, int cr_idx, u64 offset, u16 val);
>  	int (*afu_cr_write32)(struct cxl_afu *afu, int cr_idx, u64 offset, u32 val);
>  	ssize_t (*read_adapter_vpd)(struct cxl *adapter, void *buf, size_t count);
> +	/* Access to AFU descriptor */
> +	ssize_t (*afu_desc_size)(struct cxl_afu *afu);
> +	ssize_t (*afu_desc_read)(struct cxl_afu *afu, char *buf, loff_t off,
> +				 size_t count);
> +	int (*afu_desc_mmap)(struct cxl_afu *afu, struct file *filp,
> +			     struct vm_area_struct *vma);
>  };
>  extern const struct cxl_backend_ops cxl_native_ops;
>  extern const struct cxl_backend_ops cxl_guest_ops;
> diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c
> index a8b6d6a..fff3468 100644
> --- a/drivers/misc/cxl/sysfs.c
> +++ b/drivers/misc/cxl/sysfs.c
> @@ -426,6 +426,26 @@ static ssize_t afu_eb_read(struct file *filp, struct kobject *kobj,
>  	return cxl_ops->afu_read_err_buffer(afu, buf, off, count);
>  }
>
> +static ssize_t afu_desc_read(struct file *filp, struct kobject *kobj,
> +			     struct bin_attribute *bin_attr, char *buf,
> +			     loff_t off, size_t count)
> +{
> +	struct cxl_afu *afu = to_cxl_afu(kobj_to_dev(kobj));
> +
> +	return cxl_ops->afu_desc_read ?
> +		cxl_ops->afu_desc_read(afu, buf, off, count) : -EIO;
> +}
> +
> +static int afu_desc_mmap(struct file *filp, struct kobject *kobj,
> +			 struct bin_attribute *attr, struct vm_area_struct *vma)
> +{
> +	struct cxl_afu *afu = to_cxl_afu(kobj_to_dev(kobj));
> +
> +	return cxl_ops->afu_desc_mmap ?
> +		cxl_ops->afu_desc_mmap(afu, filp, vma) : -EINVAL;
> +}
> +
> +
>  static struct device_attribute afu_attrs[] = {
>  	__ATTR_RO(mmio_size),
>  	__ATTR_RO(irqs_min),
> @@ -625,6 +645,9 @@ void cxl_sysfs_afu_remove(struct cxl_afu *afu)
>  	struct afu_config_record *cr, *tmp;
>  	int i;
>
> +	if (afu->attr_afud.size > 0)
> +		device_remove_bin_file(&afu->dev, &afu->attr_afud);
> +
>  	/* remove the err buffer bin attribute */
>  	if (afu->eb_len)
>  		device_remove_bin_file(&afu->dev, &afu->attr_eb);
> @@ -686,6 +709,28 @@ int cxl_sysfs_afu_add(struct cxl_afu *afu)
>  		list_add(&cr->list, &afu->crs);
>  	}
>
> +	/* Create the sysfs binattr for afu-descriptor */
> +	afu->attr_afud.size = cxl_ops->afu_desc_size ?
> +		cxl_ops->afu_desc_size(afu) : 0;
> +
> +	if (afu->attr_afud.size > 0) {
> +		sysfs_attr_init(&afu->attr_afud.attr);
> +		afu->attr_afud.attr.name = "afu_desc";
> +		afu->attr_afud.attr.mode = 0444;

Per Fred, I'm not convinced that 444 is completely safe.

> +		afu->attr_afud.read = afu_desc_read;
> +		afu->attr_afud.mmap = afu_desc_mmap;
> +
> +		rc = device_create_bin_file(&afu->dev, &afu->attr_afud);
> +		if (rc) {
> +			/* indicate that we did'nt create the sysfs attr */

didn't

> +			afu->attr_afud.size = 0;
> +			dev_err(&afu->dev,
> +				"Unable to create afu_desc attr for the afu. Err(%d)\n",
> +				rc);
> +			goto err1;
> +		}
> +	}
> +
>  	return 0;
>
>  err1:
>

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH 1/3] cxl: Re-factor cxl_pci_afu_read_err_buffer()

[Vaibhav Jain]

Thanks for reviewing Andrew !!

Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:

>> +
>> +	/* max we can copy in one read is PAGE_SIZE */
>> +	if (aligned_length > ERR_BUFF_MAX_COPY_SIZE) {
>
> I'm not sure if the name ERR_BUFF_MAX_COPY_SIZE makes sense here any
> more.

It might make sense to change to PAGE_SIZE. Will do the change in
susequent revision.

-- 
Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.

Re: [PATCH 2/3] cxl: Introduce afu_desc sysfs attribute

[Vaibhav Jain]

Thanks for reviewing the patch Fred!!

Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes:
>> +
>> +	if (afu->attr_afud.size > 0) {
>> +		sysfs_attr_init(&afu->attr_afud.attr);
>> +		afu->attr_afud.attr.name = "afu_desc";
>> +		afu->attr_afud.attr.mode = 0444;
>
>
> So I had a long pause here, wondering if we are showing too much 
> information to ANY user. Most of the content of the AFU descriptor is 
> already world-readable through other sysfs attributes, the only ones 
> troubling me are offset 0x50 and 0x58, ie. the page resolution interrupt 
> handling. We currently don't support it (yet), but we need to think 
> about it. A user process, by monitoring offset x50 could get a glimpse 
> of the layout of the address space of other processes. Not the actual 
> content, but at least what addresses are valid. I think that's already 
> too much.
As we discussed "Paged Resolution EA (offset 0x58)" register is write
only should ideally return 0x00  for all reads (need to confirm with
h/w on this). Secondly there isnt much information exposed from register
"Paged Resolution Handle(offset 0x50)" apart from process handle. Lastly
presently Paged-Response Resolution isn't supported for cxl on linux. So,
I think we are safe for the time being.

> Why not just exporting an area only covering 64 bits (and guaranteed not 
> to be all 1's) and call it mmio_check? It would seem safer to me.
AFAIK the minimum granuality that we have is PAGE_SIZE and we cannot mmap
anything less than PAGE_SIZE. Though one exception for hashed paged
table on power, we have an ability to map 4K PFN on 64K hosts but it
isnt supported for radix on P9.

At best I can limit access to first page of the afu descriptor.


-- 
Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.