Re: [PATCH v3 10/18] cxl: New hcalls to support CAPI adapters

[Ian Munsie <imunsie@au1.ibm.com>]

> +#define H_CONTROL_CA_FUNCTION_RESET                   1 /* perform a reset */
> +#define H_CONTROL_CA_FUNCTION_SUSPEND_PROCESS         2 /* suspend a process from being executed */
> +#define H_CONTROL_CA_FUNCTION_RESUME_PROCESS          3 /* resume a process to be executed */
> +#define H_CONTROL_CA_FUNCTION_READ_ERR_STATE          4 /* read the error state */
> +#define H_CONTROL_CA_FUNCTION_GET_AFU_ERR             5 /* collect the AFU error buffer */
> +#define H_CONTROL_CA_FUNCTION_GET_CONFIG              6 /* collect configuration record */
> +#define H_CONTROL_CA_FUNCTION_GET_DOWNLOAD_STATE      7 /* query to return download status */
> +#define H_CONTROL_CA_FUNCTION_TERMINATE_PROCESS       8 /* terminate the process before completion */
> +#define H_CONTROL_CA_FUNCTION_COLLECT_VPD             9 /* collect VPD */
> +#define H_CONTROL_CA_FUNCTION_GET_FUNCTION_ERR_INT   11 /* read the function-wide error data based on an interrupt */
> +#define H_CONTROL_CA_FUNCTION_ACK_FUNCTION_ERR_INT   12 /* acknowledge function-wide error data based on an interrupt */
> +#define H_CONTROL_CA_FUNCTION_GET_ERROR_LOG          13 /* retrieve the Platform Log ID (PLID) of an error log */
> +
> +#define H_CONTROL_CA_FAULTS_RESPOND_PSL    1
> +#define H_CONTROL_CA_FAULTS_RESPOND_AFU    2
> +
> +#define OP_STR_AFU(x)                            \
> +    (x == H_CONTROL_CA_FUNCTION_RESET ? "RESET" :            \
> +        x == H_CONTROL_CA_FUNCTION_RESUME_PROCESS ? "PROCESS" :    \
> +        x == H_CONTROL_CA_FUNCTION_RESUME_PROCESS ? "RESUME" :    \

H_CONTROL_CA_FUNCTION_RESUME_PROCESS is listed twice here, and the names
don't match.

> +        x == H_CONTROL_CA_FUNCTION_READ_ERR_STATE ? "READ_ERR_STATE" : \
> +        x == H_CONTROL_CA_FUNCTION_GET_AFU_ERR ? "GET_AFU_ERR" : \
> +        x == H_CONTROL_CA_FUNCTION_GET_CONFIG ? "GET_CONFIG" :    \
> +        x == H_CONTROL_CA_FUNCTION_GET_DOWNLOAD_STATE ? "DOWNLOAD_STATE" : \
> +        x == H_CONTROL_CA_FUNCTION_TERMINATE_PROCESS ? "TERMINATE_PROC" : \
> +        x == H_CONTROL_CA_FUNCTION_COLLECT_VPD ? "COLLECT_VPD" :    \
> +        x == H_CONTROL_CA_FUNCTION_GET_FUNCTION_ERR_INT ? "GET_ERR_INTERRUPT" :    \
> +        x == H_CONTROL_CA_FUNCTION_ACK_FUNCTION_ERR_INT ? "ACK_ERR_INTERRUPT" :    \
> +        x == H_CONTROL_CA_FUNCTION_GET_ERROR_LOG ? "GET_ERROR_LOG" : \
> +        "UNKNOWN OP")
> +
> +#define H_CONTROL_CA_FACILITY_RESET                   1 /* perform a reset */
> +#define H_CONTROL_CA_FACILITY_COLLECT_VPD             2 /* collect VPD */
> +
> +#define OP_STR_CONTROL_ADAPTER(x)                    \
> +    (x == H_CONTROL_CA_FACILITY_RESET ? "RESET" :            \
> +        x == H_CONTROL_CA_FACILITY_COLLECT_VPD ? "COLLECT_VPD" : \
> +        "UNKNOWN OP")
> +
> +#define H_CONTROL_CA_FACILITY_DOWNLOAD                1 /* download adapter image */
> +#define H_CONTROL_CA_FACILITY_VALIDATE                2 /* validate adapter image */
> +
> +#define OP_STR_DOWNLOAD_ADAPTER(x)                    \
> +    (x == H_CONTROL_CA_FACILITY_DOWNLOAD ? "DOWNLOAD" :        \
> +        x == H_CONTROL_CA_FACILITY_VALIDATE ? "VALIDATE" :    \
> +        "UNKNOWN OP")
> +

Since these are almost sequential how about using static arrays and a
lookup helper, like:

static char *afu_op_names[] = {
	"UNKNOWN_OP",           /* 0 undefined */
	"RESET",                /* 1 */
	"SUSPEND_PROCESS",      /* 2 */
	"RESUME_PROCESS",       /* 3 */
	"READ_ERR_STATE",       /* 4 */
	"GET_AFU_ERR",          /* 5 */
	"GET_CONFIG",           /* 6 */
	"GET_DOWNLOAD_STATE",   /* 7 */
	"TERMINATE_PROCESS",    /* 8 */
	"COLLECT_VPD",          /* 9 */
	"UNKNOWN_OP",           /* 10 undefined */
	"GET_FUNCTION_ERR_INT", /* 11 */
	"ACK_FUNCTION_ERR_INT", /* 12 */
	"GET_ERROR_LOG",        /* 13 */
};

static char *control_adapter_op_names[] = {
	"UNKNOWN_OP",           /* 0 undefined */
	"RESET",                /* 1 */
	"COLLECT_VPD",          /* 2 */
};

static char *download_op_names[] = {
	"UNKNOWN_OP",           /* 0 undefined */
	"DOWNLOAD",             /* 1 */
	"VALIDATE",             /* 2 */
};

static char* op_str(unsigned int op, char *name_array[], int array_len)
{
	if (op >= array_len)
		return "UNKNOWN_OP";
	return name_array[op];
}

#define OP_STR(op, name_array)      op_str(op, name_array, ARRAY_SIZE(name_array))

#define OP_STR_AFU(op)              OP_STR(op, afu_op_names)
#define OP_STR_CONTROL_ADAPTER(op)  OP_STR(op, control_adapter_op_names)
#define OP_STR_DOWNLOAD_ADAPTER(op) OP_STR(op, download_op_names)

> +    buf = (u32 *) element;
> +    for (i = 0; i*4 < sizeof(struct cxl_process_element_hcall); i += 4) {
> +        if ((i+3)*4 < sizeof(struct cxl_process_element_hcall))
> +            pr_devel("%.8x %.8x %.8x %.8x\n", buf[i], buf[i + 1], buf[i + 2], buf[i + 3]);
> +        else if ((i+2)*4 < sizeof(struct cxl_process_element_hcall))
> +            pr_devel("%.8x %.8x %.8x\n", buf[i], buf[i + 1], buf[i + 2]);
> +        else if ((i+1)*4 < sizeof(struct cxl_process_element_hcall))
> +            pr_devel("%.8x %.8x\n", buf[i], buf[i + 1]);
> +        else
> +            pr_devel("%.8x\n", buf[i]);

For something that is only used with pr_devel for debugging I'd like
something that is a little easier on the eyes, like:

or (i = 0; i < sizeof(struct cxl_process_element_hcall); i++) {
	if (i && i % 4 == 0)
		pr_devel("\n");
	pr_devel("%08x ", buf[i]);
}
pr_devel("\n");



> +/*
> + * This is straight out of PAPR, but replacing some of the compound fields with
> + * a single field, where they were identical to the register layout.
> + *
> + * The 'flags' parameter regroups the various bit-fields
> + */

Thanks for getting rid of the bit fields :)

Cheers,
-Ian