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

["Alastair D'Silva" <alastair@au1.ibm.com>]

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.

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