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

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.