Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: fix fragmented memory retrieve #5966

Closed
wants to merge 1 commit into from

Conversation

Jianqiang-Mei
Copy link

add spmc_frag_retrieve for fragmented memory retrieve

Copy link
Contributor

@jenswi-linaro jenswi-linaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the checkpatch complaints.

.a1 = (uint32_t)(cookie), /* handle high addr */
.a2 = (uint32_t)(cookie >> 32), /* handle low addr */
.a3 = next_fragment_offset, /* fragment offset */
.a4 = 0x8001, /* sender_vm_id */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my_endpoint_id

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

{
struct thread_smc_args args = {
.a0 = FFA_MEM_FRAG_RX,
.a1 = (uint32_t)(cookie), /* handle high addr */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer assigning a1 and a2 with reg_pair_from_64(cookie, &args.a1, &args.a2)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

EMSG("Failed to retrieve cookie from rx buffer %#"PRIx64,
cookie);
return NULL;
}

/* Allocate the enough size buffer to copy the fragment descriptor*/
retrieve_desc = aligned_alloc(SMALL_PAGE_SIZE, SMALL_PAGE_SIZE * 16);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not use a temporary buffer. We should instead consume each fragment as it is received like in handle_mem_frag_tx(). For now, I believe we can assume that each fragment ends with a complete struct ffa_address_range like we do in add_mem_share_helper().

Copy link
Author

@Jianqiang-Mei Jianqiang-Mei May 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not quite clear what you mean for your last sentence. Could you help me revise it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In add_mem_share_helper() we just assume that the buffer starts at a struct ffa_address_range. I guess it wouldn't hurt to check that flen is a multiple of sizeof(struct ffa_address_range), but the point is that we assume that the SPMC isn't trying to make it harder for us than necessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coding style comments

@@ -1383,7 +1383,8 @@ static uint16_t spmc_get_id(void)
return args.a2;
}

static struct ffa_mem_transaction *spmc_retrieve_req(uint64_t cookie)
static struct ffa_mem_transaction *spmc_retrieve_req
(uint64_t cookie, uint32_t *total_length, uint32_t *fragment_length)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static struct ffa_mem_transaction *spmc_retrieve_req(uint64_t cookie,
						     uint32_t *total_length,
						     uint32_t *fragment_length)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -1423,7 +1424,8 @@ static struct ffa_mem_transaction *spmc_retrieve_req(uint64_t cookie)
cookie, args.a0);
return NULL;
}

*total_length = args.a1;
*fragment_length = args.a2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preserve empty line above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

.a0 = FFA_MEM_FRAG_RX,
.a3 = next_fragment_offset, /* fragment offset */
.a4 = my_endpoint_id, /* sender_vm_id */
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add an empty line below (between local variable definitions and instructions)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

thread_smccc(&args);
if (args.a0 != FFA_MEM_FRAG_TX) {
if (args.a0 == FFA_ERROR)
EMSG("Failed to fetch cookie %#"PRIx64" error code %d",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove cast below and use PRId64

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

cookie, args.a0);
return NULL;
}
*fragment_length = args.a3;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add an empty line above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

DMSG("Retrieve again, frag offset: %x; total: %x.\n",
next_fragment_offset, length);
rx_buffer = spmc_frag_retrieve(handle, next_fragment_offset,
&fragment_length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation issue

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (rx_buffer) {
memcpy((void *)((uint64_t)retrieve_desc +
(uint64_t)next_fragment_offset),
rx_buffer, SMALL_PAGE_SIZE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation:

			memcpy((void *)((uint64_t)retrieve_desc +
					(uint64_t)next_fragment_offset),
			       rx_buffer, SMALL_PAGE_SIZE);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -1465,15 +1467,43 @@ static int set_pages(struct ffa_address_range *regions,
return 0;
}

static struct ffa_mem_transaction *spmc_frag_retrieve
(uint64_t cookie, uint32_t next_fragment_offset, uint32_t *fragment_length)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe prefer

static struct ffa_mem_transaction *spmc_frag_retrieve(
					uint64_t cookie,
					uint32_t next_fragment_offset,
					uint32_t *fragment_length)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@github-actions
Copy link

github-actions bot commented Jun 5, 2023

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Jun 5, 2023
@github-actions github-actions bot closed this Jun 11, 2023
@jenswi-linaro jenswi-linaro reopened this Jun 28, 2023
@Jianqiang-Mei Jianqiang-Mei force-pushed the dev_frag_retrieve branch 3 times, most recently from c6a7f33 to 2d224a0 Compare July 4, 2023 07:23
@@ -1756,6 +1756,8 @@ static uint32_t get_ffa_version(uint32_t my_version)
}

static void *spmc_retrieve_req(uint64_t cookie,
uint32_t *total_length,
uint32_t *fragment_length,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indentation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

unsigned int num_regions, unsigned int num_pages,
struct mobj_ffa *mf)
unsigned int num_regions, unsigned int *idx,
struct mobj_ffa *mf)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indentation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return 0;
}

static struct ffa_mem_transaction *spmc_frag_retrieve(uint64_t cookie,
uint32_t next_fragment_offset,
uint32_t *fragment_length)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indentation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to fix?
BTW, where can I see your coding rules?
Thanks.

READ_ONCE(descr->address_range_count), num_pages, mf)) {
num_regions =
MIN(READ_ONCE(descr->address_range_count),
(my_rxtx.size - offs) / sizeof(struct ffa_address_range) - 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation:

		MIN(READ_ONCE(descr->address_range_count),
		    (my_rxtx.size - offs) / sizeof(struct ffa_address_range) - 1);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be more specific. Thanks

while (offs < length) {
buf = spmc_frag_retrieve(handle, offs, &frag_length);
if (buf) {
DMSG("Retrieve again, frag_offs:%#"PRIx32"\n", offs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove trailing "\n"
offs is of type unsinged int hence s/%#"PRIx32/%u"/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Retrieve next fragment at offset %#PRIx32"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant:

			DMSG("Retrieve next fragment at offset %#x", offs);

@@ -1882,7 +1915,7 @@ struct mobj_ffa *thread_spmc_populate_mobj_from_rx(uint64_t cookie)
* OP-TEE is only supporting a single mem_region while the
* specification allows for more than one.
*/
buf = spmc_retrieve_req(cookie, &retrieve_desc);
buf = spmc_retrieve_req(cookie, &length, &frag_length, &retrieve_desc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function expect uint32_t pointers for those 2 arguments.
Either change the variable types or the function definition.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -1844,26 +1849,50 @@ void thread_spmc_relinquish(uint64_t cookie)
}

static int set_pages(struct ffa_address_range *regions,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the name add_pages_at() now that this function is slightly different.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

.a4 = my_endpoint_id, /* sender_vm_id */
};

reg_pair_from_64(cookie, &args.a2, &args.a1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, this doesn't work. I've created #6154 to provide suitable helpers to initialize .a2 and .a1 above instead. Until that PR is merged please revert to what you did originally.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

while (offs < length) {
buf = spmc_frag_retrieve(handle, offs, &frag_length);
if (buf) {
DMSG("Retrieve again, frag_offs:%#"PRIx32"\n", offs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Retrieve next fragment at offset %#PRIx32"

@@ -1882,7 +1915,7 @@ struct mobj_ffa *thread_spmc_populate_mobj_from_rx(uint64_t cookie)
* OP-TEE is only supporting a single mem_region while the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment isn't accurate any longer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted

@Jianqiang-Mei
Copy link
Author

Jianqiang-Mei commented Jul 4, 2023

All the indentation problems seem to be related to the github setup. My code has 4 spaces per tab on VSCode, but after push, a tab on github has 8 spaces. Has anyone experienced this problem?

@jforissier
Copy link
Contributor

All the indentation problems seem to be related to the github setup. My code has 4 spaces per tab on VSCode, but after push, a tab on github has 8 spaces. Has anyone experienced this problem?

We follow the same coding style as the Linux kernel, so 8 spaces per tab.

@Jianqiang-Mei
Copy link
Author

Jianqiang-Mei commented Jul 4, 2023

All the indentation problems seem to be related to the github setup. My code has 4 spaces per tab on VSCode, but after push, a tab on github has 8 spaces. Has anyone experienced this problem?

We follow the same coding style as the Linux kernel, so 8 spaces per tab.

OK, now I understand the problem and I will solve it soon.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit message: could you state this change relates to FF-A SPMC. somrhting like:

core: arm: spmc: fix fragmented memory retrieve

Adds implementation for memory retrieving for the remaining fragmented
transactions in SPMC.

static int set_pages(struct ffa_address_range *regions,
unsigned int num_regions, unsigned int num_pages,
static int add_pages_at(struct ffa_address_range *regions,
unsigned int num_regions, unsigned int *idx,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indenation issue here also (sorry, i guess i missed it)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return 0;
}

static struct ffa_mem_transaction *spmc_frag_retrieve(uint64_t cookie,
uint32_t next_fragment_offset,
uint32_t *fragment_length)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation.
Suggestion: shorten next_fragment_offset to next_frq_offset:

static struct ffa_mem_transaction *spmc_frag_retrieve(uint64_t cookie,
						      uint32_t next_frag_offset,
						      uint32_t *fragment_length)
{
   ...
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

buf = spmc_frag_retrieve(handle, offs, &frag_length);
if (buf) {
DMSG("Retrieve next fragment at offset %#"PRIx32,
offs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

			DMSG("Retrieve next fragment at offset %#x", offs);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

while (offs < length) {
buf = spmc_frag_retrieve(handle, offs, &frag_length);
if (buf) {
DMSG("Retrieve again, frag_offs:%#"PRIx32"\n", offs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant:

			DMSG("Retrieve next fragment at offset %#x", offs);

@Jianqiang-Mei
Copy link
Author

In commit message: could you state this change relates to FF-A SPMC. somrhting like:

core: arm: spmc: fix fragmented memory retrieve

Adds implementation for memory retrieving for the remaining fragmented
transactions in SPMC.

Done

@Jianqiang-Mei Jianqiang-Mei force-pushed the dev_frag_retrieve branch 4 times, most recently from f83ca40 to 5f1efd0 Compare July 4, 2023 12:43
@@ -1873,16 +1902,17 @@ struct mobj_ffa *thread_spmc_populate_mobj_from_rx(uint64_t cookie)
struct mobj_ffa *mf = NULL;
unsigned int num_pages = 0;
unsigned int offs = 0;
unsigned int num_regions = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

range_count would match the FF-A terminology better.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -1898,8 +1928,31 @@ struct mobj_ffa *thread_spmc_populate_mobj_from_rx(uint64_t cookie)
if (!mf)
goto out;

if (set_pages(descr->address_range_array,
READ_ONCE(descr->address_range_count), num_pages, mf)) {
num_regions = MIN(READ_ONCE(descr->address_range_count),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check that frag_len is less than length and the size of the rx buffer.
I'm not sure we even need to look at descr->address_range_count.

offs += offsetof(struct ffa_mem_region, address_range_array);
range_count = (frag_len - offs) / sizeof(struct ffa_address_range);

Copy link
Author

@Jianqiang-Mei Jianqiang-Mei Jul 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check that frag_len is less than length and the size of the rx buffer.

Done

I'm not sure we even need to look at descr->address_range_count.

offs += offsetof(struct ffa_mem_region, address_range_array);
range_count = (frag_len - offs) / sizeof(struct ffa_address_range);

If I understand correctly, descr->address_range_count represents the number of constituent memory region descriptors. And there is a formulation:
length - offs = sizeof (ffa_address_range) * descr->address_range_count
if descr->address_range_count does not satisfy this relation, it will eventually fail to pass the check as follows:

	if (idx != num_pages) {
		mobj_ffa_spmc_delete(mf);
		goto out;
	}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we could check descr->address_range_count, but depending on what we're doing with the value it might not be that useful.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need to do anything else? In addition, will the fix be merged into the optee 3.22 version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add Jens' review tag (below). I think it makes sense to take this for 3.22.0.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

buf = spmc_frag_retrieve(handle, offs, &frag_length);
if (buf) {
DMSG("Retrieve next fragment at offset %#x", offs);
num_regions = frag_length /
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check that frag_length isn't larger than the size of the rx buffer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@jenswi-linaro jenswi-linaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Jianqiang-Mei added a commit to Jianqiang-Mei/optee_os that referenced this pull request Jul 5, 2023
Add implementation for memory retrieving for the remaining fragmented
transactions.

Link: OP-TEE#5966
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Jianqiang,Mei <meijianqiang.mjq@alibaba-inc.com>
@jenswi-linaro
Copy link
Contributor

Looks good.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>

Jianqiang-Mei added a commit to Jianqiang-Mei/optee_os that referenced this pull request Jul 6, 2023
Add implementation for memory retrieving for the remaining fragmented
transactions.

Link: OP-TEE#5966
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Jianqiang,Mei <meijianqiang.mjq@alibaba-inc.com>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
@etienne-lms
Copy link
Contributor

@jforissier, @jenswi-linaro, could you enable CI test for this P-R?

@jforissier
Copy link
Contributor

@jforissier, @jenswi-linaro, could you enable CI test for this P-R?

Done.

@jenswi-linaro
Copy link
Contributor

This should be good to merge now.

return 0;
}

static struct ffa_mem_transaction *spmc_frag_retrieve(uint64_t cookie,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

struct ffa_mem_transaction or struct ffa_address_range ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe that's correct.

Copy link
Author

@Jianqiang-Mei Jianqiang-Mei Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for missing check in detail. ffa_mem_transaction doesn't like a good fit, or modify it to the (void *) type to be consistent with the function spmc_retrieve_req return type, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Jianqiang-Mei added a commit to Jianqiang-Mei/optee_os that referenced this pull request Jul 11, 2023
Add implementation for memory retrieving for the remaining fragmented
transactions.

Link: OP-TEE#5966
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Jianqiang,Mei <meijianqiang.mjq@alibaba-inc.com>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
@jenswi-linaro
Copy link
Contributor

Looks good.

.a1 = (uint32_t)(cookie), /* handle high addr */
.a2 = (uint32_t)(cookie >> 32), /* handle low addr */
.a3 = next_frag_offset, /* offset */
.a4 = my_endpoint_id, /* sender_vm_id */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in FF-A v1.1
w4 – Bit[31:16]: Endpoint ID
– Bit[15:0]: Reserved (MBZ).

maybe it should be sender_id(linux or hyp) << 16 ?

Copy link
Author

@Jianqiang-Mei Jianqiang-Mei Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A complete description of the parameter:
• ID of the Owner or Borrower endpoint.
– Bit[31:16]: Endpoint ID.
– Bit[15:0]: Reserved (MBZ).
• Reserved (MBZ) at any virtual FF-A instance.
In this scenario, the optee should be the Borrower, so modify the code as follows:
.a4 = my_endpoint_id << 16, /* sender_vm_id */
right?

Copy link
Author

@Jianqiang-Mei Jianqiang-Mei Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or Reserved (MBZ) at any virtual FF-A instance,and
.a4 = 0, /* sender_vm_id */
According to the implementation of Hafnium, this should be right. I'm not really sure.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think 0 is fine, reference:
https://github.com/ARM-software/arm-trusted-firmware/blob/e318411f02f8a9526c97a23d0cdada2f128446e3/services/std_svc/spm/el3_spmc/spmc_shared_mem.c#L1682

i am confused too about the description "Owner or Borrower", i think they are different roles.
i think sender_id used in optee maybe is not optee partiton id, reference:

trans_descr->sender_id = thread_get_tsd()->rpc_target_info;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

uint32_t length = 0;
uint32_t frag_length = 0;
uint64_t handle = 0;
unsigned int idx = 0;
void *buf = NULL;
struct thread_smc_args ffa_rx_release_args = {
.a0 = FFA_RX_RELEASE
Copy link

@Master-Chi Master-Chi Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe FFA_RX_RELEASE should be submitted after each FFA_MEM_FRAG_RX or FFA_MEM_RETRIEVE_REQ_32, or rx buffer is full in spmc perspective

Copy link
Author

@Jianqiang-Mei Jianqiang-Mei Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rx buffer is full in spmc perspective, The specification does not seem to say anything about this.
What happens if I release rx_buffer after the complete retrieve?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jenswi-linaro Do you have any good suggestions?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the specification says nothing about it.
but i think the implementation of trusted firmware requires that.
ref:
https://github.com/ARM-software/arm-trusted-firmware/blob/e318411f02f8a9526c97a23d0cdada2f128446e3/services/std_svc/spm/el3_spmc/spmc_shared_mem.c#L1704

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, it does need to be released several times, and I will modify it as soon as possible.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Jianqiang-Mei added a commit to Jianqiang-Mei/optee_os that referenced this pull request Jul 12, 2023
Add implementation for memory retrieving for the remaining fragmented
transactions.

Link: OP-TEE#5966
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Jianqiang,Mei <meijianqiang.mjq@alibaba-inc.com>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
Jianqiang-Mei added a commit to Jianqiang-Mei/optee_os that referenced this pull request Jul 12, 2023
Add implementation for memory retrieving for the remaining fragmented
transactions.

Link: OP-TEE#5966
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Jianqiang,Mei <meijianqiang.mjq@alibaba-inc.com>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
.a1 = (uint32_t)(cookie), /* handle high addr */
.a2 = (uint32_t)(cookie >> 32), /* handle low addr */
.a3 = next_frag_offset, /* offset */
.a4 = 0, /* MBZ */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment can be skipped. But a comment would be nice, saying something like:

/* w4 MBZ since we're a virtual instance */

Copy link
Author

@Jianqiang-Mei Jianqiang-Mei Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a4 assignment skipped, how to pass check in el3 spmc, like
https://github.com/ARM-software/arm-trusted-firmware/blob/e318411f02f8a9526c97a23d0cdada2f128446e3/services/std_svc/spm/el3_spmc/spmc_shared_mem.c#L1682
and check in Hafnium

	/* Sender ID MBZ at virtual instance. */
	if (vm_id_is_current_world(to->id)) {
		if (sender_vm_id != 0) {
			dlog_verbose("%s: Invalid sender.\n", __func__);
			return ffa_error(FFA_INVALID_PARAMETERS);
		}
	}

Copy link
Author

@Jianqiang-Mei Jianqiang-Mei Jul 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean
.a4 = 0, /* w4 MBZ since we're a virtual instance */

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, just the comment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@jenswi-linaro
Copy link
Contributor

By the way, how do you test this patch?

@Jianqiang-Mei
Copy link
Author

By the way, how do you test this patch?

In the case of hello_world, you can add a const global variable to hello_world_ta.c, like:
const char test_char[10 * 1024 * 1024];
After build, TA's size will be increased by 10MB.
In this case, linux kernel will most likely allocate much discontinuous fragment memory,
and an rx_buffer will not be enough to include the address information of these shared memory,
and FFA_MEM_FRAG_RX is then used.

@Jianqiang-Mei
Copy link
Author

Jianqiang-Mei commented Jul 14, 2023

By the way, how do you test this patch?

In the case of hello_world, you can add a const global variable to hello_world_ta.c, like: const char test_char[10 * 1024 * 1024]; After build, TA's size will be increased by 10MB. In this case, linux kernel will most likely allocate much discontinuous fragment memory, and an rx_buffer will not be enough to include the address information of these shared memory, and FFA_MEM_FRAG_RX is then used.

@jenswi-linaro If a4 assignment skipped, can you pass this test?
Is there anything else need I to do for this point, or leave it to you?

@jenswi-linaro
Copy link
Contributor

jenswi-linaro commented Jul 14, 2023

I can't get that to work, I don't even reach spmc_frag_retrieve(). I'm testing this with:
OP-TEE/manifest#241
OP-TEE/build#663
make SPMC_AT_EL=2

Edit: On QEMU.

@Jianqiang-Mei
Copy link
Author

Jianqiang-Mei commented Jul 18, 2023

I can't get that to work, I don't even reach spmc_frag_retrieve(). I'm testing this with: OP-TEE/manifest#241 OP-TEE/build#663 make SPMC_AT_EL=2

Edit: On QEMU.

It seems to be related to changes to thread_smccc(&ffa_rx_release_args).
I'll push a new version.

Add implementation for memory retrieving for the remaining fragmented
transactions.

Link: OP-TEE#5966
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Jianqiang,Mei <meijianqiang.mjq@alibaba-inc.com>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
@Jianqiang-Mei
Copy link
Author

Jianqiang-Mei commented Jul 30, 2023

@jenswi-linaro
Sorry for the lack of communication over the last two weeks.
I tested this fix myself and it works fine, please let me know if there are any other problems.

@jenswi-linaro
Copy link
Contributor

I'm not able to reproduce it. Can you see if you can on QEMU? I'm using this setup:

repo init -u https://github.com/OP-TEE/manifest.git -m qemu_v8.xml
repo sync
cd build
make toolchains
# Update optee_os to use your patch +
# increase MAX_XLAT_TABLES to avoid a panic
# Update optee_examples, hello world TA with const char test_char[10 * 1024 * 1024]
make SPMC_AT_EL=2 CFG_NUM_THREADS=6 all
make SPMC_AT_EL=2 run-only

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Sep 1, 2023
@github-actions github-actions bot closed this Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants