On Thu, Feb 22, 2018 at 10:32 AM, Alastair D'Silva <alast...@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 <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.
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.
