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

rpmsg: reserve 0-1023 for predefined services in rpmsg_create_ept #209

Merged
merged 1 commit into from
May 29, 2020

Conversation

xiaoxiang781216
Copy link
Collaborator

@xiaoxiang781216 xiaoxiang781216 commented May 16, 2020

and allocate the dynamic address from 1024 like Linux kernel

Signed-off-by: Xiang Xiao [email protected]

@xiaoxiang781216
Copy link
Collaborator Author

caller can still pass the fixed address from 0 to 1023, but rpmsg_create_ept allocate the dynamic address from 1024.

@arnopo
Copy link
Collaborator

arnopo commented May 20, 2020

I think here it is important that we are aligned first on the Linux implementation.
In rpmsg/virtio_rpmsg_bus.c the reserved address are defined by:

/*
 * Local addresses are dynamically allocated on-demand.
 * We do not dynamically assign addresses from the low 1024 range,
 * in order to reserve that address range for predefined services.
 */
#define RPMSG_RESERVED_ADDRESSES	(1024)

So here the main point here is to understand what is a predefined service

Now let's have look at __rpmsg_create_ept to understand how RPMSG_RESERVED_ADDRESSES is used

	/* do we need to allocate a local address ? */
	if (addr == RPMSG_ADDR_ANY) {
		id_min = RPMSG_RESERVED_ADDRESSES;
		id_max = 0;
	} else {
		id_min = addr;
		id_max = addr + 1;
	}

Regarding this code a predefined service is a service with a fixed address. And as this function can be called through rpmsg_create_ept API, any user can define a "predefined" service by specifying the chinfo.src address.
So if i well understand, the Linux implementation rule is:

  • service based on a service name will have an address >= 1024
  • address from 0 to 1023 are reserved for services with a fixed address on endpoint creation. fixed address can be used either par a core or a user service.

Did i miss something?

@xiaoxiang781216
Copy link
Collaborator Author

xiaoxiang781216 commented May 20, 2020

Yes, your understand is correct. This patch align OpenAMP implementation with Linux kernel. The only difference is we trust that caller don't pass the duplicated address inside 0-1023 and then bypass the check to simplify the implemntation.

@arnopo
Copy link
Collaborator

arnopo commented May 25, 2020

This version seems to me a good compromise that would match with Linux implementation, while keeping compatibility with existing implementation:

  • address 0 to 1023: predefined address only, no control on source address
  • address 1024 to 1151 shared between dynamically allocated and predefined address with a control of the address
  • higher than 1151 not supported

I just need to do few more test on my side before approving it.

Then the title of this patch would need to be reworked as rpmsg_create_ept can allocated reserved address.
Some comments in code describing the address management would also be useful to understand the new behavior, especially the fact that address between 0 and 1023 are not checked.

@edmooring: could you review it and give your opinion please?

and allocate the dynamic address from 1024 like Linux kernel

Signed-off-by: Xiang Xiao <[email protected]>
@xiaoxiang781216 xiaoxiang781216 changed the title rpmsg: shouldn't allocate 0-1023 address in rpmsg_create_ept rpmsg: reserve 0-1023 for predefined services in rpmsg_create_ept May 26, 2020
@xiaoxiang781216
Copy link
Collaborator Author

Then the title of this patch would need to be reworked as rpmsg_create_ept can allocated reserved address.
Some comments in code describing the address management would also be useful to understand the new behavior, especially the fact that address between 0 and 1023 are not checked.

@arnopo done.

@xiaoxiang781216
Copy link
Collaborator Author

@arnopo could you merge the follow patch first:
#209
#207
#204
Then I can rebase other patch on top of them.

@arnopo arnopo requested a review from edmooring May 26, 2020 09:14
Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

Aside from my comments, looks good to go.

lib/include/openamp/rpmsg.h Show resolved Hide resolved
lib/rpmsg/rpmsg.c Show resolved Hide resolved
Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

Good to go.

@arnopo arnopo merged commit a3a46ab into OpenAMP:master May 29, 2020
@xiaoxiang781216 xiaoxiang781216 deleted the rpmsg-create-ept branch October 31, 2020 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants