On Wed, 2018-02-21 at 17:43 +1100, Balbir Singh wrote:
> On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <alast...@au1.ibm.c
> om> wrote:
> > From: Alastair D'Silva <alast...@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 <alast...@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.
>
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australiamob: 0423 762 819
