From: Alastair D'Silva <[email protected]>
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 <[email protected]>
---
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 <[email protected]>
>
> 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 <[email protected]>
Seems fine.
Acked-by: Andrew Donnellan <[email protected]>
--
Andrew Donnellan OzLabs, ADL Canberra
[email protected] IBM Australia Limited
On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <[email protected]> wrote:
> From: Alastair D'Silva <[email protected]>
>
> 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 <[email protected]>
> ---
> 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?
> +{
> + 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
> +
> + 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?
> + __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 <[email protected]> wrote:
>> From: Alastair D'Silva <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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?
>
>> +{
>> + 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
>
>> +
>> + 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?
>
>> + __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
>> + // 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 <[email protected]
> om> wrote:
> > From: Alastair D'Silva <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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.
> > +{
> > + 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?
> > +
> > + 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.
> > + __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.
> > + // 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 :
> > On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <[email protected]
> > .com> wrote:
> > > From: Alastair D'Silva <[email protected]>
> > >
> > > 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 <[email protected]>
> > >
<snip>
> > 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" <[email protected]> writes:
> On Wed, 2018-02-21 at 17:43 +1100, Balbir Singh wrote:
>> On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <[email protected]
>> om> wrote:
>> > From: Alastair D'Silva <[email protected]>
>> >
...
>> > 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 <[email protected]> wrote:
>
> On Wed, 2018-02-21 at 17:43 +1100, Balbir Singh wrote:
>> On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <[email protected]
>> om> wrote:
>> > From: Alastair D'Silva <[email protected]>
>> >
>> > 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 <[email protected]>
>> > ---
>> > 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?
>
>> > +{
>> > + 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?
>
>> > +
>> > + 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
>> > + __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
<[email protected]> wrote:
>
>
> Le 21/02/2018 à 07:43, Balbir Singh a écrit :
>>
>> On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <[email protected]>
>> wrote:
>>>
>>> From: Alastair D'Silva <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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?
>>
>>> +{
>>> + 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
>>
>>> +
>>> + 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?
>>
>>> + __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 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.
>
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:
>> 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
[email protected] 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 <[email protected].
> com> wrote:
> >
> > On Wed, 2018-02-21 at 17:43 +1100, Balbir Singh wrote:
> > > On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <[email protected]
> > > bm.c
> > > om> wrote:
> > > > From: Alastair D'Silva <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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.
> >
> > > > +{
> > > > + 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.
> >
> > > > +
> > > > + 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
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
> > 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 <[email protected]> wrote:
> 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
>> > 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.