[Skiboot] [RFC PATCH] fsp/ipmi: Add the in-band IPMI support for FSP systems

Alistair Popple alistair at popple.id.au
Wed Jul 1 16:32:35 AEST 2015


Hi Neelesh,

A few comments below. The main question I have is do we even need to implement 
a queue in this driver, given the FSP already has a message queue?

Regards,

Alistair

On Wed, 24 Jun 2015 22:46:20 Neelesh Gupta wrote:
> FSP implements the IPMI commands support that can be accessed over
> the mailbox interface from the host. The host needs to provide and
> receive the message/data bytes of the IPMI command in the TCE space
> in the  KCS (Keyboard Controller Style) format.
> 
> Signed-off-by: Neelesh Gupta <neelegup at linux.vnet.ibm.com>
> ---
> 
> *** Note: I hit an issue testing this patch when fsp reboots after
> we sent a specific command. This issue seem to be related to fsp
> at the moment as fsp should behave sane even if I pass the wrong
> ipmi command by notifying the status through response.. debugging
> is in progress, meanwhile posting the RFC for a general review.
> Thanks.
>  
> 
>  hw/fsp/Makefile.inc        |    2 
>  hw/fsp/fsp-ipmi.c          |  313 
++++++++++++++++++++++++++++++++++++++++++++
>  include/errorlog.h         |    6 +
>  include/fsp.h              |    6 +
>  include/psi.h              |    4 +
>  platforms/ibm-fsp/common.c |    5 +
>  6 files changed, 335 insertions(+), 1 deletion(-)
>  create mode 100644 hw/fsp/fsp-ipmi.c
> 
> diff --git a/hw/fsp/Makefile.inc b/hw/fsp/Makefile.inc
> index d4654d5..3df832c 100644
> --- a/hw/fsp/Makefile.inc
> +++ b/hw/fsp/Makefile.inc
> @@ -4,7 +4,7 @@ FSP_OBJS  = fsp.o fsp-console.o fsp-rtc.o fsp-nvram.o fsp-
sysparam.o
>  FSP_OBJS += fsp-surveillance.o fsp-codeupdate.o fsp-sensor.o
>  FSP_OBJS += fsp-diag.o fsp-leds.o fsp-mem-err.o fsp-op-panel.o
>  FSP_OBJS += fsp-elog-read.o fsp-elog-write.o fsp-epow.o fsp-dpo.o
> -FSP_OBJS += fsp-dump.o fsp-mdst-table.o
> +FSP_OBJS += fsp-dump.o fsp-mdst-table.o fsp-ipmi.o
>  FSP_OBJS += fsp-attn.o
>  FSP = hw/fsp/built-in.o
>  $(FSP): $(FSP_OBJS:%=hw/fsp/%)
> diff --git a/hw/fsp/fsp-ipmi.c b/hw/fsp/fsp-ipmi.c
> new file mode 100644
> index 0000000..ccbff10
> --- /dev/null
> +++ b/hw/fsp/fsp-ipmi.c
> @@ -0,0 +1,313 @@
> +/* Copyright 2014-2015 IBM Corp.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *	http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <errorlog.h>
> +#include <fsp.h>
> +#include <ipmi.h>
> +#include <lock.h>
> +#include <opal-api.h>
> +
> +DEFINE_LOG_ENTRY(OPAL_RC_IPMI_REQ, OPAL_PLATFORM_ERR_EVT, OPAL_IPMI,
> +		 OPAL_PLATFORM_FIRMWARE, OPAL_PREDICTIVE_ERR_GENERAL,
> +		 OPAL_NA, NULL);
> +DEFINE_LOG_ENTRY(OPAL_RC_IPMI_RESP, OPAL_PLATFORM_ERR_EVT, OPAL_IPMI,
> +		 OPAL_PLATFORM_FIRMWARE, OPAL_PREDICTIVE_ERR_GENERAL,
> +		 OPAL_NA, NULL);
> +
> +struct fsp_ipmi_msg {
> +	struct list_node	link;
> +	struct ipmi_msg		ipmi_msg;
> +};
> +
> +static struct fsp_ipmi {
> +	struct list_head	msg_queue;
> +	void			*ipmi_req_buf;
> +	void			*ipmi_resp_buf;
> +	/* There can only be one outstanding request whose reference is stored
> +	 * in 'cur_msg' and the 'lock' protects against the concurrent updates
> +	 * of it through request and response. The same 'lock' also protects
> +	 * the list manipulation.
> +	 */
> +	struct fsp_ipmi_msg	*cur_msg;
> +	struct lock		lock;
> +} fsp_ipmi;
> +
> +static int fsp_ipmi_send_request(void);
> +
> +static void fsp_ipmi_req_complete(struct fsp_msg *msg)
> +{
> +	struct fsp_ipmi_msg *fsp_ipmi_msg = msg->user_data;
> +	uint8_t status = (msg->resp->word1 >> 8) & 0xff;

GETFIELD (in include/bitutils.h) works well for these types of things.

> +	uint32_t length = msg->resp->data.words[0];
> +	struct ipmi_msg *ipmi_msg;
> +
> +	assert(fsp_ipmi_msg == fsp_ipmi.cur_msg);
> +
> +	fsp_freemsg(msg);
> +	ipmi_msg = &fsp_ipmi_msg->ipmi_msg;
> +
> +	if (status != FSP_STATUS_SUCCESS) {
> +		log_simple_error(&e_info(OPAL_RC_IPMI_REQ), "IPMI: Request "
> +				 "failed with status:0x%02x\n", status);
> +		/* FSP will not send the response now so clear the current
> +		 * outstanding request
> +		 */
> +		ipmi_cmd_done(ipmi_msg->cmd, ipmi_msg->netfn + (1 << 2),

I should add something to reformat the netfn rather than having to code it 
everytime, but this should be fine for the moment.

> +			      IPMI_INVALID_COMMAND_ERR, ipmi_msg);
> +		lock(&fsp_ipmi.lock);
> +		fsp_ipmi.cur_msg = NULL;

Is there really a need to keep a reference to current message? Once it's in 
the FSP queue can't we just forget about it? This would avoid this 
lock/unlock.

> +		unlock(&fsp_ipmi.lock);
> +
> +		/* Send the next request in the queue */
> +		fsp_ipmi_send_request();
> +		return;
> +	}
> +
> +	if (length != ipmi_msg->req_size)
> +		prlog(PR_DEBUG, "IPMI: Length mismatch in req completion "
> +		      "(%d, %d)\n", ipmi_msg->req_size, length);
> +}
> +
> +static int fsp_ipmi_send_request(void)
> +{
> +	uint8_t *req_buf = fsp_ipmi.ipmi_req_buf;
> +	struct ipmi_msg *ipmi_msg;
> +	struct fsp_msg *msg;
> +	int rc;
> +
> +	lock(&fsp_ipmi.lock);
> +	/* An outstanding request is still pending */
> +	if (fsp_ipmi.cur_msg) {
> +		unlock(&fsp_ipmi.lock);
> +		return 0;
> +	}
> +
> +	fsp_ipmi.cur_msg = list_top(&fsp_ipmi.msg_queue, struct fsp_ipmi_msg,
> +				    link);

Minor style comment, you could just do this here...
	unlock(&fsp_ipmi.lock);


> +	if (!fsp_ipmi.cur_msg) {
> +		unlock(&fsp_ipmi.lock);
> +		return 0;
> +	}
> +	unlock(&fsp_ipmi.lock);

... and remove the two unlocks here.

> +
> +	ipmi_msg = &fsp_ipmi.cur_msg->ipmi_msg;
> +	prlog(PR_TRACE, "IPMI: Send request, netfn:0x%02x, cmd:0x%02x, "
> +	      "req_len:%d\n", ipmi_msg->netfn, ipmi_msg->cmd, ipmi_msg-
>req_size);
> +
> +	/* FSP implements KCS (Keyboard Controller Style) interface, 
underneath
> +	 * the mailbox transactions for IPMI
> +	 */
> +	*req_buf++ = ipmi_msg->netfn;	/* BYTE 1 */
> +	*req_buf++ = ipmi_msg->cmd;	/* BYTE 2 */
> +	memcpy(req_buf, ipmi_msg->data, ipmi_msg->req_size);
> +
> +	msg = fsp_mkmsg(FSP_CMD_FETCH_PLAT_DATA, 5, 0, PSI_DMA_PLAT_REQ_BUF,
> +			0, PSI_DMA_PLAT_RESP_BUF, ipmi_msg->req_size);
> +	if (!msg) {
> +		log_simple_error(&e_info(OPAL_RC_IPMI_REQ), "IPMI: Failed to "
> +				 "allocate request message\n");
> +		return OPAL_NO_MEM;
> +	}
> +
> +	msg->user_data = fsp_ipmi.cur_msg;
> +	rc = fsp_queue_msg(msg, fsp_ipmi_req_complete);

Do you actually even need to implement a queue in this driver? If the FSP 
already has a message queue why not just reformat the ipmi message into an FSP 
message and add it to the FSP queue? The only reason the BT driver implements 
a queue is because it is the thing managing the hardware. In this case it 
already looks like there is FSP code to manage queuing/unqueuing requests. 
This should also remove the need for locks here.

> +	if (rc) {
> +		fsp_freemsg(msg);
> +		log_simple_error(&e_info(OPAL_RC_IPMI_REQ), "IPMI: Failed to "
> +				 "queue request message (%d)\n", rc);
> +		return OPAL_INTERNAL_ERROR;
> +	}
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +static struct ipmi_msg *fsp_ipmi_alloc_msg(size_t req_size, size_t 
resp_size)
> +{
> +	struct fsp_ipmi_msg *fsp_ipmi_msg;
> +	struct ipmi_msg *ipmi_msg;
> +
> +	fsp_ipmi_msg = zalloc(sizeof(*fsp_ipmi_msg) + MAX(req_size, 
resp_size));
> +	if (!fsp_ipmi_msg)
> +		return NULL;
> +
> +	ipmi_msg = &fsp_ipmi_msg->ipmi_msg;
> +
> +	ipmi_msg->req_size = req_size;
> +	ipmi_msg->resp_size = resp_size;
> +	ipmi_msg->data = (uint8_t *)fsp_ipmi_msg + 1;
> +
> +	return ipmi_msg;
> +}
> +
> +static void fsp_ipmi_free_msg(struct ipmi_msg *ipmi_msg)
> +{
> +	struct fsp_ipmi_msg *fsp_ipmi_msg = container_of(ipmi_msg,
> +			struct fsp_ipmi_msg, ipmi_msg);
> +
> +	free(fsp_ipmi_msg);
> +}

These two functions look familiar, we should create some helper functions that 
could be used by both BT and FSP backend drivers.

> +static int fsp_ipmi_queue_msg(struct ipmi_msg *ipmi_msg)
> +{
> +	struct fsp_ipmi_msg *fsp_ipmi_msg = container_of(ipmi_msg,
> +			struct fsp_ipmi_msg, ipmi_msg);
> +
> +	lock(&fsp_ipmi.lock);
> +	list_add_tail(&fsp_ipmi.msg_queue, &fsp_ipmi_msg->link);

Again, can you just queue it in the FSP driver instead of creating another 
queue?

> +	unlock(&fsp_ipmi.lock);
> +
> +	return fsp_ipmi_send_request();
> +}
> +
> +static int fsp_ipmi_queue_msg_head(struct ipmi_msg *ipmi_msg)
> +{
> +	struct fsp_ipmi_msg *fsp_ipmi_msg = container_of(ipmi_msg,
> +			struct fsp_ipmi_msg, ipmi_msg);
> +	int rc;
> +
> +	lock(&fsp_ipmi.lock);
> +	list_add(&fsp_ipmi.msg_queue, &fsp_ipmi_msg->link);
> +	unlock(&fsp_ipmi.lock);
> +
> +	rc = fsp_ipmi_send_request();
> +
> +	return rc;
> +}
> +
> +static int fsp_ipmi_dequeue_msg(struct ipmi_msg *ipmi_msg)
> +{
> +	struct fsp_ipmi_msg *fsp_ipmi_msg = container_of(ipmi_msg,
> +			struct fsp_ipmi_msg, ipmi_msg);
> +
> +	lock(&fsp_ipmi.lock);
> +	list_del(&fsp_ipmi_msg->link);
> +	unlock(&fsp_ipmi.lock);
> +
> +	return 0;
> +}
> +
> +static struct ipmi_backend fsp_ipmi_backend = {
> +	.alloc_msg	= fsp_ipmi_alloc_msg,
> +	.free_msg	= fsp_ipmi_free_msg,
> +	.queue_msg	= fsp_ipmi_queue_msg,
> +	.queue_msg_head	= fsp_ipmi_queue_msg_head,
> +	.dequeue_msg	= fsp_ipmi_dequeue_msg,
> +};
> +
> +static bool fsp_ipmi_send_response(uint32_t cmd)
> +{
> +	struct fsp_msg *resp;
> +	int rc;
> +
> +	resp = fsp_mkmsg(cmd, 0);
> +	if (!resp) {
> +		log_simple_error(&e_info(OPAL_RC_IPMI_RESP), "IPMI: Failed to 
"
> +				 "allocate response message\n");
> +		return false;
> +	}
> +
> +	rc = fsp_queue_msg(resp, fsp_freemsg);
> +	if (rc) {
> +		fsp_freemsg(resp);
> +		log_simple_error(&e_info(OPAL_RC_IPMI_RESP), "IPMI: Failed to 
"
> +				 "queue response message\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static bool fsp_ipmi_read_response(struct fsp_msg *msg)
> +{
> +	struct fsp_ipmi_msg *fsp_ipmi_msg = fsp_ipmi.cur_msg;
> +	struct ipmi_msg *ipmi_msg = &fsp_ipmi_msg->ipmi_msg;
> +	uint8_t *resp_buf = fsp_ipmi.ipmi_resp_buf;
> +	uint32_t resp_status = msg->data.words[3];
> +	uint32_t length = msg->data.words[2];
> +	uint8_t netfn, cmd, cc;
> +
> +	/* Response TCE token */
> +	assert(msg->data.words[1] == PSI_DMA_PLAT_RESP_BUF);
> +
> +	if (resp_status != FSP_STATUS_SUCCESS)
> +		return fsp_ipmi_send_response(FSP_RSP_PLAT_DATA |
> +					     FSP_STATUS_GENERIC_ERROR);
> +
> +	/* KCS response message format */
> +	netfn = *resp_buf++;
> +	cmd = *resp_buf++;
> +	cc = *resp_buf++;
> +
> +	prlog(PR_TRACE, "IPMI: fsp response received, netfn:0x%02x, 
cmd:0x%02x,"
> +	      " cc:0x%02x, length:%d\n", netfn, cmd, cc, length);
> +
> +	if (length > ipmi_msg->resp_size) {
> +		prlog(PR_TRACE, "IPMI: Length mismatch in response (%d, 
%d)\n",
> +		      length, ipmi_msg->resp_size);
> +		length = ipmi_msg->resp_size; /* Truncate */
> +		cc = IPMI_ERR_MSG_TRUNCATED;
> +	}
> +
> +	ipmi_msg->resp_size = length;
> +	memcpy(ipmi_msg->data, resp_buf, length);
> +
> +	ipmi_cmd_done(cmd, netfn, cc, ipmi_msg);
> +
> +	return fsp_ipmi_send_response(FSP_RSP_PLAT_DATA);
> +}
> +
> +static bool fsp_ipmi_response(uint32_t cmd_sub_mod, struct fsp_msg *msg)
> +{
> +	bool rc;
> +
> +	assert(fsp_ipmi.cur_msg);
> +
> +	switch (cmd_sub_mod) {
> +	case FSP_CMD_SEND_PLAT_DATA:
> +		prlog(PR_TRACE, "FSP_CMD_SEND_PLAT_DATA command received\n");
> +		rc = fsp_ipmi_read_response(msg);
> +		break;
> +	default:
> +		return false;
> +	};
> +
> +	lock(&fsp_ipmi.lock);
> +	fsp_ipmi.cur_msg = NULL;
> +	unlock(&fsp_ipmi.lock);
> +
> +	/* Send the next request in the queue */
> +	fsp_ipmi_send_request();
> +
> +	return rc;
> +}
> +
> +static struct fsp_client fsp_ipmi_client = {
> +	.message = fsp_ipmi_response,
> +};
> +
> +void fsp_ipmi_init(void)
> +{
> +	fsp_tce_map(PSI_DMA_PLAT_REQ_BUF, fsp_ipmi.ipmi_req_buf,
> +		    PSI_DMA_PLAT_REQ_BUF_SIZE);
> +	fsp_tce_map(PSI_DMA_PLAT_RESP_BUF, fsp_ipmi.ipmi_resp_buf,
> +		    PSI_DMA_PLAT_RESP_BUF_SIZE);
> +
> +	list_head_init(&fsp_ipmi.msg_queue);
> +	init_lock(&fsp_ipmi.lock);
> +
> +	fsp_register_client(&fsp_ipmi_client, FSP_MCLASS_FETCH_SPDATA);
> +	ipmi_register_backend(&fsp_ipmi_backend);
> +}
> diff --git a/include/errorlog.h b/include/errorlog.h
> index 49c60cf..295acf6 100644
> --- a/include/errorlog.h
> +++ b/include/errorlog.h
> @@ -192,6 +192,7 @@ struct opal_err_info {
>  #define OPAL_SLW			0x534C	/* SL */
>  #define OPAL_FSP			0x4650	/* FP */
>  #define OPAL_I2C			0x4943	/* IC */
> +#define OPAL_IPMI			0x4950  /* IP */
>  
>  /* SAPPHIRE SRC componenet ID*/
>  #define OPAL_CU				0x1000
> @@ -223,6 +224,7 @@ struct opal_err_info {
>  #define OPAL_SL				0x2100
>  #define OPAL_FP				0x2200
>  #define OPAL_IC				0x2300
> +#define OPAL_IP				0x2400
>  
>  enum opal_reasoncode {
>  /* code update */
> @@ -317,6 +319,10 @@ enum opal_reasoncode {
>  	OPAL_RC_I2C_TIMEOUT	= OPAL_IC | 0x12,
>  	OPAL_RC_I2C_TRANSFER	= OPAL_IC | 0x13,
>  	OPAL_RC_I2C_RESET	= OPAL_IC | 0x14,
> +
> +/* IPMI */
> +	OPAL_RC_IPMI_REQ	= OPAL_IP | 0x10,
> +	OPAL_RC_IPMI_RESP	= OPAL_IP | 0x11,
>  };
>  
>  #define DEFINE_LOG_ENTRY(reason, type, id, subsys,			\
> diff --git a/include/fsp.h b/include/fsp.h
> index 5689798..c6f0d8a 100644
> --- a/include/fsp.h
> +++ b/include/fsp.h
> @@ -421,6 +421,9 @@
>   */
>  #define FSP_CMD_FETCH_SP_DATA	0x1d40101 /* HV->FSP: Fetch & DMA data 
*/
>  #define FSP_CMD_WRITE_SP_DATA	0x1d40201 /* HV->FSP: Fetch & DMA data 
*/
> +#define FSP_CMD_FETCH_PLAT_DATA	0x1d40500 /* HV->FSP: Platform function 
data */
> +#define FSP_CMD_SEND_PLAT_DATA	0x0d40501 /* FSP->HV */
> +#define FSP_RSP_PLAT_DATA	0x0d48500 /* HV->FSP */
>  
>  /* Data set IDs for SP data commands */
>  #define FSP_DATASET_SP_DUMP	0x01
> @@ -787,6 +790,9 @@ extern int (*fsp_flash_term_hook)(void);
>  extern void fsp_init_surveillance(void);
>  extern void fsp_surv_query(void);
>  
> +/* IPMI */
> +extern void fsp_ipmi_init(void);
> +
>  /* Reset/Reload */
>  extern void fsp_reinit_fsp(void);
>  extern void fsp_trigger_reset(void);
> diff --git a/include/psi.h b/include/psi.h
> index 62643aa..36240e1 100644
> --- a/include/psi.h
> +++ b/include/psi.h
> @@ -193,6 +193,10 @@
>  #define PSI_DMA_MEMCONS_SZ		0x00001000
>  #define PSI_DMA_LOG_BUF			0x03200000
>  #define PSI_DMA_LOG_BUF_SZ		0x00100000 /* INMEM_CON_LEN */
> +#define PSI_DMA_PLAT_REQ_BUF		0x03300000
> +#define PSI_DMA_PLAT_REQ_BUF_SIZE	0x00001000
> +#define PSI_DMA_PLAT_RESP_BUF		0x03301000
> +#define PSI_DMA_PLAT_RESP_BUF_SIZE	0x00001000
>  
>  /* P8 only mappings */
>  #define PSI_DMA_TRACE_BASE		0x04000000
> diff --git a/platforms/ibm-fsp/common.c b/platforms/ibm-fsp/common.c
> index d993b95..3068b2b 100644
> --- a/platforms/ibm-fsp/common.c
> +++ b/platforms/ibm-fsp/common.c
> @@ -21,6 +21,7 @@
>  #include <opal.h>
>  #include <console.h>
>  #include <hostservices.h>
> +#include <ipmi.h>
>  
>  #include "ibm-fsp.h"
>  
> @@ -122,6 +123,10 @@ void ibm_fsp_init(void)
>  	op_display(OP_LOG, OP_MOD_INIT, 0x0002);
>  	fsp_init_surveillance();
>  
> +	/* IPMI */
> +	fsp_ipmi_init();
> +	ipmi_opal_init();
> +
>  	/* Initialize sensor access */
>  	op_display(OP_LOG, OP_MOD_INIT, 0x0003);
>  	fsp_init_sensor();
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
> 



More information about the Skiboot mailing list