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

[Balbir Singh]

On Wed, Feb 21, 2018 at 10:25 PM, Frederic Barrat
<fbarrat@xxxxxxxxxxxxxxxxxx> wrote:
>
>
> Le 21/02/2018 Ã 07:43, Balbir Singh a Ãcrit :
>>
>> On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <alastair@xxxxxxxxxxx>
>> 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?
>>
>>> +{
>>> +       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.