[Skiboot] [PATCH V2 05/15] core/pldm: Decode the SetStateEffecterStates request

[Frederic Barrat]

On 29/04/2022 11:47, Christophe Lombard wrote:
> The SetStateEffecterStates command is used to set the state of one or more
> effecters within a PLDM State Effecter.
> The field comp_effecter_count indicates the number of individual sets of
> state effecter information that are accessed by this command.
> 
> The Event Receiver acknowledges receiving the PLDM Event Message in the
> response to this command.
> 
> Signed-off-by: Christophe Lombard <clombard at linux.vnet.ibm.com>
> ---
>   core/pldm/pldm-responder.c | 130 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 130 insertions(+)
> 
> diff --git a/core/pldm/pldm-responder.c b/core/pldm/pldm-responder.c
> index e44bb509..8c543b6c 100644
> --- a/core/pldm/pldm-responder.c
> +++ b/core/pldm/pldm-responder.c
> @@ -8,8 +8,11 @@
>   #include <opal.h>
>   #include <stdio.h>
>   #include <string.h>
> +#include <opal-msg.h>
> +#include <debug_descriptor.h>
>   #include <pldm/ibm/libpldm/platform_oem_ibm.h>
>   #include <pldm/libpldm/platform.h>
> +#include <pldm/libpldm/state_set.h>
>   #include <pldm/libpldm/utils.h>
>   #include "pldm.h"
>   
> @@ -647,6 +650,132 @@ static struct pldm_cmd pldm_platform_get_state_sensor_readings = {
>   	.handler = platform_get_state_sensor_readings,
>   };
>   
> +#define SOFT_OFF		0x00
> +#define SOFT_REBOOT		0x01
> +#define CHASSIS_PWR_DOWN	0x00
> +#define DEFAULT_CHIP_ID		0
> +
> +struct effecter_states_req {
> +	uint16_t effecter_id;
> +	uint8_t comp_effecter_count;
> +	set_effecter_state_field field[8];
> +};
> +
> +/*
> + * SetStateEffecterStates (0x39)
> + * The SetStateEffecterStates command is used to set the state of one
> + * or more effecters within a PLDM State Effecter.
> + */
> +static int platform_set_state_effecter_states_handler(const struct pldm_rx_data *req)
> +{
> +	struct effecter_states_req states_req;
> +	set_effecter_state_field *field;
> +	int rc, i;
> +
> +	/* decode SetStateEffecterStates request data */
> +	rc = decode_set_state_effecter_states_req(
> +				req->msg,
> +				PLDM_SET_STATE_EFFECTER_STATES_REQ_BYTES,
> +				&states_req.effecter_id,
> +				&states_req.comp_effecter_count,
> +				states_req.field);
> +	if (rc) {
> +		prlog(PR_ERR, "Failed to decode SetStateEffecterStates request, rc = %d\n", rc);
> +		pldm_cc_resp(req, req->hdrinf.pldm_type,
> +			     req->hdrinf.command, PLDM_ERROR);
> +		return OPAL_INTERNAL_ERROR;
> +	}
> +
> +	/* invoke the appropriate callback handler */
> +	prlog(PR_DEBUG, "%s - effecter_id: %d, comp_effecter_count: %d\n",
> +			__func__,
> +			states_req.effecter_id,
> +			states_req.comp_effecter_count);
> +


Should we check that we match the value of states_req.effecter_id? How 
do we find out our own effecter ID?


> +	for (i = 0; i < states_req.comp_effecter_count; i++) {
> +		field = states_req.field;


This will force to always look at the first effecter state (see below).


> +
> +		/* other set_request not supported */
> +		if (field->set_request != PLDM_REQUEST_SET) {
> +			prlog(PR_ERR, "Got invalid set request 0x%x in "
> +				      "SetStateEffecterStates request\n",
> +				      field->set_request);
> +			pldm_cc_resp(req, req->hdrinf.pldm_type,
> +				     req->hdrinf.command,
> +				     PLDM_PLATFORM_INVALID_STATE_VALUE);
> +			return OPAL_PARAMETER;
> +		}
> +
> +		switch (field->effecter_state) {
> +		case PLDM_SW_TERM_GRACEFUL_SHUTDOWN_REQUESTED:
> +		case PLDM_STATE_SET_SYS_POWER_STATE_OFF_SOFT_GRACEFUL:
> +			prlog(PR_NOTICE, "Soft shutdown requested\n");
> +			pldm_cc_resp(req, PLDM_PLATFORM,
> +				     PLDM_SET_STATE_EFFECTER_STATES,
> +				     PLDM_SUCCESS);
> +
> +			if (opal_booting() && platform.cec_power_down) {
> +				prlog(PR_NOTICE, "Host not up, shutting down now\n");
> +				platform.cec_power_down(CHASSIS_PWR_DOWN);
> +			} else {
> +				opal_queue_msg(OPAL_MSG_SHUTDOWN,
> +					       NULL, NULL,
> +					       cpu_to_be64(SOFT_OFF));
> +			}
> +
> +			break;
> +
> +		case PLDM_SW_TERM_GRACEFUL_RESTART_REQUESTED:
> +			prlog(PR_NOTICE, "Soft reboot requested\n");
> +			pldm_cc_resp(req, PLDM_PLATFORM,
> +				     PLDM_SET_STATE_EFFECTER_STATES,
> +				     PLDM_SUCCESS);
> +
> +			if (opal_booting() && platform.cec_reboot) {
> +				prlog(PR_NOTICE, "Host not up, rebooting now\n");
> +				platform.cec_reboot();
> +			} else {
> +				opal_queue_msg(OPAL_MSG_SHUTDOWN,
> +					       NULL, NULL,
> +					       cpu_to_be64(SOFT_REBOOT));
> +			}
> +
> +			break;
> +
> +		case PLDM_STATE_SET_BOOT_RESTART_CAUSE_WARM_RESET:
> +		case PLDM_STATE_SET_BOOT_RESTART_CAUSE_HARD_RESET:
> +			prlog(PR_NOTICE, "OCC reset requested\n");


I didn't see those in my logs. Did you manage to test those? From the 
FIXME below, it seems we still have to worry about multiple chips.


> +			pldm_cc_resp(req, PLDM_PLATFORM,
> +				     PLDM_SET_STATE_EFFECTER_STATES,
> +				     PLDM_SUCCESS);
> +
> +			/* invoke the appropriate callback handler */
> +			prd_occ_reset(DEFAULT_CHIP_ID); /* FIXME, others chip ? */
> +			break;
> +
> +		default:
> +			prlog(PR_ERR, "Got invalid effecter state 0x%x in "
> +				      "SetStateEffecterStates request\n",
> +				      field->effecter_state);
> +			pldm_cc_resp(req, req->hdrinf.pldm_type,
> +				     req->hdrinf.command,
> +				     PLDM_PLATFORM_INVALID_STATE_VALUE);
> +			return OPAL_PARAMETER;
> +		}
> +
> +		/* next */
> +		field++;


This is overwritten at the top of the loop so it's useless (and we 
process the same element over and over).

   Fred


> +	}
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +static struct pldm_cmd pldm_platform_set_state_effecter_states = {
> +	.name = "PLDM_SET_STATE_EFFECTER_STATES",
> +	.pldm_cmd_id = PLDM_SET_STATE_EFFECTER_STATES,
> +	.handler = platform_set_state_effecter_states_handler,
> +};
> +
>   int pldm_rx_handle_request(struct pldm_rx_data *rx)
>   {
>   	const struct pldm_type *t;
> @@ -692,6 +821,7 @@ int pldm_mctp_responder_init(void)
>   	pldm_add_cmd(&pldm_platform_type, &pldm_platform_set_event_receiver);
>   	pldm_add_cmd(&pldm_platform_type, &pldm_platform_event_message);
>   	pldm_add_cmd(&pldm_platform_type, &pldm_platform_get_state_sensor_readings);
> +	pldm_add_cmd(&pldm_platform_type, &pldm_platform_set_state_effecter_states);
>   
>   	return OPAL_SUCCESS;
>   }