ocxl: Add get_metadata IOCTL to share OCXL information to userspace

From: Alastair D'Silva <alastair@d-silva.org>

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 <alastair@d-silva.org>
---
 drivers/misc/ocxl/file.c | 27 +++++++++++++++++++++++++++
 include/uapi/misc/ocxl.h | 22 ++++++++++++++++++++++
 2 files changed, 49 insertions(+)

On 21/02/18 15:57, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> 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 <alastair@d-silva.org>

Seems fine.

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

On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> 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 <alastair@d-silva.org>
> ---
>  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 <alastair@au1.ibm.com> wrote:
>> From: Alastair D'Silva <alastair@d-silva.org>
>>
>> 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 <alastair@d-silva.org>
>> ---
>>   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 <alastair@au1.ibm.c
> om> wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> > 
> > 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 <alastair@d-silva.org>
> > ---
> >  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.
>

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 <alastair@au1.ibm
> > .com> wrote:
> > > From: Alastair D'Silva <alastair@d-silva.org>
> > > 
> > > 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 <alastair@d-silva.org>
> > > 
<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" <alastair@au1.ibm.com> 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 <alastair@au1.ibm.c
>> om> wrote:
>> > From: Alastair D'Silva <alastair@d-silva.org>
>> > 
...
>> > 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 <alastair@au1.ibm.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 <alastair@au1.ibm.c
>> om> wrote:
>> > From: Alastair D'Silva <alastair@d-silva.org>
>> >
>> > 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 <alastair@d-silva.org>
>> > ---
>> >  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
<fbarrat@linux.vnet.ibm.com> wrote:
>
>
> Le 21/02/2018 à 07:43, Balbir Singh a écrit :
>>
>> On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <alastair@au1.ibm.com>
>> wrote:
>>>
>>> From: Alastair D'Silva <alastair@d-silva.org>
>>>
>>> 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 <alastair@d-silva.org>
>>> ---
>>>   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.

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:
> > 
> > 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.i
> > > bm.c
> > > om> wrote:
> > > > From: Alastair D'Silva <alastair@d-silva.org>
> > > > 
> > > > 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 <alastair@d-silva.org>
> > > > ---
> > > >  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.

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).

On Thu, Feb 22, 2018 at 2:51 PM, Alastair D'Silva <alastair@au1.ibm.com> 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.

>>> 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