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

[Michael Ellerman <mpe@ellerman.id.au>]

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