[PATCH] ocxl: Add get_metadata IOCTL to share OCXL information to userspace

From: Alastair D'Silva <redacted>

Some required information is not exposed to userspace currently (eg. the
PASID), pass this information back, along with other information which
is currently communicated via sysfs, which saves some parsing effort in
userspace.

Signed-off-by: Alastair D'Silva <redacted>
---
 drivers/misc/ocxl/file.c | 27 +++++++++++++++++++++++++++
 include/uapi/misc/ocxl.h | 22 ++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
index d9aa407db06a..11514a8444e5 100644
--- a/drivers/misc/ocxl/file.c
+++ b/drivers/misc/ocxl/file.c

@@ -102,10 +102,32 @@ static long afu_ioctl_attach(struct ocxl_context *ctx,
 	return rc;
 }
 
+static long afu_ioctl_get_metadata(struct ocxl_context *ctx,
+		struct ocxl_ioctl_get_metadata __user *uarg)
+{
+	struct ocxl_ioctl_get_metadata arg;
+
+	memset(&arg, 0, sizeof(arg));
+
+	arg.version = 0;
+
+	arg.afu_version_major = ctx->afu->config.version_major;
+	arg.afu_version_minor = ctx->afu->config.version_minor;
+	arg.pasid = ctx->pasid;
+	arg.pp_mmio_size = ctx->afu->config.pp_mmio_stride;
+	arg.global_mmio_size = ctx->afu->config.global_mmio_size;
+
+	if (copy_to_user(uarg, &arg, sizeof(arg)))
+		return -EFAULT;
+
+	return 0;
+}
+
 #define CMD_STR(x) (x == OCXL_IOCTL_ATTACH ? "ATTACH" :			\
 			x == OCXL_IOCTL_IRQ_ALLOC ? "IRQ_ALLOC" :	\
 			x == OCXL_IOCTL_IRQ_FREE ? "IRQ_FREE" :		\
 			x == OCXL_IOCTL_IRQ_SET_FD ? "IRQ_SET_FD" :	\
+			x == OCXL_IOCTL_GET_METADATA ? "GET_METADATA" :	\
 			"UNKNOWN")
 
 static long afu_ioctl(struct file *file, unsigned int cmd,

@@ -157,6 +179,11 @@ static long afu_ioctl(struct file *file, unsigned int cmd,
 					irq_fd.eventfd);
 		break;
 
+	case OCXL_IOCTL_GET_METADATA:
+		rc = afu_ioctl_get_metadata(ctx,
+				(struct ocxl_ioctl_get_metadata __user *) args);
+		break;
+
 	default:
 		rc = -EINVAL;
 	}

diff --git a/include/uapi/misc/ocxl.h b/include/uapi/misc/ocxl.h
index 4b0b0b756f3e..16e1f48ce280 100644
--- a/include/uapi/misc/ocxl.h
+++ b/include/uapi/misc/ocxl.h

@@ -32,6 +32,27 @@ struct ocxl_ioctl_attach {
 	__u64 reserved3;
 };
 
+/*
+ * Version contains the version of the struct.
+ * Versions will always be backwards compatible, that is, new versions will not
+ * alter existing fields
+ */
+struct ocxl_ioctl_get_metadata {
+	__u16 version;
+
+	// Version 0 fields
+	__u8  afu_version_major;
+	__u8  afu_version_minor;
+	__u32 pasid;
+
+	__u64 pp_mmio_size;
+	__u64 global_mmio_size;
+
+	// End version 0 fields
+
+	__u64 reserved[13]; // Total of 16*u64
+};
+
 struct ocxl_ioctl_irq_fd {
 	__u64 irq_offset;
 	__s32 eventfd;

@@ -45,5 +66,6 @@ struct ocxl_ioctl_irq_fd {
 #define OCXL_IOCTL_IRQ_ALLOC	_IOR(OCXL_MAGIC, 0x11, __u64)
 #define OCXL_IOCTL_IRQ_FREE	_IOW(OCXL_MAGIC, 0x12, __u64)
 #define OCXL_IOCTL_IRQ_SET_FD	_IOW(OCXL_MAGIC, 0x13, struct ocxl_ioctl_irq_fd)
+#define OCXL_IOCTL_GET_METADATA _IOR(OCXL_MAGIC, 0x14, struct ocxl_ioctl_get_metadata)
 
 #endif /* _UAPI_MISC_OCXL_H */

-- 
2.14.3

On 21/02/18 15:57, Alastair D'Silva wrote:

From: Alastair D'Silva <redacted>

Some required information is not exposed to userspace currently (eg. the
PASID), pass this information back, along with other information which
is currently communicated via sysfs, which saves some parsing effort in
userspace.

Signed-off-by: Alastair D'Silva <redacted>

Seems fine.

Acked-by: Andrew Donnellan <redacted>

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

On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva [off-list ref] wrote:

Why do we call this metadata? Isn't this an afu_descriptor?

+{
+       struct ocxl_ioctl_get_metadata arg;
+
+       memset(&arg, 0, sizeof(arg));
+
+       arg.version = 0;

Does it make sense to have version 0? Even if does, you can afford
to skip initialization due to the memset above. I prefer that versions
start with 1

This sounds more like a function name, do we need it to be _get_metdata?

+       __u16 version;
+
+       // Version 0 fields
+       __u8  afu_version_major;
+       __u8  afu_version_minor;
+       __u32 pasid;
+
+       __u64 pp_mmio_size;
+       __u64 global_mmio_size;
+

Should we document the fields? pp_ stands for per process, but is not
very clear at first look. Why do we care to return only the size, what
about lpc size?

+       // End version 0 fields
+
+       __u64 reserved[13]; // Total of 16*u64
+};

Balbir Singh.

Le 21/02/2018 à 07:43, Balbir Singh a écrit :

On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva [off-list ref] wrote:

quoted

From: Alastair D'Silva <redacted>

Some required information is not exposed to userspace currently (eg. the
PASID), pass this information back, along with other information which
is currently communicated via sysfs, which saves some parsing effort in
userspace.

Signed-off-by: Alastair D'Silva <redacted>
---
  drivers/misc/ocxl/file.c | 27 +++++++++++++++++++++++++++
  include/uapi/misc/ocxl.h | 22 ++++++++++++++++++++++
  2 files changed, 49 insertions(+)

diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
index d9aa407db06a..11514a8444e5 100644
--- a/drivers/misc/ocxl/file.c
+++ b/drivers/misc/ocxl/file.c

@@ -102,10 +102,32 @@ static long afu_ioctl_attach(struct ocxl_context *ctx,
         return rc;
  }

+static long afu_ioctl_get_metadata(struct ocxl_context *ctx,
+               struct ocxl_ioctl_get_metadata __user *uarg)

Why do we call this metadata? Isn't this an afu_descriptor?

quoted

+{
+       struct ocxl_ioctl_get_metadata arg;
+
+       memset(&arg, 0, sizeof(arg));
+
+       arg.version = 0;

Does it make sense to have version 0? Even if does, you can afford
to skip initialization due to the memset above. I prefer that versions
start with 1

quoted

+
+       arg.afu_version_major = ctx->afu->config.version_major;
+       arg.afu_version_minor = ctx->afu->config.version_minor;
+       arg.pasid = ctx->pasid;
+       arg.pp_mmio_size = ctx->afu->config.pp_mmio_stride;
+       arg.global_mmio_size = ctx->afu->config.global_mmio_size;
+
+       if (copy_to_user(uarg, &arg, sizeof(arg)))
+               return -EFAULT;
+
+       return 0;
+}
+
  #define CMD_STR(x) (x == OCXL_IOCTL_ATTACH ? "ATTACH" :                        \
                         x == OCXL_IOCTL_IRQ_ALLOC ? "IRQ_ALLOC" :       \
                         x == OCXL_IOCTL_IRQ_FREE ? "IRQ_FREE" :         \
                         x == OCXL_IOCTL_IRQ_SET_FD ? "IRQ_SET_FD" :     \
+                       x == OCXL_IOCTL_GET_METADATA ? "GET_METADATA" : \
                         "UNKNOWN")

  static long afu_ioctl(struct file *file, unsigned int cmd,

@@ -157,6 +179,11 @@ static long afu_ioctl(struct file *file, unsigned int cmd,
                                         irq_fd.eventfd);
                 break;

+       case OCXL_IOCTL_GET_METADATA:
+               rc = afu_ioctl_get_metadata(ctx,
+                               (struct ocxl_ioctl_get_metadata __user *) args);
+               break;
+
         default:
                 rc = -EINVAL;
         }

diff --git a/include/uapi/misc/ocxl.h b/include/uapi/misc/ocxl.h
index 4b0b0b756f3e..16e1f48ce280 100644
--- a/include/uapi/misc/ocxl.h
+++ b/include/uapi/misc/ocxl.h

@@ -32,6 +32,27 @@ struct ocxl_ioctl_attach {
         __u64 reserved3;
  };

+/*
+ * Version contains the version of the struct.
+ * Versions will always be backwards compatible, that is, new versions will not
+ * alter existing fields
+ */
+struct ocxl_ioctl_get_metadata {

This sounds more like a function name, do we need it to be _get_metdata?

quoted

+       __u16 version;
+
+       // Version 0 fields
+       __u8  afu_version_major;
+       __u8  afu_version_minor;
+       __u32 pasid;
+
+       __u64 pp_mmio_size;
+       __u64 global_mmio_size;
+

Should we document the fields? pp_ stands for per process, but is not
very clear at first look. Why do we care to return only the size, what
about lpc size?

My bad, I forgot to mention it before. There's a somewhat high-level 
description which needs updating in:
Documentation/accelerators/ocxl.rst

It doesn't go down to the level of the structure members, but at least 
all ioctl commands should have a brief description.

lpc_size could be added. It's currently useless to the library, but 
doesn't hurt. The one which was giving me troubles on a previous version 
of this patch was the lpc numa node ID, since that was experimental code 
and felt out of place considering what's been upstreamed in skiboot and 
linux so far.

   Fred

quoted

+       // End version 0 fields
+
+       __u64 reserved[13]; // Total of 16*u64
+};

Balbir Singh.

On Wed, 2018-02-21 at 17:43 +1100, Balbir Singh wrote:

On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <alastair@au1.ibm.c
om> wrote:

quoted

From: Alastair D'Silva <redacted>

Some required information is not exposed to userspace currently
(eg. the
PASID), pass this information back, along with other information
which
is currently communicated via sysfs, which saves some parsing
effort in
userspace.

Signed-off-by: Alastair D'Silva <redacted>
---
 drivers/misc/ocxl/file.c | 27 +++++++++++++++++++++++++++
 include/uapi/misc/ocxl.h | 22 ++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
index d9aa407db06a..11514a8444e5 100644
--- a/drivers/misc/ocxl/file.c
+++ b/drivers/misc/ocxl/file.c

@@ -102,10 +102,32 @@ static long afu_ioctl_attach(struct

ocxl_context *ctx,
        return rc;
 }

+static long afu_ioctl_get_metadata(struct ocxl_context *ctx,
+               struct ocxl_ioctl_get_metadata __user *uarg)

Why do we call this metadata? Isn't this an afu_descriptor?

It's metadata for the descriptor.

quoted

+{
+       struct ocxl_ioctl_get_metadata arg;
+
+       memset(&arg, 0, sizeof(arg));
+
+       arg.version = 0;

Does it make sense to have version 0? Even if does, you can afford
to skip initialization due to the memset above. I prefer that
versions
start with 1

Setting it to 0 is for the reader, not the compiler. I'm not clear on
the benefit of starting the version at 1, could you clarify?

quoted

+
+       arg.afu_version_major = ctx->afu->config.version_major;
+       arg.afu_version_minor = ctx->afu->config.version_minor;
+       arg.pasid = ctx->pasid;
+       arg.pp_mmio_size = ctx->afu->config.pp_mmio_stride;
+       arg.global_mmio_size = ctx->afu->config.global_mmio_size;
+
+       if (copy_to_user(uarg, &arg, sizeof(arg)))
+               return -EFAULT;
+
+       return 0;
+}
+
 #define CMD_STR(x) (x == OCXL_IOCTL_ATTACH ? "ATTACH"
:                        \
                        x == OCXL_IOCTL_IRQ_ALLOC ? "IRQ_ALLOC"
:       \
                        x == OCXL_IOCTL_IRQ_FREE ? "IRQ_FREE"
:         \
                        x == OCXL_IOCTL_IRQ_SET_FD ? "IRQ_SET_FD"
:     \
+                       x == OCXL_IOCTL_GET_METADATA ?
"GET_METADATA" : \
                        "UNKNOWN")

 static long afu_ioctl(struct file *file, unsigned int cmd,

@@ -157,6 +179,11 @@ static long afu_ioctl(struct file *file,

unsigned int cmd,
                                        irq_fd.eventfd);
                break;

+       case OCXL_IOCTL_GET_METADATA:
+               rc = afu_ioctl_get_metadata(ctx,
+                               (struct ocxl_ioctl_get_metadata
__user *) args);
+               break;
+
        default:
                rc = -EINVAL;
        }

diff --git a/include/uapi/misc/ocxl.h b/include/uapi/misc/ocxl.h
index 4b0b0b756f3e..16e1f48ce280 100644
--- a/include/uapi/misc/ocxl.h
+++ b/include/uapi/misc/ocxl.h

@@ -32,6 +32,27 @@ struct ocxl_ioctl_attach {
        __u64 reserved3;
 };

+/*
+ * Version contains the version of the struct.
+ * Versions will always be backwards compatible, that is, new

versions will not
+ * alter existing fields
+ */
+struct ocxl_ioctl_get_metadata {

This sounds more like a function name, do we need it to be
_get_metdata?

It pretty much is a function, it returns to userspace metadata about
the descriptor being operated on.

quoted

+       __u16 version;
+
+       // Version 0 fields
+       __u8  afu_version_major;
+       __u8  afu_version_minor;
+       __u32 pasid;
+
+       __u64 pp_mmio_size;
+       __u64 global_mmio_size;
+

Should we document the fields? pp_ stands for per process, but is not
very clear at first look. Why do we care to return only the size,
what
about lpc size?

Yes, I would rather call it per_pasid_mmio_size, but consistency with
the rest of the driver (& exposed sysfs entries) is also important.

quoted

+       // End version 0 fields
+
+       __u64 reserved[13]; // Total of 16*u64
+};

Balbir Singh.

-- 
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australiamob: 0423 762 819

On Wed, 2018-02-21 at 12:25 +0100, Frederic Barrat wrote:

Le 21/02/2018 à 07:43, Balbir Singh a écrit :

quoted

On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <alastair@au1.ibm
.com> wrote:

quoted

From: Alastair D'Silva <redacted>

Some required information is not exposed to userspace currently
(eg. the
PASID), pass this information back, along with other information
which
is currently communicated via sysfs, which saves some parsing
effort in
userspace.

Signed-off-by: Alastair D'Silva <redacted>

<snip>

quoted

Should we document the fields? pp_ stands for per process, but is
not
very clear at first look. Why do we care to return only the size,
what
about lpc size?

My bad, I forgot to mention it before. There's a somewhat high-level 
description which needs updating in:
Documentation/accelerators/ocxl.rst

It doesn't go down to the level of the structure members, but at
least 
all ioctl commands should have a brief description.

I'll update the docs.

lpc_size could be added. It's currently useless to the library, but 
doesn't hurt. The one which was giving me troubles on a previous
version 
of this patch was the lpc numa node ID, since that was experimental
code 
and felt out of place considering what's been upstreamed in skiboot
and 
linux so far.

I'd rather add the LPC members when the rest of the LPC code goes in.
At the moment, the LPC size represents the window size (as a power of
2), whereas we expect that it should represent the actual amount of LPC
memory exposed. I would rather avoid changing semantics of members in
released code, or burning another reserved member for the updated
definition if we can avoid it.

-- 
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819

"Alastair D'Silva" [off-list ref] writes:

On Wed, 2018-02-21 at 17:43 +1100, Balbir Singh wrote:

quoted

On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <alastair@au1.ibm.c
om> wrote:

quoted

From: Alastair D'Silva <redacted>

...

quoted

quoted

diff --git a/include/uapi/misc/ocxl.h b/include/uapi/misc/ocxl.h
index 4b0b0b756f3e..16e1f48ce280 100644
--- a/include/uapi/misc/ocxl.h
+++ b/include/uapi/misc/ocxl.h

@@ -32,6 +32,27 @@ struct ocxl_ioctl_attach {
        __u64 reserved3;
 };

+/*
+ * Version contains the version of the struct.
+ * Versions will always be backwards compatible, that is, new

versions will not
+ * alter existing fields
+ */
+struct ocxl_ioctl_get_metadata {

This sounds more like a function name, do we need it to be
_get_metdata?

It pretty much is a function, it returns to userspace metadata about
the descriptor being operated on.

It's not a function, it's a struct?

Outside of "management English" "get" is a verb, so using it in the name
of the struct is confusing, it should be a noun phrase.

cheers

On Thu, Feb 22, 2018 at 10:32 AM, Alastair D'Silva [off-list ref] wrote:

On Wed, 2018-02-21 at 17:43 +1100, Balbir Singh wrote:

quoted

On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <alastair@au1.ibm.c
om> wrote:

quoted

From: Alastair D'Silva <redacted>

Some required information is not exposed to userspace currently
(eg. the
PASID), pass this information back, along with other information
which
is currently communicated via sysfs, which saves some parsing
effort in
userspace.

Signed-off-by: Alastair D'Silva <redacted>
---
 drivers/misc/ocxl/file.c | 27 +++++++++++++++++++++++++++
 include/uapi/misc/ocxl.h | 22 ++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
index d9aa407db06a..11514a8444e5 100644
--- a/drivers/misc/ocxl/file.c
+++ b/drivers/misc/ocxl/file.c

@@ -102,10 +102,32 @@ static long afu_ioctl_attach(struct

ocxl_context *ctx,
        return rc;
 }

+static long afu_ioctl_get_metadata(struct ocxl_context *ctx,
+               struct ocxl_ioctl_get_metadata __user *uarg)

Why do we call this metadata? Isn't this an afu_descriptor?

It's metadata for the descriptor.

I meant metadata is too generic, could we have other types of metadata in OCXL?

quoted

quoted

+{
+       struct ocxl_ioctl_get_metadata arg;
+
+       memset(&arg, 0, sizeof(arg));
+
+       arg.version = 0;

Does it make sense to have version 0? Even if does, you can afford
to skip initialization due to the memset above. I prefer that
versions
start with 1

Setting it to 0 is for the reader, not the compiler. I'm not clear on
the benefit of starting the version at 1, could you clarify?

How do I distinguish between version number never set and 0?

quoted

quoted

+
+       arg.afu_version_major = ctx->afu->config.version_major;
+       arg.afu_version_minor = ctx->afu->config.version_minor;
+       arg.pasid = ctx->pasid;
+       arg.pp_mmio_size = ctx->afu->config.pp_mmio_stride;
+       arg.global_mmio_size = ctx->afu->config.global_mmio_size;
+
+       if (copy_to_user(uarg, &arg, sizeof(arg)))
+               return -EFAULT;
+
+       return 0;
+}
+
 #define CMD_STR(x) (x == OCXL_IOCTL_ATTACH ? "ATTACH"
:                        \
                        x == OCXL_IOCTL_IRQ_ALLOC ? "IRQ_ALLOC"
:       \
                        x == OCXL_IOCTL_IRQ_FREE ? "IRQ_FREE"
:         \
                        x == OCXL_IOCTL_IRQ_SET_FD ? "IRQ_SET_FD"
:     \
+                       x == OCXL_IOCTL_GET_METADATA ?
"GET_METADATA" : \
                        "UNKNOWN")

 static long afu_ioctl(struct file *file, unsigned int cmd,

@@ -157,6 +179,11 @@ static long afu_ioctl(struct file *file,

unsigned int cmd,
                                        irq_fd.eventfd);
                break;

+       case OCXL_IOCTL_GET_METADATA:
+               rc = afu_ioctl_get_metadata(ctx,
+                               (struct ocxl_ioctl_get_metadata
__user *) args);
+               break;
+
        default:
                rc = -EINVAL;
        }

diff --git a/include/uapi/misc/ocxl.h b/include/uapi/misc/ocxl.h
index 4b0b0b756f3e..16e1f48ce280 100644
--- a/include/uapi/misc/ocxl.h
+++ b/include/uapi/misc/ocxl.h

@@ -32,6 +32,27 @@ struct ocxl_ioctl_attach {
        __u64 reserved3;
 };

+/*
+ * Version contains the version of the struct.
+ * Versions will always be backwards compatible, that is, new

versions will not
+ * alter existing fields
+ */
+struct ocxl_ioctl_get_metadata {

This sounds more like a function name, do we need it to be
_get_metdata?

It pretty much is a function, it returns to userspace metadata about
the descriptor being operated on.

It has a verb indicating action

quoted

quoted

+       __u16 version;
+
+       // Version 0 fields
+       __u8  afu_version_major;
+       __u8  afu_version_minor;
+       __u32 pasid;
+
+       __u64 pp_mmio_size;
+       __u64 global_mmio_size;
+

Should we document the fields? pp_ stands for per process, but is not
very clear at first look. Why do we care to return only the size,
what
about lpc size?

Yes, I would rather call it per_pasid_mmio_size, but consistency with
the rest of the driver (& exposed sysfs entries) is also important.

Balbir Singh.

On Wed, Feb 21, 2018 at 10:25 PM, Frederic Barrat
[off-list ref] wrote:

Le 21/02/2018 =C3=A0 07:43, Balbir Singh a =C3=A9crit :

quoted

On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva [off-list ref]
wrote:

quoted

From: Alastair D'Silva <redacted>

Some required information is not exposed to userspace currently (eg. th=

e

quoted

quoted

PASID), pass this information back, along with other information which
is currently communicated via sysfs, which saves some parsing effort in
userspace.

Signed-off-by: Alastair D'Silva <redacted>
---
  drivers/misc/ocxl/file.c | 27 +++++++++++++++++++++++++++
  include/uapi/misc/ocxl.h | 22 ++++++++++++++++++++++
  2 files changed, 49 insertions(+)

diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
index d9aa407db06a..11514a8444e5 100644
--- a/drivers/misc/ocxl/file.c
+++ b/drivers/misc/ocxl/file.c

@@ -102,10 +102,32 @@ static long afu_ioctl_attach(struct ocxl_context

*ctx,
         return rc;
  }

+static long afu_ioctl_get_metadata(struct ocxl_context *ctx,
+               struct ocxl_ioctl_get_metadata __user *uarg)

Why do we call this metadata? Isn't this an afu_descriptor?

quoted

+{
+       struct ocxl_ioctl_get_metadata arg;
+
+       memset(&arg, 0, sizeof(arg));
+
+       arg.version =3D 0;

Does it make sense to have version 0? Even if does, you can afford
to skip initialization due to the memset above. I prefer that versions
start with 1

quoted

+
+       arg.afu_version_major =3D ctx->afu->config.version_major;
+       arg.afu_version_minor =3D ctx->afu->config.version_minor;
+       arg.pasid =3D ctx->pasid;
+       arg.pp_mmio_size =3D ctx->afu->config.pp_mmio_stride;
+       arg.global_mmio_size =3D ctx->afu->config.global_mmio_size;
+
+       if (copy_to_user(uarg, &arg, sizeof(arg)))
+               return -EFAULT;
+
+       return 0;
+}
+
  #define CMD_STR(x) (x =3D=3D OCXL_IOCTL_ATTACH ? "ATTACH" :
\
                         x =3D=3D OCXL_IOCTL_IRQ_ALLOC ? "IRQ_ALLOC" :
\
                         x =3D=3D OCXL_IOCTL_IRQ_FREE ? "IRQ_FREE" :
\
                         x =3D=3D OCXL_IOCTL_IRQ_SET_FD ? "IRQ_SET_FD" =

:

quoted

quoted

\
+                       x =3D=3D OCXL_IOCTL_GET_METADATA ? "GET_METADAT=

A" : \

quoted

quoted

                         "UNKNOWN")

  static long afu_ioctl(struct file *file, unsigned int cmd,

@@ -157,6 +179,11 @@ static long afu_ioctl(struct file *file, unsigned

int cmd,
                                         irq_fd.eventfd);
                 break;

+       case OCXL_IOCTL_GET_METADATA:
+               rc =3D afu_ioctl_get_metadata(ctx,
+                               (struct ocxl_ioctl_get_metadata __user =

*)

quoted

quoted

args);
+               break;
+
         default:
                 rc =3D -EINVAL;
         }

diff --git a/include/uapi/misc/ocxl.h b/include/uapi/misc/ocxl.h
index 4b0b0b756f3e..16e1f48ce280 100644
--- a/include/uapi/misc/ocxl.h
+++ b/include/uapi/misc/ocxl.h

@@ -32,6 +32,27 @@ struct ocxl_ioctl_attach {
         __u64 reserved3;
  };

+/*
+ * Version contains the version of the struct.
+ * Versions will always be backwards compatible, that is, new versions

will not
+ * alter existing fields
+ */
+struct ocxl_ioctl_get_metadata {

This sounds more like a function name, do we need it to be _get_metdata?

quoted

+       __u16 version;
+
+       // Version 0 fields
+       __u8  afu_version_major;
+       __u8  afu_version_minor;
+       __u32 pasid;
+
+       __u64 pp_mmio_size;
+       __u64 global_mmio_size;
+

Should we document the fields? pp_ stands for per process, but is not
very clear at first look. Why do we care to return only the size, what
about lpc size?

My bad, I forgot to mention it before. There's a somewhat high-level
description which needs updating in:
Documentation/accelerators/ocxl.rst

Thanks, that's helpful

It doesn't go down to the level of the structure members, but at least al=

l

ioctl commands should have a brief description.

lpc_size could be added. It's currently useless to the library, but doesn=

't

hurt. The one which was giving me troubles on a previous version of this
patch was the lpc numa node ID, since that was experimental code and felt
out of place considering what's been upstreamed in skiboot and linux so f=

ar.

Yeah, I think metadata will evolve for a while till it settle's down.
Since ocxl_ioctl_get_metadata is exposed via uapi, a newer program
calling an older kernel will never work, since the size of that struct
will always be larger than what the OS supports and our copy_to_user()
will fail. The other option is for the user program to try all
possible versions till one succeeds, that is bad as well. I think
there are a few ways around it, if we care about this combination.

Balbir Singh.

On 22/02/18 14:41, Balbir Singh wrote:

quoted

Setting it to 0 is for the reader, not the compiler. I'm not clear on
the benefit of starting the version at 1, could you clarify?

How do I distinguish between version number never set and 0?

The ioctl won't exist without a version number set.

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

On Thu, 2018-02-22 at 14:41 +1100, Balbir Singh wrote:

On Thu, Feb 22, 2018 at 10:32 AM, Alastair D'Silva <alastair@au1.ibm.
com> wrote:

quoted

On Wed, 2018-02-21 at 17:43 +1100, Balbir Singh wrote:

quoted

On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <alastair@au1.i
bm.c
om> wrote:

quoted

From: Alastair D'Silva <redacted>

Some required information is not exposed to userspace currently
(eg. the
PASID), pass this information back, along with other
information
which
is currently communicated via sysfs, which saves some parsing
effort in
userspace.

Signed-off-by: Alastair D'Silva <redacted>
---
 drivers/misc/ocxl/file.c | 27 +++++++++++++++++++++++++++
 include/uapi/misc/ocxl.h | 22 ++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/misc/ocxl/file.c

b/drivers/misc/ocxl/file.c
index d9aa407db06a..11514a8444e5 100644

--- a/drivers/misc/ocxl/file.c
+++ b/drivers/misc/ocxl/file.c

@@ -102,10 +102,32 @@ static long afu_ioctl_attach(struct

ocxl_context *ctx,
        return rc;
 }

+static long afu_ioctl_get_metadata(struct ocxl_context *ctx,
+               struct ocxl_ioctl_get_metadata __user *uarg)

Why do we call this metadata? Isn't this an afu_descriptor?

It's metadata for the descriptor.

I meant metadata is too generic, could we have other types of
metadata in OCXL?

I don't believe so, we would instead expand the scope of this IOCTL
using version & space available from the reserved fields.

quoted

quoted

quoted

+{
+       struct ocxl_ioctl_get_metadata arg;
+
+       memset(&arg, 0, sizeof(arg));
+
+       arg.version = 0;

Does it make sense to have version 0? Even if does, you can
afford
to skip initialization due to the memset above. I prefer that
versions
start with 1

Setting it to 0 is for the reader, not the compiler. I'm not clear
on
the benefit of starting the version at 1, could you clarify?

How do I distinguish between version number never set and 0?

The version number is always set. If the IOCTL doesn't exist, the ioctl
call will error instead.

quoted

quoted

quoted

+
+       arg.afu_version_major = ctx->afu->config.version_major;
+       arg.afu_version_minor = ctx->afu->config.version_minor;
+       arg.pasid = ctx->pasid;
+       arg.pp_mmio_size = ctx->afu->config.pp_mmio_stride;
+       arg.global_mmio_size = ctx->afu-

quoted

config.global_mmio_size;

+
+       if (copy_to_user(uarg, &arg, sizeof(arg)))
+               return -EFAULT;
+
+       return 0;
+}
+
 #define CMD_STR(x) (x == OCXL_IOCTL_ATTACH ? "ATTACH"
:                        \
                        x == OCXL_IOCTL_IRQ_ALLOC ? "IRQ_ALLOC"
:       \
                        x == OCXL_IOCTL_IRQ_FREE ? "IRQ_FREE"
:         \
                        x == OCXL_IOCTL_IRQ_SET_FD ?
"IRQ_SET_FD"
:     \
+                       x == OCXL_IOCTL_GET_METADATA ?
"GET_METADATA" : \
                        "UNKNOWN")

 static long afu_ioctl(struct file *file, unsigned int cmd,

@@ -157,6 +179,11 @@ static long afu_ioctl(struct file *file,

unsigned int cmd,
                                        irq_fd.eventfd);
                break;

+       case OCXL_IOCTL_GET_METADATA:
+               rc = afu_ioctl_get_metadata(ctx,
+                               (struct ocxl_ioctl_get_metadata
__user *) args);
+               break;
+
        default:
                rc = -EINVAL;
        }

diff --git a/include/uapi/misc/ocxl.h

b/include/uapi/misc/ocxl.h
index 4b0b0b756f3e..16e1f48ce280 100644

--- a/include/uapi/misc/ocxl.h
+++ b/include/uapi/misc/ocxl.h

@@ -32,6 +32,27 @@ struct ocxl_ioctl_attach {
        __u64 reserved3;
 };

+/*
+ * Version contains the version of the struct.
+ * Versions will always be backwards compatible, that is, new

versions will not
+ * alter existing fields
+ */
+struct ocxl_ioctl_get_metadata {

This sounds more like a function name, do we need it to be
_get_metdata?

It pretty much is a function, it returns to userspace metadata
about
the descriptor being operated on.

It has a verb indicating action

I misunderstood, I had named the struct to match the IOCTL, but that
isn't necessary. I'll update it in the next patch.

-- 
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819

On Thu, 2018-02-22 at 14:46 +1100, Balbir Singh wrote:
<snip>

lpc_size could be added. It's currently useless to the library, but

quoted

doesn't
hurt. The one which was giving me troubles on a previous version of
this
patch was the lpc numa node ID, since that was experimental code
and felt
out of place considering what's been upstreamed in skiboot and
linux so far.

Yeah, I think metadata will evolve for a while till it settle's down.
Since ocxl_ioctl_get_metadata is exposed via uapi, a newer program
calling an older kernel will never work, since the size of that
struct
will always be larger than what the OS supports and our
copy_to_user()
will fail. The other option is for the user program to try all
possible versions till one succeeds, that is bad as well. I think
there are a few ways around it, if we care about this combination.

Balbir Singh.

We have a number of reserved members at the end of the struct which can
be re-purposed for future information (with a corresponding bump of the
version number).

-- 
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819

On Thu, Feb 22, 2018 at 2:51 PM, Alastair D'Silva [off-list ref] wrote:

On Thu, 2018-02-22 at 14:46 +1100, Balbir Singh wrote:
<snip>

quoted

lpc_size could be added. It's currently useless to the library, but

quoted

doesn't
hurt. The one which was giving me troubles on a previous version of
this
patch was the lpc numa node ID, since that was experimental code
and felt
out of place considering what's been upstreamed in skiboot and
linux so far.

Yeah, I think metadata will evolve for a while till it settle's down.
Since ocxl_ioctl_get_metadata is exposed via uapi, a newer program
calling an older kernel will never work, since the size of that
struct
will always be larger than what the OS supports and our
copy_to_user()
will fail. The other option is for the user program to try all
possible versions till one succeeds, that is bad as well. I think
there are a few ways around it, if we care about this combination.

Balbir Singh.

We have a number of reserved members at the end of the struct which can
be re-purposed for future information (with a corresponding bump of the
version number).

Good point, agreed

Balbir Singh.

quoted

quoted

Yeah, I think metadata will evolve for a while till it settle's down.
Since ocxl_ioctl_get_metadata is exposed via uapi, a newer program
calling an older kernel will never work, since the size of that
struct
will always be larger than what the OS supports and our
copy_to_user()
will fail. The other option is for the user program to try all
possible versions till one succeeds, that is bad as well. I think
there are a few ways around it, if we care about this combination.

Balbir Singh.

We have a number of reserved members at the end of the struct which can
be re-purposed for future information (with a corresponding bump of the
version number).

Good point, agreed

I initially had reservations about using an ioctl command for various 
AFU/context parameters because extensibility is going to be a pain. With 
the current reserved fields and versioning, we're ok for some time 
(version handling will remain a bit of a pain but that's life). I agree 
it helps the library by making things more light weight compared to 
sysfs. But if we need to add parameters in the future, we should keep 
sysfs as an option, especially if it's for rarely used parameters, i.e. 
not something we'll need immediately after an open().

   Fred