Enhance rpmsg buf config by tnmysh · Pull Request #684 · OpenAMP/open-amp

tnmysh · 2026-04-29T03:46:25Z

Corresponding Linux kernel patch series: https://lore.kernel.org/all/20251114184640.3020427-1-tanmay.shah@amd.com/

tnmysh · 2026-04-29T03:47:06Z

we can review this one after the release.

arnopo · 2026-06-02T09:02:51Z

+			return status;
+
+		if (features & (1 << VIRTIO_RPMSG_F_BUFSZ))
+			rvdev->config = *config;


Here you should use virtio_read_config to retrieve the config from the transport layer
Did you have a look to @xiaoxiang781216 work in #155 ?
Seems to me the good approach

@arnopo done in latest push.

edmooring

Looks good to go.

arnopo · 2026-06-19T08:33:02Z

+
+		if (features & (1 << VIRTIO_RPMSG_F_BUFSZ))
+			virtio_write_config(rvdev->vdev, 0, (void *)config,
+					    sizeof(*config));


The config could be read by the virtio driver before this code is executed. For instance the Linux probes the rpmsg in parallel of the start of the remoteproc firmware based on the resource table that should contain the config.
For my here we should only read the config if VIRTIO_RPMSG_F_BUFSZ feature bit is set else we should rely on *config.
One question is: should we also mange the config write or should we consider that it is part of the resource table.

I propose to discuss this in next OpenAMP meeting

7/1 OpenAMP System Reference call:

Arnaud to confirm it is a standard pattern w/ MMIO

Tanmay to confirm would work to have read instead of write for current use case & update accordingly.

Allow write only for use case when it is compiled as driver.

Extracted from virtio spec 1 .4:

2.5.2 Device Requirements: Device Configuration Space The device MUST allow reading of any device-specific configuration field before FEATURES_OK is set by the driver. This includes fields which are conditional on feature bits, as long as those feature bits are offered by the device.

3.2 Device Operation When operating the device, each field in the device configuration space can be changed by either the driver or the device. Whenever such a configuration change is triggered by the device, driver is notified. This makes it possible for drivers to cache device configuration, avoiding expensive configuration reads unless notified.

So it seems that having a static configuration as first step is quite simple and reliable

the device provides a static configuration space (in the resource table).

the driver can update it until FEATURES_OK is set

the device read the configuration space when DRIVER_OK is set

iuliana-prodan · 2026-07-15T15:30:21Z

 */
+METAL_PACKED_BEGIN
 struct rpmsg_virtio_config {
+	/** version of this struct */


This needs an update, right?
Because it doesn't match the Linux v5 header which dropped the reserved fields, so every field after version is now at a different offset.

Yes. same as mentioned in: OpenAMP/openamp-system-reference#106

Getting the whole picture with the Linux implementation. I wonder if updating this structure would not add confusion and extra management:

split_shpool field

updating rpmsg_virtio_config could break the API

what about introducing struct rpmsg_virtio_config_space structure?

@arnopo It's better if we introduce new PR for that. This PR is only to modify the default buffer size via vdev config space. So, for now I prefer to keep it as it is.

@arnopo It's better if we introduce new PR for that. This PR is only to modify the default buffer size via vdev config space. So, for now I prefer to keep it as it is.

As this also impacts the API, I would prefer not to implement it in two steps, especially if we introduce it in this release. In its current state, this would likely introduce regressions, for instance in NuttX:
https://github.com/apache/nuttx/blob/master/drivers/rpmsg/rpmsg_virtio_lite.c#L697

iuliana-prodan · 2026-07-15T15:33:23Z

 		 */
-		size = (int)virtqueue_get_desc_size(rvdev->rvq) -
-		       sizeof(struct rpmsg_hdr);
+		features = 0;


Redundant, is already zero-initialized at declaration.

iuliana-prodan · 2026-07-15T15:36:39Z

+			return RPMSG_ERR_DEV_STATE;
+		}
+
+		if (features & (1 << VIRTIO_RPMSG_F_BUFSZ)) {


Nitpick: 1U << for feature masks (here and in rpmsg_init_vdev_with_config).

iuliana-prodan · 2026-07-15T15:46:46Z

+			return status;
+
+		if (features & (1 << VIRTIO_RPMSG_F_BUFSZ))
+			virtio_write_config(rvdev->vdev, 0, (void *)config,


Return value ignored, the same happens for virtio_read_config above.

Ack, I will fix in next rev.

iuliana-prodan · 2026-07-15T15:47:59Z

+			return status;
+
+		if (features & (1 << VIRTIO_RPMSG_F_BUFSZ))
+			virtio_write_config(rvdev->vdev, 0, (void *)config,


sizeof(*config) writes the full struct including split_shpool, inconsistent with the advertised size field.

Ack, I will fix in next rev.

nathalie-ckc · 2026-07-15T16:12:36Z

2026-07-15 System Reference Call:
Tanmay to address Iulia's comments, but will make update to PR once the Linux kernel side is finalized. Tanmay has sent latest revision in Linux kernel.

nathalie-ckc · 2026-08-26T16:09:57Z

8/26/26 System Reference call:
Tanmay updated based on V7 on Linux side. A few small items to address in Mathieu's most recent feedback. open-amp reviewers can wait until Linux side is complete before reviewing.

nathalie-ckc · 2026-09-08T16:13:31Z

9/8/26: Mathieu merged the Linux part. Tanmay will rebase, then it will be ready for OpenAMP review

tnmysh · 2026-09-18T13:59:42Z

This PR is ready for review. @arnopo

arnopo · 2026-09-21T12:53:12Z

+	if (ret)
+		return RPMSG_ERR_DEV_STATE;
+
+	if (config.size < RPMSG_VIRTIO_CONFIG_SIZE ||


Check on the config version seems missing here and in rpmsg_virtio_write_config

agree - check version == 1 like linux does and reject sizes <= sizeof(struct rpmsg_hdr)

arnopo · 2026-09-21T12:57:59Z


 	if (VIRTIO_ROLE_IS_DRIVER(vdev)) {
+		if (features & (1U << VIRTIO_RPMSG_F_BUFSZ)) {
+			status = rpmsg_virtio_write_config(rvdev);


This part seems to me not valid. The driver should only read the config. It must not update it.

it seems that something is also wrong in rproc_virtio_write_config

Yeah, this part is redundant. I will remove it.

arnopo · 2026-09-21T12:59:10Z

 	if (status)
 		return status;
-	rdev->support_ns = !!(features & (1 << VIRTIO_RPMSG_F_NS));
+	rdev->support_ns = !!(features & (1U << VIRTIO_RPMSG_F_NS));


Please move this in a separate Commit

iuliana-prodan · 2026-09-23T14:45:56Z

+	uint16_t size;
+
 	/** The size of the buffer used to send data from host to remote */
 	uint32_t h2r_buf_size;


The size fields are in the opposite order from the merged Linux abi https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/include/linux/rpmsg/virtio_rpmsg.h?h=for-next

This is expected, as it's parsed accordingly in the driver: https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/rpmsg/virtio_rpmsg_bus.c?h=for-next#n846

I wonder if it would be more obvious to talk about a driver and a device. What about drv2dev_buf_size?

@arnopo If we want to rename the structures field, can we do it in a different commit?
This name exists before this change, and irrelevant to the functionality being introduced in the PR.

iuliana-prodan · 2026-09-23T14:50:27Z

+	if (ret)
+		return RPMSG_ERR_DEV_STATE;
+
+	if (config.size < RPMSG_VIRTIO_CONFIG_SIZE ||


agree - check version == 1 like linux does and reject sizes <= sizeof(struct rpmsg_hdr)

arnopo · 2026-09-28T07:59:39Z

 	rdev->support_ns = !!(features & (1 << VIRTIO_RPMSG_F_NS));

 	if (VIRTIO_ROLE_IS_DRIVER(vdev)) {
+


To remove useless additional blank lines.

arnopo · 2026-09-28T08:21:26Z

 */
+METAL_PACKED_BEGIN
 struct rpmsg_virtio_config {
+	/** version of this struct */


Getting the whole picture with the Linux implementation. I wonder if updating this structure would not add confusion and extra management:

split_shpool field

updating rpmsg_virtio_config could break the API

what about introducing struct rpmsg_virtio_config_space structure?

arnopo · 2026-09-28T08:30:01Z

 	}

 	if (VIRTIO_ROLE_IS_DEVICE(vdev)) {
+		if (features & (1U << VIRTIO_RPMSG_F_BUFSZ)) {


It seems to me that this commit needs more rework

Legacy code provides struct rpmsg_virtio_config as argument of rpmsg_init_vdev_with_config function. This APi should still work if VIRTIO_RPMSG_F_BUFSZ is not set.

What about following algorithms

For the driver role:

if (features & (1U << VIRTIO_RPMSG_F_BUFSZ)) get the config from the rpmsg_virtio_config_space in shared memory else get the config from the rpmsg_virtio_config input argument

Notice that split_shpool should still be get from rpmsg_virtio_config

For the device role:
Current implementation should work and seems to me more flexible.

if (VIRTIO_ROLE_IS_DEVICE(rvdev->vdev)) { /* * If other core is host then buffers are provided by it, * so get the buffer size from the virtqueue. */ size = (int)virtqueue_get_desc_size(rvdev->rvq) - sizeof(struct rpmsg_hdr); }

@arnopo, I prefer this algorithm:

For the driver role:

If input param is provided to the API, then use it to override the one in the resource table, and then use resource table's parameters everywhere. If the input param is not provided, then assume it is provided by the resource table and use it directly from there. Let me know your view.

your proposal makes sense but will imply more update as the config argument is never null in our API regarding rpmsg_init_vdev function

Introduce new feature bit that allows rpmsg buffer size via virtio device config space. If the feature is available then, driver will set single rpmsg buffer size from virtio device config space in the resource table. Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>

tnmysh · 2026-10-01T16:04:55Z

@arnopo addressed all the comments.

tnmysh requested review from arnopo and xiaoxiang781216 April 29, 2026 03:46

tnmysh mentioned this pull request Apr 29, 2026

rpmsg buf size config via vdev config space. OpenAMP/openamp-system-reference#106

Open

tnmysh force-pushed the enhance_rpmsg_buf_config branch 2 times, most recently from 7310369 to 3474c42 Compare May 29, 2026 20:12

tnmysh marked this pull request as ready for review May 29, 2026 20:13

tnmysh requested a review from wmamills May 29, 2026 20:13

tnmysh force-pushed the enhance_rpmsg_buf_config branch 2 times, most recently from 6ddcf11 to 7c0b658 Compare May 30, 2026 02:47

arnopo requested changes Jun 2, 2026

View reviewed changes

arnopo requested a review from edmooring June 2, 2026 09:06

tnmysh force-pushed the enhance_rpmsg_buf_config branch 3 times, most recently from 8289cfd to a361586 Compare June 16, 2026 15:28

edmooring approved these changes Jun 17, 2026

View reviewed changes

wmamills requested a review from iuliana-prodan June 17, 2026 16:53

arnopo reviewed Jun 19, 2026

View reviewed changes

iuliana-prodan reviewed Jul 15, 2026

View reviewed changes

tnmysh force-pushed the enhance_rpmsg_buf_config branch from a361586 to 8211ae2 Compare August 17, 2026 14:17

tnmysh force-pushed the enhance_rpmsg_buf_config branch from 8211ae2 to 9a39a75 Compare September 17, 2026 19:33

arnopo requested review from arnopo, edmooring and nathalie-ckc September 21, 2026 12:18

arnopo requested changes Sep 21, 2026

View reviewed changes

iuliana-prodan reviewed Sep 23, 2026

View reviewed changes

tnmysh force-pushed the enhance_rpmsg_buf_config branch from 9a39a75 to c54c7e8 Compare September 24, 2026 20:17

arnopo requested changes Sep 28, 2026

View reviewed changes

tnmysh force-pushed the enhance_rpmsg_buf_config branch from c54c7e8 to 46bfee4 Compare September 29, 2026 20:49

		rdev->support_ns = !!(features & (1 << VIRTIO_RPMSG_F_NS));

		if (VIRTIO_ROLE_IS_DRIVER(vdev)) {

Conversation

tnmysh commented Apr 29, 2026

Uh oh!

tnmysh commented Apr 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edmooring left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathalie-ckc commented Jul 15, 2026

Uh oh!

nathalie-ckc commented Aug 26, 2026

Uh oh!

nathalie-ckc commented Sep 8, 2026

Uh oh!

tnmysh commented Sep 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnopo Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnmysh Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnopo Sep 21, 2026 •

edited

Loading

tnmysh Sep 24, 2026 •

edited

Loading

tnmysh Sep 28, 2026 •

edited

Loading

tnmysh commented Oct 1, 2026 •

edited

Loading