[Skiboot] [PATCH v3 1/9] core/pldm/test : Implement PLDM self test common api

[Abhishek SIngh Tomar]

On Mon, May 23, 2022 at 01:47:30PM +0200, Christophe Lombard wrote:
> 
> Le 18/05/2022 à 09:42, Abhishek Singh Tomar a écrit :
> > The patch bypasses hardware-dependent implementation for
> > PLDM test. This patch provides API that will bypass the
> > need to exchange messages with BMC over the LPC bus whereas
> > messages are handled by the same program for self test.
> > 
> > It also contains common functions and declarations that
> > are used to implement the PLDM self test.
> > 
> > Signed-off-by: Abhishek Singh Tomar<abhishek at linux.ibm.com>
> > ---
> >   core/pldm/test/common/test-pldm-common.c | 190 +++++++++++++++++++++++
> >   1 file changed, 190 insertions(+)
> >   create mode 100644 core/pldm/test/common/test-pldm-common.c
> 
> why did you create a subfolder "common" ?
The idea was to create sub folder that give clear view that common
is part of code which is to be used by all tests but is not test in
itself. if you want me to put file 'test-pldm-common.c' in test directory
it will be ok for me.

> 
> > diff --git a/core/pldm/test/common/test-pldm-common.c b/core/pldm/test/common/test-pldm-common.c
> > new file mode 100644
> > index 00000000..4acbfd35
> > --- /dev/null
> > +++ b/core/pldm/test/common/test-pldm-common.c
> > @@ -0,0 +1,190 @@
> > +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
> > +// Copyright 2022 IBM Corp.
> > +
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <stdarg.h>
> > +#include <stdbool.h>
> > +#include <types.h>
> > +#include <stdint.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <time.h>
> > +#include <timer.h>
> > +#include <ccan/list/list.h>
> > +#include <ccan/short_types/short_types.h>
> > +
> > +#define __LITTLE_ENDIAN_BITFIELD
> > +#define __TEST__
> > +#define __SKIBOOT__
> > +#define zalloc(bytes) calloc((bytes), 1)
> > +static inline unsigned long mftb(void);
> > +#include <timebase.h>
> > +#include <op-panel.h>
> > +#include <include/platform.h>
> > +#include "../../pldm.h"
> > +#include <include/pldm.h>
> > +#include <ast.h>
> > +#ifdef ARRAY_SIZE
> > +#undef ARRAY_SIZE
> > +#endif
> > +
> > +
> > +#include <pldm/libpldm/bios_table.h>
> > +#include <pldm/libpldm/bios_table.c>
> > +#undef pr_fmt
> > +#include "../../pldm-bios-requests.c"
> > +#include <pldm/libpldm/base.c>
> > +#include <pldm/libpldm/pdr.c>
> > +#include <pldm/libpldm/bios.c>
> > +#include <pldm/libpldm/platform.c>
> > +#include <pldm/libpldm/utils.c>
> > +#include <pldm/ibm/libpldm/file_io.c>
> > +#include <pldm/libpldm/fru.c>
> > +#include "../../pldm-file-io-requests.c"
> > +#include "../../pldm-requester.c"
> > +#include "../../pldm-common.c"
> > +#include "../../pldm-responder.c"
> > +#include "../../pldm-base-requests.c"
> > +#include "../../pldm-watchdog.c"
> > +#include "../../pldm-fru-requests.c"
> > +#include "../../pldm-platform-requests.c"
> > +#include "../../../device.c"
> > +
> > +
> > +
> > +char __rodata_start[1], __rodata_end[1];
> > +unsigned long tb_hz = 512000000;
> > +struct dt_node *dt_root;
> > +struct debug_descriptor debug_descriptor;
> > +struct platform platform;
> > +
> > +int pldm_test_reply_request(void *request_msg, size_t request_len,
> > +		void **response_msg, size_t *response_len);
> > +int pldm_test_verify_response(void *response_msg, size_t response_len);
> > +
> 
> I am not so fan about the global functions. So, rather than declaring
> these functions in each test file, we could instead have a common list,
> each test would record its type and the functions to call.
I will try to avoid use global function in same file.
if decleration is needed can I create .h file for respective file?

Why I added defination in same file? -> 
As i noticed that .h extension is not common in test folders
hence i add decleration in starting of file. 

Having a common list is not possible as one testcase defination not
visible with other testcase

> 
> > +void time_wait_ms(unsigned long ms)
> > +{
> > +	usleep(ms * 1000);
> > +}
> > +void init_timer(struct timer *t, timer_func_t expiry, void *data)
> > +{
> > +	t->link.next = t->link.prev = NULL;
> > +	t->target = 0;
> > +	t->expiry = expiry;
> > +	t->user_data = data;
> > +	t->running = NULL;
> > +}
> > +uint64_t schedule_timer(struct timer *t, uint64_t how_long)
> > +{
> > +	if (t != NULL)
> > +		return how_long;
> > +	return 0;
> > +}
> > +void cancel_timer(struct timer *t)
> > +{
> > +	t->link.next = t->link.prev = NULL;
> > +}
> > +
> > +static inline unsigned long mftb(void)
> > +{
> > +	unsigned long clk;
> > +
> > +	clk = clock();
> > +	return clk;
> > +}
> > +
> > +int ast_mctp_init(void (*fn)(uint8_t src_eid, bool tag_owner, uint8_t msg_tag, void *data,
> > +			void *msg, size_t len))
> > +{
> > +	if (fn != NULL)
> > +		return OPAL_SUCCESS;
> > +	return OPAL_PARAMETER;
> > +}
> > +
> > +int ast_mctp_message_tx(uint8_t eid, uint8_t *msg, int len)
> > +{
> > +	int ret;
> > +	uint8_t *pldm_received_msg = msg+1;
> > +	void *response_msg;
> > +	char *vmsg;
> > +	size_t response_len;
> > +
> > +	/* TEST eid is BMC_ID */
> > +	if (eid != BMC_EID)
> > +		return OPAL_PARAMETER;
> > +
> > +	/* TEST Message TYPE: PLDM = 0x01 (000_0001b) as per MCTP - DSP0240 */
> > +	if (msg[0] != 0x01) {
> > +		printf("TEST : %s : request MCTP message type not set for PLDM\n", __func__);
> > +		return OPAL_PARAMETER;
> > +	}
> > +
> > +	if (((struct pldm_msg *)pldm_received_msg)->hdr.request == PLDM_RESPONSE) {
> > +		ret = pldm_test_verify_response(pldm_received_msg, len-1);
> > +		if (ret != OPAL_SUCCESS)
> > +			return ret;
> > +	}
> > +
> 
> You could simplify the code:
> 
>     request = ((struct pldm_msg *)pldm_received_msg)->hdr.request;
> 
>     switch (request) {
>     case PLDM_RESPONSE:
>         return pldm_test_verify_response(pldm_received_msg, len-1);
>         break;
> 
>     case PLDM_REQUEST:
>         ....
Ok

> > +	/* Reply to requests */
> > +	else if (((struct pldm_msg *)pldm_received_msg)->hdr.request == PLDM_REQUEST) {
> > +		ret = pldm_test_reply_request(pldm_received_msg, len-1,
> > +				&response_msg, &response_len);
> 
> The name of the function is not very explicit.
> pldm_test_reply_request() seems to build a response to the Skiboot request.
> The response is sent afterwards.
so name is chosen such that it generate reply based on iput request

will changing pldm_test_reply_request() with pldm_test_process_request()
seems good?
> 
> > +		if (ret != OPAL_SUCCESS)
> > +			return ret;
> > +
> 
> blanck is not necessary.
> 
> > +
> > +		/*
> > +		 * If response length > 0 then response back
> > +		 * else if response length == 0 then no need to response
> > +		 */
> > +		if (response_len > 0) {
> 
> Why do you need to test response_len ?
> If response_len is equal to 0 or <0, so pldm_test_reply_request will return
> an error.
reponse_len is used to make decision that is response message need to be
replied or not as commented.

The example when response need not to replied :
BMC doesn't answer for specific effecter states 
(PLDM_SW_TERM_GRACEFUL_RESTARTand PLDM_STATE_SET_SYS_POWER_STATE_OFF)
* hence *response len = 0

As response_len is unsigned hence response_len < 0 will never be case

> > +			vmsg = malloc(response_len+1);
> > +			/*  TYPE: PLDM = 0x01 (000_0001b) as per MCTP - DSP0240 */
> > +			vmsg[0] = 1;
> > +			memcpy(vmsg + 1, response_msg, response_len);
> > +			free(response_msg);
> > +			pldm_rx_message(BMC_EID, 0, 0, NULL, vmsg, response_len+1);
> > +			free(vmsg);
> > +		}
> > +	}
> > +
> > +	return OPAL_SUCCESS;
> > +}
> > +
> > +void ast_mctp_exit(void)
> > +{
> > +}
> > +
> > +void lock_caller(struct lock *l, const char *caller)
> > +{
> > +	(void)caller;
> > +	assert(!l->lock_val);
> > +	l->lock_val++;
> > +}
> > +
> > +int _opal_queue_msg(enum opal_msg_type msg_type, void *data,
> > +		void (*consumed)(void *data, int status),
> > +		size_t params_size, const void *params)
> > +{
> > +	(void)msg_type;
> > +	if (data != NULL || consumed != NULL)
> > +		return OPAL_PARAMETER;
> > +	if (params != NULL && params_size > 0)
> > +		return OPAL_PARAMETER;
> > +	return OPAL_SUCCESS;
> > +
> > +}
> > +
> > +
> > +void unlock(struct lock *l)
> > +{
> > +	assert(l->lock_val);
> > +	l->lock_val = 0;
> > +}
> > +
> > +void prd_occ_reset(uint32_t proc)
> > +{
> > +	(void)proc;
> > +}
> > +