[v2,15/17] cxl: Userspace header file.

From: Ian Munsie <imunsie@au1.ibm.com>

This defines structs and magic numbers required for userspace to interact with
the kernel cxl driver via /dev/cxl/afu0.0.

It adds this header file Kbuild so it's exported when doing make
headers_installs.

Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 include/uapi/Kbuild      |  1 +
 include/uapi/misc/Kbuild |  2 ++
 include/uapi/misc/cxl.h  | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+)
 create mode 100644 include/uapi/misc/Kbuild
 create mode 100644 include/uapi/misc/cxl.h

On Tue, 2014-30-09 at 10:35:04 UTC, Michael Neuling wrote:
> From: Ian Munsie <imunsie@au1.ibm.com>
> 
> This defines structs and magic numbers required for userspace to interact with
> the kernel cxl driver via /dev/cxl/afu0.0.
> 
> diff --git a/include/uapi/misc/cxl.h b/include/uapi/misc/cxl.h
> new file mode 100644
> index 0000000..6a394b5
> --- /dev/null
> +++ b/include/uapi/misc/cxl.h
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright 2014 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#ifndef _UAPI_ASM_CXL_H
> +#define _UAPI_ASM_CXL_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +/* ioctls */
> +struct cxl_ioctl_start_work {
> +	__u64 wed;
> +	__u64 amr;
> +	__u64 reserved1;
> +	__u32 reserved2;
> +	__s16 num_interrupts; /* -1 = use value from afu descriptor */
> +	__u16 process_element; /* returned from kernel */
> +	__u64 reserved3;
> +	__u64 reserved4;
> +	__u64 reserved5;
> +	__u64 reserved6;

Why so many reserved fields?

What mechanism is there that will allow you to ever unreserve them?

ie. how does a new userspace detect that the kernel it's running on supports
new fields?

Or conversely how does a new kernel detect that userspace has passed it a
meaningful value in one of the previously reserved fields?

> +#define CXL_MAGIC 0xCA
> +#define CXL_IOCTL_START_WORK      _IOWR(CXL_MAGIC, 0x00, struct cxl_ioctl_start_work)

What happened to 0x1 ?

> +#define CXL_IOCTL_CHECK_ERROR     _IO(CXL_MAGIC,   0x02)
> +
> +/* events from read() */
> +
> +enum cxl_event_type {
> +	CXL_EVENT_READ_FAIL     = -1,

I don't see this used?

> +	CXL_EVENT_RESERVED      = 0,
> +	CXL_EVENT_AFU_INTERRUPT = 1,
> +	CXL_EVENT_DATA_STORAGE  = 2,
> +	CXL_EVENT_AFU_ERROR     = 3,
> +};
> +
> +struct cxl_event_header {
> +	__u32 type;
> +	__u16 size;
> +	__u16 process_element;
> +	__u64 reserved1;
> +	__u64 reserved2;
> +	__u64 reserved3;
> +};

Again lots of reserved fields?

> +struct cxl_event_afu_interrupt {
> +	struct cxl_event_header header;
> +	__u16 irq; /* Raised AFU interrupt number */
> +	__u16 reserved1;
> +	__u32 reserved2;
> +	__u64 reserved3;
> +	__u64 reserved4;
> +	__u64 reserved5;
> +};
> +
> +struct cxl_event_data_storage {
> +	struct cxl_event_header header;
> +	__u64 addr;
> +	__u64 reserved1;
> +	__u64 reserved2;
> +	__u64 reserved3;
> +};
> +
> +struct cxl_event_afu_error {
> +	struct cxl_event_header header;
> +	__u64 err;
> +	__u64 reserved1;
> +	__u64 reserved2;
> +	__u64 reserved3;
> +};
> +
> +struct cxl_event {
> +	union {
> +		struct cxl_event_header header;
> +		struct cxl_event_afu_interrupt irq;
> +		struct cxl_event_data_storage fault;
> +		struct cxl_event_afu_error afu_err;
> +	};
> +};

Rather than having the header included in every event, would it be clearer if
the cxl_event was:

struct cxl_event {
	struct cxl_event_header header;
	union {
		struct cxl_event_afu_interrupt irq;
		struct cxl_event_data_storage fault;
		struct cxl_event_afu_error afu_err;
	};
};

cheers

Hey Michael,

Excerpts from Michael Ellerman's message of 2014-10-02 16:02:37 +1000:
> > +/* ioctls */
> > +struct cxl_ioctl_start_work {
> > +    __u64 wed;
> > +    __u64 amr;
> > +    __u64 reserved1;
> > +    __u32 reserved2;
> > +    __s16 num_interrupts; /* -1 = use value from afu descriptor */
> > +    __u16 process_element; /* returned from kernel */
> > +    __u64 reserved3;
> > +    __u64 reserved4;
> > +    __u64 reserved5;
> > +    __u64 reserved6;
> 
> Why so many reserved fields?

The first two are reserved for the context save area (reserved1) and
size (reserved2) of the "shared" (AKA time sliced) virtualisation model,
which we don't yet support. That only leaves us with four reserved
fields for anything that we haven't thought of or that the hardware team
hasn't come up with yet ;-)

> What mechanism is there that will allow you to ever unreserve them?
>
> ie. how does a new userspace detect that the kernel it's running on supports
> new fields?

The ioctl will return -EINVAL if any of them are set to non-zero values,
so userspace can easily tell if it's running on an old kernel.

> Or conversely how does a new kernel detect that userspace has passed it a
> meaningful value in one of the previously reserved fields?

They would have to be non-zero (certainly true of the context save
area's size), or one could turn into a flags field or api version.

> > +#define CXL_MAGIC 0xCA
> > +#define CXL_IOCTL_START_WORK      _IOWR(CXL_MAGIC, 0x00, struct cxl_ioctl_start_work)
> 
> What happened to 0x1 ?

That was used to dynamically program the FPGA with a new AFU image, but
we don't have anything to test it on yet and I'm not convinced that the
procedure won't change by the time we do, so we pulled the code.

We can repack the ioctl numbers easily enough... Will do :)

> > +enum cxl_event_type {
> > +    CXL_EVENT_READ_FAIL     = -1,
> 
> I don't see this used?

That was used in the userspace library to mark it's buffer as bad if the
read() call failed for whatever reason... but you're right - it isn't
used by the kernel and doesn't belong in this header. Will remove.

> > +struct cxl_event_header {
> > +    __u32 type;
> > +    __u16 size;
> > +    __u16 process_element;
> > +    __u64 reserved1;
> > +    __u64 reserved2;
> > +    __u64 reserved3;
> > +};
> 
> Again lots of reserved fields?

Figured it was better to have a bit more than we expect we might need
just in case... We can reduce this if you feel it is excessive?

In an earlier version of the code the kernel would fill out the header
and not clear an event if a buffer was passed in that was too small, so
userspace could realloc a larger buffer and try again. This made the API
a bit more complex and our internal users weren't too keen on it, so we
decided to use a fixed-size buffer and make it larger than we strictly
needed so we have plenty of room for further expansion.

> Rather than having the header included in every event, would it be clearer if
> the cxl_event was:
> 
> struct cxl_event {
>     struct cxl_event_header header;
>     union {
>         struct cxl_event_afu_interrupt irq;
>         struct cxl_event_data_storage fault;
>         struct cxl_event_afu_error afu_err;
>     };
> };

Sounds like a good idea to me :)

Cheers,
-Ian

On Thu, 2014-10-02 at 20:28 +1000, Ian Munsie wrote:
> Hey Michael,
> 
> Excerpts from Michael Ellerman's message of 2014-10-02 16:02:37 +1000:
> > > +/* ioctls */
> > > +struct cxl_ioctl_start_work {
> > > +    __u64 wed;
> > > +    __u64 amr;
> > > +    __u64 reserved1;
> > > +    __u32 reserved2;
> > > +    __s16 num_interrupts; /* -1 = use value from afu descriptor */
> > > +    __u16 process_element; /* returned from kernel */
> > > +    __u64 reserved3;
> > > +    __u64 reserved4;
> > > +    __u64 reserved5;
> > > +    __u64 reserved6;
> > 
> > Why so many reserved fields?
> 
> The first two are reserved for the context save area (reserved1) and
> size (reserved2) of the "shared" (AKA time sliced) virtualisation model,
> which we don't yet support. That only leaves us with four reserved
> fields for anything that we haven't thought of or that the hardware team
> hasn't come up with yet ;-)
> 
> > What mechanism is there that will allow you to ever unreserve them?
> >
> > ie. how does a new userspace detect that the kernel it's running on supports
> > new fields?
> 
> The ioctl will return -EINVAL if any of them are set to non-zero values,
> so userspace can easily tell if it's running on an old kernel.

Not good enough in my experience. Throw in a flags field I'd say..

> > Or conversely how does a new kernel detect that userspace has passed it a
> > meaningful value in one of the previously reserved fields?
> 
> They would have to be non-zero (certainly true of the context save
> area's size), or one could turn into a flags field or api version.

If you go that way you need to negociate as well latest compatible
etc...

> > > +#define CXL_MAGIC 0xCA
> > > +#define CXL_IOCTL_START_WORK      _IOWR(CXL_MAGIC, 0x00, struct cxl_ioctl_start_work)
> > 
> > What happened to 0x1 ?
> 
> That was used to dynamically program the FPGA with a new AFU image, but
> we don't have anything to test it on yet and I'm not convinced that the
> procedure won't change by the time we do, so we pulled the code.
> 
> We can repack the ioctl numbers easily enough... Will do :)
> 
> > > +enum cxl_event_type {
> > > +    CXL_EVENT_READ_FAIL     = -1,
> > 
> > I don't see this used?
> 
> That was used in the userspace library to mark it's buffer as bad if the
> read() call failed for whatever reason... but you're right - it isn't
> used by the kernel and doesn't belong in this header. Will remove.
> 
> > > +struct cxl_event_header {
> > > +    __u32 type;
> > > +    __u16 size;
> > > +    __u16 process_element;
> > > +    __u64 reserved1;
> > > +    __u64 reserved2;
> > > +    __u64 reserved3;
> > > +};
> > 
> > Again lots of reserved fields?
> 
> Figured it was better to have a bit more than we expect we might need
> just in case... We can reduce this if you feel it is excessive?
> 
> In an earlier version of the code the kernel would fill out the header
> and not clear an event if a buffer was passed in that was too small, so
> userspace could realloc a larger buffer and try again. This made the API
> a bit more complex and our internal users weren't too keen on it, so we
> decided to use a fixed-size buffer and make it larger than we strictly
> needed so we have plenty of room for further expansion.
> 
> > Rather than having the header included in every event, would it be clearer if
> > the cxl_event was:
> > 
> > struct cxl_event {
> >     struct cxl_event_header header;
> >     union {
> >         struct cxl_event_afu_interrupt irq;
> >         struct cxl_event_data_storage fault;
> >         struct cxl_event_afu_error afu_err;
> >     };
> > };
> 
> Sounds like a good idea to me :)
> 
> Cheers,
> -Ian