diff mbox series

[4/5] discover/platform-powerpc: add mailbox message structure

Message ID 20190626123701.GA16687@gmail.com
State Not Applicable, archived
Headers show
Series [1/5] discover/platform-powerpc: add missing mbox block selector | expand

Commit Message

Maxim Polyakov June 26, 2019, 12:37 p.m. UTC
Use structure for the IPMI response mailbox message instead of raw byte
array as its done in the ipmitool utility:
https://github.com/ipmitool/ipmitool/commit/62a04390e10f8e62ce16b7bc95bf6ced419b80eb

Signed-off-by: Maxim Polyakov <m.polyakov@yadro.com>
---
 discover/platform-powerpc.c | 83 ++++++++++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 31 deletions(-)

Comments

Geoff Levand June 28, 2019, 3:30 p.m. UTC | #1
Hi Maxim,

On 6/26/19 5:37 AM, Maxim Polyakov wrote:
> Use structure for the IPMI response mailbox message instead of raw byte
> array as its done in the ipmitool utility:
> https://github.com/ipmitool/ipmitool/commit/62a04390e10f8e62ce16b7bc95bf6ced419b80eb
> 
> Signed-off-by: Maxim Polyakov <m.polyakov@yadro.com>
> ---
>  discover/platform-powerpc.c | 83 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 52 insertions(+), 31 deletions(-)
> 
> diff --git a/discover/platform-powerpc.c b/discover/platform-powerpc.c
> index c874560..6500058 100644
> --- a/discover/platform-powerpc.c
> +++ b/discover/platform-powerpc.c
> @@ -22,6 +22,29 @@
>  #include "ipmi.h"
>  #include "dt.h"
>  
> +#define CHASSIS_BOOT_MBOX_IANA_SZ 3
> +#define CHASSIS_BOOT_MBOX_DATA_SZ 16
> +#define CHASSIS_BOOT_MBOX_BLOCK0_DATA_SZ \
> +	(CHASSIS_BOOT_MBOX_DATA_SZ - CHASSIS_BOOT_MBOX_IANA_SZ)
> +
> +typedef struct __attribute__((packed)) {
> +	uint8_t iana[CHASSIS_BOOT_MBOX_IANA_SZ];
> +	uint8_t data[CHASSIS_BOOT_MBOX_BLOCK0_DATA_SZ];
> +} mbox_block0_t;
> +
> +typedef union {
> +		uint8_t data[CHASSIS_BOOT_MBOX_DATA_SZ];
> +		mbox_block0_t b0;
> +} mbox_t;
> +
> +typedef struct __attribute__((packed)) {
> +	uint8_t cc;
> +	uint8_t param_version;
> +	uint8_t param_valid;
> +	uint8_t block_selector;
> +	mbox_t mbox;
> +} ipmi_mbox_response_t;
> +

It seems these definitions are not specific to powerpc.  Should
they go in discover/ipmi.h?

I think in general a lot of the ipmi code in platform-powerpc.c
can me moved out to ipmi.c or some other generic file.

-Geoff
Maxim Polyakov July 8, 2019, 9:04 a.m. UTC | #2
Hi Geoff, 

28.06.2019 18:30, Geoff Levand пишет:
> Hi Maxim,
> 
> On 6/26/19 5:37 AM, Maxim Polyakov wrote:
>> Use structure for the IPMI response mailbox message instead of raw byte
>> array as its done in the ipmitool utility:
>> https://github.com/ipmitool/ipmitool/commit/62a04390e10f8e62ce16b7bc95bf6ced419b80eb
>>
>> Signed-off-by: Maxim Polyakov <m.polyakov@yadro.com>
>> ---
>>  discover/platform-powerpc.c | 83 ++++++++++++++++++++++++++++-----------------
>>  1 file changed, 52 insertions(+), 31 deletions(-)
>>
>> diff --git a/discover/platform-powerpc.c b/discover/platform-powerpc.c
>> index c874560..6500058 100644
>> --- a/discover/platform-powerpc.c
>> +++ b/discover/platform-powerpc.c
>> @@ -22,6 +22,29 @@
>>  #include "ipmi.h"
>>  #include "dt.h"
>>  
>> +#define CHASSIS_BOOT_MBOX_IANA_SZ 3
>> +#define CHASSIS_BOOT_MBOX_DATA_SZ 16
>> +#define CHASSIS_BOOT_MBOX_BLOCK0_DATA_SZ \
>> +	(CHASSIS_BOOT_MBOX_DATA_SZ - CHASSIS_BOOT_MBOX_IANA_SZ)
>> +
>> +typedef struct __attribute__((packed)) {
>> +	uint8_t iana[CHASSIS_BOOT_MBOX_IANA_SZ];
>> +	uint8_t data[CHASSIS_BOOT_MBOX_BLOCK0_DATA_SZ];
>> +} mbox_block0_t;
>> +
>> +typedef union {
>> +		uint8_t data[CHASSIS_BOOT_MBOX_DATA_SZ];
>> +		mbox_block0_t b0;
>> +} mbox_t;
>> +
>> +typedef struct __attribute__((packed)) {
>> +	uint8_t cc;
>> +	uint8_t param_version;
>> +	uint8_t param_valid;
>> +	uint8_t block_selector;
>> +	mbox_t mbox;
>> +} ipmi_mbox_response_t;
>> +
> 
> It seems these definitions are not specific to powerpc.  Should
> they go in discover/ipmi.h?

Ok, I move this code to ipmi.h in the patch v2.

> 
> I think in general a lot of the ipmi code in platform-powerpc.c
> can me moved out to ipmi.c or some other generic file.
> 
> -Geoff
>
diff mbox series

Patch

diff --git a/discover/platform-powerpc.c b/discover/platform-powerpc.c
index c874560..6500058 100644
--- a/discover/platform-powerpc.c
+++ b/discover/platform-powerpc.c
@@ -22,6 +22,29 @@ 
 #include "ipmi.h"
 #include "dt.h"
 
+#define CHASSIS_BOOT_MBOX_IANA_SZ 3
+#define CHASSIS_BOOT_MBOX_DATA_SZ 16
+#define CHASSIS_BOOT_MBOX_BLOCK0_DATA_SZ \
+	(CHASSIS_BOOT_MBOX_DATA_SZ - CHASSIS_BOOT_MBOX_IANA_SZ)
+
+typedef struct __attribute__((packed)) {
+	uint8_t iana[CHASSIS_BOOT_MBOX_IANA_SZ];
+	uint8_t data[CHASSIS_BOOT_MBOX_BLOCK0_DATA_SZ];
+} mbox_block0_t;
+
+typedef union {
+		uint8_t data[CHASSIS_BOOT_MBOX_DATA_SZ];
+		mbox_block0_t b0;
+} mbox_t;
+
+typedef struct __attribute__((packed)) {
+	uint8_t cc;
+	uint8_t param_version;
+	uint8_t param_valid;
+	uint8_t block_selector;
+	mbox_t mbox;
+} ipmi_mbox_response_t;
+
 static const char *partition = "common";
 static const char *sysparams_dir = "/sys/firmware/opal/sysparams/";
 static const char *devtree_dir = "/proc/device-tree/";
@@ -437,10 +460,10 @@  static int get_ipmi_bootdev_ipmi(struct platform_powerpc *platform,
 }
 
 static int get_ipmi_boot_mailbox_block(struct platform_powerpc *platform,
-		char *buf, uint8_t block)
+		mbox_t *mailbox, uint8_t block)
 {
 	size_t blocksize = 16;
-	uint8_t resp[3 + 1 + 16];
+	ipmi_mbox_response_t ipmi_mbox_resp = { .cc = 0xFF };
 	uint16_t resp_len;
 	char *debug_buf;
 	int rc;
@@ -449,59 +472,60 @@  static int get_ipmi_boot_mailbox_block(struct platform_powerpc *platform,
 		block, /* set selector */
 		0x00,  /* no block selector */
 	};
+	size_t ipmi_header_len = sizeof(ipmi_mbox_response_t) - sizeof(mbox_t);
 
-	resp_len = sizeof(resp);
+	resp_len = sizeof(ipmi_mbox_response_t);
 	rc = ipmi_transaction(platform->ipmi, IPMI_NETFN_CHASSIS,
 			IPMI_CMD_CHASSIS_GET_SYSTEM_BOOT_OPTIONS,
 			req, sizeof(req),
-			resp, &resp_len,
+			(uint8_t*)&ipmi_mbox_resp, &resp_len,
 			ipmi_timeout);
 	if (rc) {
 		pb_log("platform: error reading IPMI boot options\n");
 		return -1;
 	}
 
-	if (resp_len > sizeof(resp)) {
+	if (resp_len > sizeof(ipmi_mbox_response_t)) {
 		pb_debug("platform: invalid mailbox response size!\n");
 		return -1;
 	}
 
-	if (resp_len < 4) {
+	if (resp_len < ipmi_header_len) {
 		pb_log("platform: unexpected length (%d) in "
 				"boot options mailbox response\n",
 				resp_len);
 		return -1;
 	}
 
-	blocksize = resp_len - 4;
+	blocksize = resp_len - ipmi_header_len;
 	pb_debug_fn("Mailbox block %hu returns only %zu bytes in block\n",
 			block, blocksize);
 
-	debug_buf = format_buffer(platform, resp, resp_len);
+	debug_buf = format_buffer(platform, (uint8_t*)&ipmi_mbox_resp, resp_len);
 	pb_debug_fn("IPMI bootdev mailbox block %hu:\n%s\n", block, debug_buf);
 	talloc_free(debug_buf);
 
-	if (resp[0] != 0) {
+	if (ipmi_mbox_resp.cc != 0) {
 		pb_log("platform: non-zero completion code %d from IPMI req\n",
-				resp[0]);
+				ipmi_mbox_resp.cc);
 		return -1;
 	}
 
 	/* check for correct parameter version */
-	if ((resp[1] & 0xf) != 0x1) {
+	if ((ipmi_mbox_resp.param_version & 0xf) != 0x1) {
 		pb_log("platform: unexpected version (0x%x) in "
-				"boot mailbox response\n", resp[0]);
+				"boot mailbox response\n", ipmi_mbox_resp.param_version);
 		return -1;
 	}
 
 	/* check for valid paramters */
-	if (resp[2] & 0x80) {
+	if (ipmi_mbox_resp.param_valid & 0x80) {
 		pb_debug("platform: boot mailbox parameters are invalid/locked\n");
 		return -1;
 	}
 
 	/* check for block number */
-	if (resp[3] != block) {
+	if (ipmi_mbox_resp.block_selector != block) {
 		pb_debug("platform: returned boot mailbox block doesn't match "
 				  "requested\n");
 		return -1;
@@ -512,8 +536,7 @@  static int get_ipmi_boot_mailbox_block(struct platform_powerpc *platform,
 		return 0;
 	}
 
-
-	memcpy(buf, &resp[4], blocksize);
+	memcpy(mailbox, &ipmi_mbox_resp.mbox, blocksize);
 
 	return blocksize;
 }
@@ -522,12 +545,10 @@  static int get_ipmi_boot_mailbox(struct platform_powerpc *platform,
 		char **buf)
 {
 	char *mailbox_buffer, *prefix;
-	const size_t blocksize = 16;
-	char block_buffer[blocksize];
+	mbox_t mailbox;
 	size_t mailbox_size;
 	int content_size;
 	uint8_t i;
-	int rc;
 
 	mailbox_buffer = NULL;
 	mailbox_size = 0;
@@ -538,15 +559,15 @@  static int get_ipmi_boot_mailbox(struct platform_powerpc *platform,
 	 * on higher numbers.
 	 */
 	for (i = 0; i < UCHAR_MAX; i++) {
-		rc = get_ipmi_boot_mailbox_block(platform, block_buffer, i);
-		if (rc < 3 && i == 0) {
+		int block_size = get_ipmi_boot_mailbox_block(platform, &mailbox, i);
+		if (block_size < CHASSIS_BOOT_MBOX_IANA_SZ && i == 0) {
 			/*
 			 * Immediate failure, no blocks read or missing IANA
 			 * number.
 			 */
 			return -1;
 		}
-		if (rc < 1) {
+		if (block_size < 1) {
 			/* Error or no bytes read */
 			break;
 		}
@@ -557,25 +578,25 @@  static int get_ipmi_boot_mailbox(struct platform_powerpc *platform,
 			 * Enterprise ID number. Check it matches the IBM
 			 * number, '2'.
 			 */
-			if (block_buffer[0] != 0x02 ||
-				block_buffer[1] != 0x00 ||
-				block_buffer[2] != 0x00) {
+			if (mailbox.b0.iana[0] != 0x02 ||
+				mailbox.b0.iana[1] != 0x00 ||
+				mailbox.b0.iana[2] != 0x00) {
 				pb_log_fn("IANA number unrecognised: 0x%x:0x%x:0x%x\n",
-						block_buffer[0],
-						block_buffer[1],
-						block_buffer[2]);
+						mailbox.b0.iana[0],
+						mailbox.b0.iana[1],
+						mailbox.b0.iana[2]);
 				return -1;
 			}
 		}
 
 		mailbox_buffer = talloc_realloc(platform, mailbox_buffer,
-				char, mailbox_size + rc);
+				char, mailbox_size + block_size);
 		if (!mailbox_buffer) {
 			pb_log_fn("Failed to allocate mailbox buffer\n");
 			return -1;
 		}
-		memcpy(mailbox_buffer + mailbox_size, block_buffer, rc);
-		mailbox_size += rc;
+		memcpy(mailbox_buffer + mailbox_size, &mailbox.data, block_size);
+		mailbox_size += block_size;
 	}
 
 	if (i < 5)