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

[Balbir Singh]

On Thu, Feb 22, 2018 at 10:32 AM, Alastair D'Silva <alastair@xxxxxxxxxxx> 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@xxxxxxxxx
>> om> wrote:
>> > From: Alastair D'Silva <alastair@xxxxxxxxxxx>
>> >
>> > 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@xxxxxxxxxxx>
>> > ---
>> >  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.