Rpmsg multi services upstream cleanup by bentheredonethat · Pull Request #115 · OpenAMP/openamp-system-reference

bentheredonethat · 2026-08-03T18:05:16Z

No description provided.

tnmysh · 2026-08-10T16:35:34Z


 project(rpmsg_multi_services)

+if(CONFIG_OPENAMP_EXTERNAL_LINKER_SCRIPT)


Where is this variable used? Is it in AMD-Xilinx's downstream zephyr?

If so, this is not standard for other platforms. We need to discuss process to allow custom linker script use for every platform.

By default, each platform uses "SOC_LINKER_SCRIPT" variable. Can we override it via command line? and then in the default zephyr assign default linker script only if that variable is not set?

@arnopo do you have any opinion on this?

Or, Zephyr already provide options to use custom linker script we can use that:

CONFIG_HAVE_CUSTOM_LINKER_SCRIPT=y
CONFIG_CUSTOM_LINKER_SCRIPT="linker_xtensa_intel_adsp_cavs.ld"

We can pass CONFIG_HAVE_CUSTOM_LINKER_SCRIPT=y in the prj.conf, and CONFIG_CUSTOM_LINKER_SCRIPT="ld path" in the west command line.

What do you all think?

@bentheredonethat, @arnopo

@tnmysh i am fine to remove and build instead with

I am fine to remove this @tnmysh

we can replace with

west build -p always -b
openamp-system-reference/examples/zephyr/rpmsg_multi_services --
-DDTS_SOURCE=/absolute/path/to/rpu_zephyr.dts
-DCONFIG_HAVE_CUSTOM_LINKER_SCRIPT=y
-DCONFIG_CUSTOM_LINKER_SCRIPT='"/absolute/path/to/openamp-zephyr.ld"'

CONFIG_HAVE_CUSTOM_LINKER_SCRIPT=y have this in conf file,

and only pass the path from the command line.

tnmysh · 2026-08-10T16:40:18Z

+   west build -p always -b <target board> \
+     openamp-system-reference/examples/zephyr/rpmsg_multi_services -- \
+     -DDTS_SOURCE=/absolute/path/to/rpu_zephyr.dts \
+     -DCONFIG_OPENAMP_EXTERNAL_LINKER_SCRIPT='"/absolute/path/to/openamp-zephyr.ld"'


Here, we should pass CONFIG_CUSTOM_LINKER_SCRIPT directly. No need of another config option for the same purpose.

sure -- as noted above

tnmysh · 2026-08-10T16:42:39Z

 		exit(1);
 	}
-	ret = read(fd, buffer, 256);
+	ret = read(fd, buffer, sizeof(buffer) - 1);


The size of buffer should come from rpmsg buffer size. Which I believe is fixed to 512 bytes fix for now. Why can't we keep it 512 bytes?

Agreed on increasing the local receive buffer to 512 bytes. I’ll retain sizeof(buffer) in the read() call so the bound remains tied to the destination array rather than duplicating its size as a literal.

tnmysh · 2026-08-10T16:44:58Z

 		fprintf(stderr, "failed to read endpoint %s", argv[1]);
 		exit(1);
 	}
+	buffer[ret] = '\0';


So, read on rpmsg device will always return 512. So, no point on NULL terminating it at the end of 512 bytes, I guess. Instead, we should always print only 512 bytes and stop printing if '\0' is found in between.

Edit: So, above buffer[ret] = '\0' not needed.

read() returns the actual RPMsg message length rather than always returning 512 bytes. However, I agree that modifying the buffer is unnecessary if the print is explicitly bounded.

I’ll read up to sizeof(buffer) and use:
fprintf(stderr, "message received: \"%.*s\"\n", (int)ret, buffer);
This prints at most the number of bytes returned by read() and stops earlier if a NUL is present. I’ll remove buffer[ret] = '\0'.

tnmysh · 2026-08-10T16:47:23Z

 		exit(1);
 	}
+	buffer[ret] = '\0';
 	fprintf(stderr, "message received: \"%s\"\n", buffer);


If you really want, see if there something like fnprintf, which allwows to print fix number of bytes. If not, you can use snprintf.

there is not -- i have current latest as:
fprintf(stderr, "message received: \"%.*s\"\n", (int)ret, buffer);

tnmysh · 2026-08-10T16:48:29Z

 #define MAX_TTY_EPT  2
 #define MAX_RAW_EPT  2

+#define TTY_RESPONSE_PREFIX_LEN (sizeof("TTY 0: ") - 1U)


In response prefix we don't need device id i.e. "0".

Infact, I don't understand purpose of this. It doesn't match with the commit text.

"TTY 0: " was only being used to calculate the length of the existing "TTY %d: " prefix; it was not replacing the formatted endpoint ID. I agree that the macro makes this unnecessarily obscure, so I’ll remove it.

The underlying one-line correction is still needed: the payload is copied at offset 7, but the existing code sends len + 8, which includes one byte beyond the prefix and payload. I’ll reduce the hunk to the direct len + 8 to len + 7 correction and explicitly mention the TTY off-by-one in the commit message.

tnmysh · 2026-08-10T16:50:06Z

+				       tty_msg[i].data, tty_msg[i].len);
+				rpmsg_send(&tty_ept[i], tx_buff,
+					   tty_msg[i].len +
+					   TTY_RESPONSE_PREFIX_LEN);


This whole hunk can be dropped. It's not related to the commit text, and not needed at all.

tnmysh · 2026-08-10T16:51:01Z

+				     raw_msg[i].len] = '\0';
+				rpmsg_sendto(&raw_ept[i], buff,
+					     raw_msg[i].len +
+					     RAW_RESPONSE_PREFIX_LEN + 1U,


Same for this hunk too. I don't understand why it's needed.

I guess, can you show what problem you are facing via logs that needs this commit?

The failure is visible as trailing garbage:

message received: "from ept 0x0402: ping /dev/rpmsg0Eү??" message received: "from ept 0x0001: ping /dev/rpmsg1E????"

The raw prefix is 17 bytes. The current memcpy() starts at offset 17 and overwrites the NUL written by snprintf(), but rpmsg_sendto() sends payload length + 18. It therefore includes one byte
beyond the prefix and payload. A maximum-sized incoming payload can also exceed the 512-byte response buffer once the prefix is added.

I’ll simplify the fix: use the return value from snprintf() as the actual prefix length, check prefix length + payload length against the response buffer, and transmit exactly that many bytes.
No NUL needs to be put on the wire because RPMsg is length-framed. The Linux utility will bound its output using the length returned by read().

Configure the RPMsg multi-services example to use the AMD IPI mailbox on Versal R5. Keep memory and linker placement outside the application so generated platform inputs remain the source of the RPU memory layout. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>

bentheredonethat · 2026-08-10T18:30:50Z

@tnmysh i have updated per my findings and your review comments -- i have tested latest on hardware to confirm

tnmysh · 2026-08-10T19:00:03Z

 	/* declare resource table region */
-	rsc_table_get(&rsc_table, &rcs_size);
+	rsc_table_get(&rsc_table_ptr, &rcs_size);
+	rsc_table = rsc_table_ptr;


I think this whole commit can be dropped too. rsc_table, and rsc_table_ptr both are (void *) same type. I don't know what is solved here.

if incompatible pointer type is not seen now, it shouldn't be seen with the original patch too.

Please check again, if this is needed or not. Could you paste original error?

ok can drop this commit - thanks @tnmysh

The raw and TTY responders transmit one byte beyond their formatted prefix and payload. In the raw path this can expose stale buffer data, and adding the prefix to a maximum-sized received message can exceed the local response buffer. Send exact prefix-plus-payload lengths and validate raw response construction. Increase the Linux utility's local receive buffer and bound text output by the length returned from read(). Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>

Linux can own the physical UART while loading an RPU application through remoteproc. Disable serial support for the AMD RPU targets and use the RAM console where required so the example does not contend for that UART. Keep immediate logging for Versal Net, enable OpenAMP cache maintenance on Versal2, and retain the executable-memory settings used by the generated R52 memory policies. Represent the supported platforms and tags as YAML sequences so Twister retains every entry and can build all AMD RPU targets. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>

tnmysh reviewed Aug 10, 2026

View reviewed changes

bentheredonethat force-pushed the rpmsg-multi-services-upstream-cleanup branch from 2148050 to 47ad029 Compare August 10, 2026 18:31

tnmysh reviewed Aug 10, 2026

View reviewed changes

bentheredonethat added 2 commits August 10, 2026 12:02

bentheredonethat force-pushed the rpmsg-multi-services-upstream-cleanup branch from 47ad029 to be82acf Compare August 10, 2026 19:02

tnmysh approved these changes Aug 10, 2026

View reviewed changes

tnmysh requested review from arnopo and edmooring August 11, 2026 14:46


		project(rpmsg_multi_services)

		if(CONFIG_OPENAMP_EXTERNAL_LINKER_SCRIPT)

Uh oh!

Conversation

bentheredonethat commented Aug 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnmysh Aug 10, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnmysh Aug 10, 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!

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!

bentheredonethat commented Aug 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tnmysh Aug 10, 2026 •

edited

Loading

tnmysh Aug 10, 2026 •

edited

Loading