[V6,20/21] core/pldm: Add file io write request

Send/receive a PLDM WriteFile request message.

Due to maximum transfer size for PLDM protocol, we have to send several
write requests, if necessary.

Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com>
---
 core/pldm/pldm-file-io-requests.c | 123 ++++++++++++++++++++++++++++++
 core/pldm/pldm.h                  |   2 +
 2 files changed, 125 insertions(+)

Hello Christophe
I am having some questions in this patch
> +
> +	if ((offset) && (offset > size))
> +		return OPAL_PARAMETER;
Why is offset need to be lesser than size of buffer to write?
> +
> +	request_length = sizeof(struct pldm_msg_hdr) +
> +			 sizeof(struct pldm_write_file_req) +
> +			 size;
Why request payload is equal to size (size of buffer)?
The Maximum request payload needed when size > MAX_TRANSFER_SIZE_BYTES
is MAX_TRANSFER_SIZE_BYTES


> +
> +	for (i = 0; i < num_transfers; i++) {
> +		file_req.offset = offset + (i * MAX_TRANSFER_SIZE_BYTES);
> +
> +		/* Encode the file request */
> +		rc = encode_write_file_req(
> +				DEFAULT_INSTANCE_ID,
> +				file_req.file_handle,
> +				file_req.offset,
> +				file_req.length,
> +				(struct pldm_msg *)request_msg);
> +		if (rc != PLDM_SUCCESS) {
> +			prlog(PR_ERR, "Encode WriteFileReq Error, rc: %d\n", rc);
> +			free(request_msg);
> +			return OPAL_PARAMETER;
> +		}
> +
> +		/* Send and get the response message bytes */
> +		rc = pldm_requester_queue_and_wait(
> +					request_msg, request_length - 1,
> +					&response_msg, &response_len);
> +		if (rc) {
> +			prlog(PR_ERR, "Communication Error, req: WriteFileReq, rc: %d\n", rc);
> +			free(request_msg);
> +			return rc;
> +		}
> +
> +		/* Decode the message */
> +		payload_len = response_len - sizeof(struct pldm_msg_hdr);
> +		rc = decode_write_file_resp(
> +				response_msg,
> +				payload_len,
> +				&completion_code,
> +				&resp_length);
> +		if (rc != PLDM_SUCCESS || completion_code != PLDM_SUCCESS) {
> +			prlog(PR_ERR, "Decode WriteFileResp Error, rc: %d, cc: %d\n",
> +				      rc, completion_code);
> +			free(request_msg);
> +			free(response_msg);
> +			return OPAL_PARAMETER;
> +		}
> +
> +		if (resp_length == 0) {
> +			free(response_msg);
> +			break;
> +		}
> +
> +		total_write += resp_length;
> +		current_ptr += resp_length;
> +		free(response_msg);
> +
In this loop, if size > MAX_TRANSFER_SIZE_BYTES, after sending
MAX_TRANSFER_SIZE_BYTES once how is payload pointer increasing?
I think we should copy current_ptr to request_msg payload

> +		if (total_write == size)
> +			break;
> +		else if (resp_length != file_req.length) {
> +			/* end of file */
> +			break;
> +		} else if (MAX_TRANSFER_SIZE_BYTES > (size - total_write))
> +			file_req.length = size - total_write;
> +	}
> +
Reviewed-by: Abhishek Singh Tomar <abhishek@linux.ibm.com>

Le 30/09/2022 à 08:27, Abhishek SIngh Tomar a écrit :
> Hello Christophe
> I am having some questions in this patch
>> +
>> +	if ((offset) && (offset > size))
>> +		return OPAL_PARAMETER;
> Why is offset need to be lesser than size of buffer to write?
Good catch ! The test is not correct. The good solution is:
     if ((offset) && ((size + offset) > file_length))
         return OPAL_PARAMETER;

>> +
>> +	request_length = sizeof(struct pldm_msg_hdr) +
>> +			 sizeof(struct pldm_write_file_req) +
>> +			 size;
> Why request payload is equal to size (size of buffer)?
> The Maximum request payload needed when size > MAX_TRANSFER_SIZE_BYTES
> is MAX_TRANSFER_SIZE_BYTES
>
request_length is the size of the complete PLDM request you send to the BMC.
We need to add the size of the common header + the specific header + the
size of lid file requested.

>> +
>> +	for (i = 0; i < num_transfers; i++) {
>> +		file_req.offset = offset + (i * MAX_TRANSFER_SIZE_BYTES);
>> +
>> +		/* Encode the file request */
>> +		rc = encode_write_file_req(
>> +				DEFAULT_INSTANCE_ID,
>> +				file_req.file_handle,
>> +				file_req.offset,
>> +				file_req.length,
>> +				(struct pldm_msg *)request_msg);
>> +		if (rc != PLDM_SUCCESS) {
>> +			prlog(PR_ERR, "Encode WriteFileReq Error, rc: %d\n", rc);
>> +			free(request_msg);
>> +			return OPAL_PARAMETER;
>> +		}
>> +
>> +		/* Send and get the response message bytes */
>> +		rc = pldm_requester_queue_and_wait(
>> +					request_msg, request_length - 1,
>> +					&response_msg, &response_len);
>> +		if (rc) {
>> +			prlog(PR_ERR, "Communication Error, req: WriteFileReq, rc: %d\n", rc);
>> +			free(request_msg);
>> +			return rc;
>> +		}
>> +
>> +		/* Decode the message */
>> +		payload_len = response_len - sizeof(struct pldm_msg_hdr);
>> +		rc = decode_write_file_resp(
>> +				response_msg,
>> +				payload_len,
>> +				&completion_code,
>> +				&resp_length);
>> +		if (rc != PLDM_SUCCESS || completion_code != PLDM_SUCCESS) {
>> +			prlog(PR_ERR, "Decode WriteFileResp Error, rc: %d, cc: %d\n",
>> +				      rc, completion_code);
>> +			free(request_msg);
>> +			free(response_msg);
>> +			return OPAL_PARAMETER;
>> +		}
>> +
>> +		if (resp_length == 0) {
>> +			free(response_msg);
>> +			break;
>> +		}
>> +
>> +		total_write += resp_length;
>> +		current_ptr += resp_length;
>> +		free(response_msg);
>> +
> In this loop, if size > MAX_TRANSFER_SIZE_BYTES, after sending
> MAX_TRANSFER_SIZE_BYTES once how is payload pointer increasing?
> I think we should copy current_ptr to request_msg payload
You are right. Good point.

>> +		if (total_write == size)
>> +			break;
>> +		else if (resp_length != file_req.length) {
>> +			/* end of file */
>> +			break;
>> +		} else if (MAX_TRANSFER_SIZE_BYTES > (size - total_write))
>> +			file_req.length = size - total_write;
>> +	}
>> +
> Reviewed-by: Abhishek Singh Tomar <abhishek@linux.ibm.com>

On 13/09/2022 12:27, Christophe Lombard wrote:
> Send/receive a PLDM WriteFile request message.
> 
> Due to maximum transfer size for PLDM protocol, we have to send several
> write requests, if necessary.
> 
> Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com>
> ---
>   core/pldm/pldm-file-io-requests.c | 123 ++++++++++++++++++++++++++++++
>   core/pldm/pldm.h                  |   2 +
>   2 files changed, 125 insertions(+)
> 
> diff --git a/core/pldm/pldm-file-io-requests.c b/core/pldm/pldm-file-io-requests.c
> index 48b00ef2..0665a679 100644
> --- a/core/pldm/pldm-file-io-requests.c
> +++ b/core/pldm/pldm-file-io-requests.c
> @@ -162,6 +162,129 @@ int pldm_file_io_read_file(uint32_t file_handle, uint32_t file_length,
>   	return read_file_req(file_handle, file_length, buf, offset, size);
>   }
>   
> +/*
> + * Send/receive a PLDM WriteFile request message.
> + */
> +static int write_file_req(uint32_t file_handle, const void *buf,
> +			  uint32_t offset, uint64_t size)
> +{


Unlike the read API, we don't have the file_size of the lid. Expected? 
Can we "grow" the file? what happens if we go over?


> +	void *response_msg, *current_ptr, *payload_data;
> +	uint32_t total_write, resp_length, request_length;
> +	size_t response_len, payload_len;
> +	uint8_t completion_code;
> +	int num_transfers;
> +	char *request_msg;
> +	int rc, i;
> +
> +	struct pldm_write_file_req file_req = {
> +		.file_handle = file_handle,
> +		.offset = offset,
> +		.length = size
> +	};
> +
> +	if (!size)
> +		return OPAL_PARAMETER;
> +
> +	if ((offset) && (offset > size))
> +		return OPAL_PARAMETER;
> +
> +	request_length = sizeof(struct pldm_msg_hdr) +
> +			 sizeof(struct pldm_write_file_req) +
> +			 size;
> +	request_msg = malloc(request_length);


zalloc()? (it's actually in the next patch)
We should check for a failed allocation.


> +
> +	payload_data = ((struct pldm_msg *)request_msg)->payload
> +			+ sizeof(file_req.file_handle)
> +			+ sizeof(file_req.offset)
> +			+ sizeof(file_req.length);


We could use offsetof(struct pldm_write_file_req, file_data)


> +	memcpy(payload_data, buf, size);
> +	current_ptr = payload_data;
> +	num_transfers = 1;
> +	total_write = 0;
> +
> +	if (size > MAX_TRANSFER_SIZE_BYTES) {
> +		num_transfers = (size + MAX_TRANSFER_SIZE_BYTES - 1) /
> +				MAX_TRANSFER_SIZE_BYTES;
> +		file_req.length = MAX_TRANSFER_SIZE_BYTES;
> +	}
> +
> +	prlog(PR_TRACE, "%s - file_handle: %d, offset: 0x%x, size: 0x%x, num_transfers: %d\n",
> +			__func__, file_handle, file_req.offset,
> +			file_req.length, num_transfers);
> +
> +	for (i = 0; i < num_transfers; i++) {
> +		file_req.offset = offset + (i * MAX_TRANSFER_SIZE_BYTES);
> +


I don't understand how it works when num_transfers > 1. Seems like we 
never update the content being sent over.


> +		/* Encode the file request */
> +		rc = encode_write_file_req(
> +				DEFAULT_INSTANCE_ID,
> +				file_req.file_handle,
> +				file_req.offset,
> +				file_req.length,
> +				(struct pldm_msg *)request_msg);
> +		if (rc != PLDM_SUCCESS) {
> +			prlog(PR_ERR, "Encode WriteFileReq Error, rc: %d\n", rc);
> +			free(request_msg);
> +			return OPAL_PARAMETER;
> +		}
> +
> +		/* Send and get the response message bytes */
> +		rc = pldm_requester_queue_and_wait(
> +					request_msg, request_length - 1,
> +					&response_msg, &response_len);
> +		if (rc) {
> +			prlog(PR_ERR, "Communication Error, req: WriteFileReq, rc: %d\n", rc);
> +			free(request_msg);
> +			return rc;
> +		}
> +
> +		/* Decode the message */
> +		payload_len = response_len - sizeof(struct pldm_msg_hdr);
> +		rc = decode_write_file_resp(
> +				response_msg,
> +				payload_len,
> +				&completion_code,
> +				&resp_length);
> +		if (rc != PLDM_SUCCESS || completion_code != PLDM_SUCCESS) {
> +			prlog(PR_ERR, "Decode WriteFileResp Error, rc: %d, cc: %d\n",
> +				      rc, completion_code);
> +			free(request_msg);
> +			free(response_msg);
> +			return OPAL_PARAMETER;
> +		}
> +
> +		if (resp_length == 0) {
> +			free(response_msg);
> +			break;
> +		}
> +
> +		total_write += resp_length;
> +		current_ptr += resp_length;
> +		free(response_msg);
> +
> +		if (total_write == size)
> +			break;
> +		else if (resp_length != file_req.length) {
> +			/* end of file */
> +			break;
> +		} else if (MAX_TRANSFER_SIZE_BYTES > (size - total_write))
> +			file_req.length = size - total_write;
> +	}
> +
> +	free(request_msg);
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +int pldm_file_io_write_file(uint32_t file_handle, const void *buf,
> +			    uint32_t offset, uint64_t size)
> +{
> +	if (!file_io_ready)
> +		return OPAL_PARAMETER;
> +
> +	return write_file_req(file_handle, buf, offset, size);
> +}
> +
>   /*
>    * Send/receive a PLDM GetFileTable request message.
>    * The file table contains the list of files available and
> diff --git a/core/pldm/pldm.h b/core/pldm/pldm.h
> index 2b2a979d..688c7996 100644
> --- a/core/pldm/pldm.h
> +++ b/core/pldm/pldm.h
> @@ -49,6 +49,8 @@ int pldm_responder_init(void);
>   /* Requester support */
>   int pldm_file_io_read_file(uint32_t file_handle, uint32_t file_length,
>   			   void *buf, uint32_t offset, uint64_t size);
> +int pldm_file_io_write_file(uint32_t file_handle, const void *buf,
> +			    uint32_t offset, uint64_t size);
>   int pldm_file_io_init(void);
>   
>   int pldm_fru_get_bmc_version(void *bv);

Le 12/04/2023 à 18:37, Frederic Barrat a écrit :
>
>
> On 13/09/2022 12:27, Christophe Lombard wrote:
>> Send/receive a PLDM WriteFile request message.
>>
>> Due to maximum transfer size for PLDM protocol, we have to send several
>> write requests, if necessary.
>>
>> Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com>
>> ---
>>   core/pldm/pldm-file-io-requests.c | 123 ++++++++++++++++++++++++++++++
>>   core/pldm/pldm.h                  |   2 +
>>   2 files changed, 125 insertions(+)
>>
>> diff --git a/core/pldm/pldm-file-io-requests.c 
>> b/core/pldm/pldm-file-io-requests.c
>> index 48b00ef2..0665a679 100644
>> --- a/core/pldm/pldm-file-io-requests.c
>> +++ b/core/pldm/pldm-file-io-requests.c
>> @@ -162,6 +162,129 @@ int pldm_file_io_read_file(uint32_t 
>> file_handle, uint32_t file_length,
>>       return read_file_req(file_handle, file_length, buf, offset, size);
>>   }
>>   +/*
>> + * Send/receive a PLDM WriteFile request message.
>> + */
>> +static int write_file_req(uint32_t file_handle, const void *buf,
>> +              uint32_t offset, uint64_t size)
>> +{
>
>
> Unlike the read API, we don't have the file_size of the lid. Expected? 
> Can we "grow" the file? what happens if we go over?
>

We encode a write file request with a specific offset and a length 
(data). The BMC will handle writing the file.
It's up to the BMC to manage the size.

>
>> +    void *response_msg, *current_ptr, *payload_data;
>> +    uint32_t total_write, resp_length, request_length;
>> +    size_t response_len, payload_len;
>> +    uint8_t completion_code;
>> +    int num_transfers;
>> +    char *request_msg;
>> +    int rc, i;
>> +
>> +    struct pldm_write_file_req file_req = {
>> +        .file_handle = file_handle,
>> +        .offset = offset,
>> +        .length = size
>> +    };
>> +
>> +    if (!size)
>> +        return OPAL_PARAMETER;
>> +
>> +    if ((offset) && (offset > size))
>> +        return OPAL_PARAMETER;
>> +
>> +    request_length = sizeof(struct pldm_msg_hdr) +
>> +             sizeof(struct pldm_write_file_req) +
>> +             size;
>> +    request_msg = malloc(request_length);
>
>
> zalloc()? (it's actually in the next patch)
> We should check for a failed allocation.
>

Yep.

>
>> +
>> +    payload_data = ((struct pldm_msg *)request_msg)->payload
>> +            + sizeof(file_req.file_handle)
>> +            + sizeof(file_req.offset)
>> +            + sizeof(file_req.length);
>
>
> We could use offsetof(struct pldm_write_file_req, file_data)
>
>

correct. Thanks

>> +    memcpy(payload_data, buf, size);
>> +    current_ptr = payload_data;
>> +    num_transfers = 1;
>> +    total_write = 0;
>> +
>> +    if (size > MAX_TRANSFER_SIZE_BYTES) {
>> +        num_transfers = (size + MAX_TRANSFER_SIZE_BYTES - 1) /
>> +                MAX_TRANSFER_SIZE_BYTES;
>> +        file_req.length = MAX_TRANSFER_SIZE_BYTES;
>> +    }
>> +
>> +    prlog(PR_TRACE, "%s - file_handle: %d, offset: 0x%x, size: 0x%x, 
>> num_transfers: %d\n",
>> +            __func__, file_handle, file_req.offset,
>> +            file_req.length, num_transfers);
>> +
>> +    for (i = 0; i < num_transfers; i++) {
>> +        file_req.offset = offset + (i * MAX_TRANSFER_SIZE_BYTES);
>> +
>
>
> I don't understand how it works when num_transfers > 1. Seems like we 
> never update the content being sent over.
>
>

During the loop, we update the offset. The request will take into 
account of this value.

>> +        /* Encode the file request */
>> +        rc = encode_write_file_req(
>> +                DEFAULT_INSTANCE_ID,
>> +                file_req.file_handle,
>> +                file_req.offset,
>> +                file_req.length,
>> +                (struct pldm_msg *)request_msg);
>> +        if (rc != PLDM_SUCCESS) {
>> +            prlog(PR_ERR, "Encode WriteFileReq Error, rc: %d\n", rc);
>> +            free(request_msg);
>> +            return OPAL_PARAMETER;
>> +        }
>> +
>> +        /* Send and get the response message bytes */
>> +        rc = pldm_requester_queue_and_wait(
>> +                    request_msg, request_length - 1,
>> +                    &response_msg, &response_len);
>> +        if (rc) {
>> +            prlog(PR_ERR, "Communication Error, req: WriteFileReq, 
>> rc: %d\n", rc);
>> +            free(request_msg);
>> +            return rc;
>> +        }
>> +
>> +        /* Decode the message */
>> +        payload_len = response_len - sizeof(struct pldm_msg_hdr);
>> +        rc = decode_write_file_resp(
>> +                response_msg,
>> +                payload_len,
>> +                &completion_code,
>> +                &resp_length);
>> +        if (rc != PLDM_SUCCESS || completion_code != PLDM_SUCCESS) {
>> +            prlog(PR_ERR, "Decode WriteFileResp Error, rc: %d, cc: 
>> %d\n",
>> +                      rc, completion_code);
>> +            free(request_msg);
>> +            free(response_msg);
>> +            return OPAL_PARAMETER;
>> +        }
>> +
>> +        if (resp_length == 0) {
>> +            free(response_msg);
>> +            break;
>> +        }
>> +
>> +        total_write += resp_length;
>> +        current_ptr += resp_length;
>> +        free(response_msg);
>> +
>> +        if (total_write == size)
>> +            break;
>> +        else if (resp_length != file_req.length) {
>> +            /* end of file */
>> +            break;
>> +        } else if (MAX_TRANSFER_SIZE_BYTES > (size - total_write))
>> +            file_req.length = size - total_write;
>> +    }
>> +
>> +    free(request_msg);
>> +
>> +    return OPAL_SUCCESS;
>> +}
>> +
>> +int pldm_file_io_write_file(uint32_t file_handle, const void *buf,
>> +                uint32_t offset, uint64_t size)
>> +{
>> +    if (!file_io_ready)
>> +        return OPAL_PARAMETER;
>> +
>> +    return write_file_req(file_handle, buf, offset, size);
>> +}
>> +
>>   /*
>>    * Send/receive a PLDM GetFileTable request message.
>>    * The file table contains the list of files available and
>> diff --git a/core/pldm/pldm.h b/core/pldm/pldm.h
>> index 2b2a979d..688c7996 100644
>> --- a/core/pldm/pldm.h
>> +++ b/core/pldm/pldm.h
>> @@ -49,6 +49,8 @@ int pldm_responder_init(void);
>>   /* Requester support */
>>   int pldm_file_io_read_file(uint32_t file_handle, uint32_t file_length,
>>                  void *buf, uint32_t offset, uint64_t size);
>> +int pldm_file_io_write_file(uint32_t file_handle, const void *buf,
>> +                uint32_t offset, uint64_t size);
>>   int pldm_file_io_init(void);
>>     int pldm_fru_get_bmc_version(void *bv);