On Wed, Feb 21, 2018 at 10:25 PM, Frederic Barrat
<fbar...@linux.vnet.ibm.com> wrote:
>
>
> Le 21/02/2018 à 07:43, Balbir Singh a écrit :
>>
>> On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <alast...@au1.ibm.com>
>> 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?
>>
>>> +{
>>> + 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
>>
>>> +
>>> + 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?
>>
>>> + __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?
>
>
> My bad, I forgot to mention it before. There's a somewhat high-level
> description which needs updating in:
> Documentation/accelerators/ocxl.rst
Thanks, that's helpful
>
> It doesn't go down to the level of the structure members, but at least all
> ioctl commands should have a brief description.
>
> lpc_size could be added. It's currently useless to the library, but doesn't
> hurt. The one which was giving me troubles on a previous version of this
> patch was the lpc numa node ID, since that was experimental code and felt
> out of place considering what's been upstreamed in skiboot and linux so far.
>
Yeah, I think metadata will evolve for a while till it settle's down.
Since ocxl_ioctl_get_metadata is exposed via uapi, a newer program
calling an older kernel will never work, since the size of that struct
will always be larger than what the OS supports and our copy_to_user()
will fail. The other option is for the user program to try all
possible versions till one succeeds, that is bad as well. I think
there are a few ways around it, if we care about this combination.
Balbir Singh.
