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

[Alastair D'Silva]

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.

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