Skip to content

legacy_apps: xlnx: add api to poll apps on vdev status #100

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.

Sign up for GitHub

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

Open
tnmysh wants to merge 2 commits into
base: main
Choose a base branch
from
Open

legacy_apps: xlnx: add api to poll apps on vdev status #100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6812ef4
legacy_apps: xlnx: add api to poll apps on vdev status
tnmysh Mar 13, 2026
4608802
legacy_apps: recreate rpmsg devices after vdev reset
tnmysh Mar 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions examples/legacy_apps/examples/echo/freertos/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,18 +48,21 @@ static void rpmsg_listen_task(void *arg)
LPRINTF("libmetal lib version: %s\n", metal_ver());
LPRINTF("Starting application...\r\n");

while (1) {
do {
rpdev = platform_create_rpmsg_vdev(platform, 0,
VIRTIO_DEV_DEVICE,
NULL, NULL);
if (!rpdev) {
LPERROR("Failed to create rpmsg virtio device.\r\n");
break;
ret = 0;
Copy link
Copy Markdown
Collaborator

@arnopo arnopo Apr 10, 2026

Choose a reason for hiding this comment

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

Suggested change
ret = 0;
ret = -1;

} else {
rpmsg_echo_app(rpdev, platform);
ret = rpmsg_echo_app(rpdev, platform);
if (ret)
metal_err("rpmsg_echo_app failed with err, %d\n",
ret);
platform_release_rpmsg_vdev(rpdev, platform);
}
}
} while (!ret);

LPRINTF("Stopping application...\r\n");
platform_cleanup(platform);
Expand Down
25 changes: 13 additions & 12 deletions examples/legacy_apps/examples/echo/generic/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,22 +32,23 @@ int main(int argc, char *argv[])
return ret;
}


LPRINTF("openamp lib version: %s\n", openamp_version());
LPRINTF("libmetal lib version: %s\n", metal_ver());
LPRINTF("Starting application...\r\n");

rpdev = platform_create_rpmsg_vdev(platform, 0,
VIRTIO_DEV_DEVICE,
NULL, NULL);
if (!rpdev) {
LPERROR("Failed to create rpmsg virtio device.\r\n");
ret = -1;
} else {
rpmsg_echo_app(rpdev, platform);
platform_release_rpmsg_vdev(rpdev, platform);
ret = 0;
}
/* support repeate attach-detach by recreating virtio devices on vdev reset */
do {
rpdev = platform_create_rpmsg_vdev(platform, 0,
VIRTIO_DEV_DEVICE,
NULL, NULL);
if (!rpdev) {
LPERROR("Failed to create rpmsg virtio device.\r\n");
ret = -1;
} else {
ret = rpmsg_echo_app(rpdev, platform);
platform_release_rpmsg_vdev(rpdev, platform);
}
} while (!ret);

LPRINTF("Stopping application...\r\n");
platform_cleanup(platform);
Expand Down
38 changes: 19 additions & 19 deletions examples/legacy_apps/examples/echo/rpmsg-echo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ int rpmsg_echo_app(struct rpmsg_device *rdev, void *priv)
rpmsg_service_unbind);
if (ret) {
LPERROR("Failed to create endpoint.\r\n");
return -1;
return ret;
}

metal_log(METAL_LOG_NOTICE,
Expand All @@ -82,16 +82,12 @@ int rpmsg_echo_app(struct rpmsg_device *rdev, void *priv)
metal_dbg("RPMsg device TX buffer size: %#x\r\n", rpmsg_get_tx_buffer_size(&lept));
metal_dbg("RPMsg device RX buffer size: %#x\r\n", rpmsg_get_rx_buffer_size(&lept));

while(1) {
platform_poll(priv);
/* we got a shutdown request, exit */
if (shutdown_req) {
break;
}
}
ret = platform_poll(priv);
Copy link
Copy Markdown
Collaborator

@arnopo arnopo Apr 10, 2026

Choose a reason for hiding this comment

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

This make break all machines implementation not only the AMD ones.

Copy link
Copy Markdown
Collaborator Author

@tnmysh tnmysh Apr 13, 2026

Choose a reason for hiding this comment

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

I don't know how it can break. Remote firmware will still work as it is.

Copy link
Copy Markdown
Collaborator

@arnopo arnopo Apr 13, 2026

Choose a reason for hiding this comment

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

Not tested but, , for instance, seems not compatible with following codes:
k

if (ret)
metal_err("platform poll failed err = %d\n", ret);
rpmsg_destroy_ept(&lept);

return 0;
return ret;
}

/*-----------------------------------------------------------------------------*
Expand Down Expand Up @@ -122,17 +118,21 @@ int __attribute__((weak)) main(int argc, char *argv[])

LPRINTF("Starting application...\r\n");

rpdev = platform_create_rpmsg_vdev(platform, 0,
VIRTIO_DEV_DEVICE,
NULL, NULL);
if (!rpdev) {
LPERROR("Failed to create rpmsg virtio device.\r\n");
ret = -1;
} else {
rpmsg_echo_app(rpdev, platform);
platform_release_rpmsg_vdev(rpdev, platform);
/* keep repeating application until it fails */
do {
ret = 0;
}
rpdev = platform_create_rpmsg_vdev(platform, 0,
VIRTIO_DEV_DEVICE,
NULL, NULL);
if (!rpdev) {
LPERROR("Failed to create rpmsg virtio device.\r\n");
ret = -1;
} else {
ret = rpmsg_echo_app(rpdev, platform);
platform_release_rpmsg_vdev(rpdev, platform);
}
LPRINTF("Creating vdev devices again\r\n");
} while (!ret);

LPRINTF("Stopping application...\r\n");
platform_cleanup(platform);
Expand Down
12 changes: 6 additions & 6 deletions examples/legacy_apps/examples/matrix_multiply/freertos/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,18 @@ static void rpmsg_listen_task(__attribute__((unused)) void *arg)

LPRINTF("Starting application...\r\n");

while (1) {
do {
rpdev = platform_create_rpmsg_vdev(platform, 0,
VIRTIO_DEV_DEVICE,
NULL, NULL);
if (!rpdev) {
LPERROR("Failed to create rpmsg virtio device.\r\n");
break;
ret = -EINVAL;
} else {
ret = rpmsg_matrix_app(rpdev, platform);
platform_release_rpmsg_vdev(rpdev, platform);
}

rpmsg_matrix_app(rpdev, platform);
platform_release_rpmsg_vdev(rpdev, platform);
}
} while (!ret);

LPRINTF("Stopping application...\r\n");
platform_cleanup(platform);
Expand Down
24 changes: 13 additions & 11 deletions examples/legacy_apps/examples/matrix_multiply/generic/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,19 @@ int main(int argc, char *argv[])
LPRINTF("libmetal lib version: %s\n", metal_ver());
LPRINTF("Starting application...\r\n");

rpdev = platform_create_rpmsg_vdev(platform, 0,
VIRTIO_DEV_DEVICE,
NULL, NULL);
if (!rpdev) {
LPERROR("Failed to create rpmsg virtio device.\r\n");
ret = -1;
} else {
rpmsg_matrix_app(rpdev, platform);
platform_release_rpmsg_vdev(rpdev, platform);
ret = 0;
}
/* on vdev reset, recreate rpmsg devices */
do {
rpdev = platform_create_rpmsg_vdev(platform, 0,
VIRTIO_DEV_DEVICE,
NULL, NULL);
if (!rpdev) {
LPERROR("Failed to create rpmsg virtio device.\r\n");
ret = -EINVAL;
Copy link
Copy Markdown
Collaborator

@arnopo arnopo Apr 10, 2026

Choose a reason for hiding this comment

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

sometime -1 is returned, sometime -EINVAL. What about using EPERM or ENXIO everywhere

} else {
ret = rpmsg_matrix_app(rpdev, platform);
platform_release_rpmsg_vdev(rpdev, platform);
}
} while (!ret);

LPRINTF("Stopping application...\r\n");
platform_cleanup(platform);
Expand Down
27 changes: 13 additions & 14 deletions examples/legacy_apps/examples/matrix_multiply/matrix_multiplyd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,24 +105,20 @@ int rpmsg_matrix_app(struct rpmsg_device *rdev, void *priv)
rpmsg_service_unbind);
if (ret) {
LPERROR("Failed to create endpoint.\r\n");
return -1;
return ret;
}

metal_log(METAL_LOG_NOTICE,
"created rpmsg channel %s, src=0x%x, dst=0x%x\r\n",
RPMSG_SERVICE_NAME, lept.addr, lept.dest_addr);

LPRINTF("Waiting for events...\r\n");
while(1) {
platform_poll(priv);
/* we got a shutdown request, exit */
if (shutdown_req) {
break;
}
}
ret = platform_poll(priv);
if (ret)
metal_err("platform_poll failed with error %d\n", ret);
rpmsg_destroy_ept(&lept);

return 0;
return ret;
}

/*-----------------------------------------------------------------------------*
Expand All @@ -140,20 +136,23 @@ int __attribute__((weak)) main(int argc, char *argv[])
ret = platform_init(argc, argv, &platform);
if (ret) {
LPERROR("Failed to initialize platform.\r\n");
ret = -1;
} else {
return ret;
}

do {
rpdev = platform_create_rpmsg_vdev(platform, 0,
VIRTIO_DEV_DEVICE,
NULL, NULL);
if (!rpdev) {
LPERROR("Failed to create rpmsg virtio device.\r\n");
ret = -1;
ret = -EINVAL;
} else {
rpmsg_matrix_app(rpdev, platform);
ret = rpmsg_matrix_app(rpdev, platform);
platform_release_rpmsg_vdev(rpdev, platform);
ret = 0;
}
LPRINTF("creating rpmsg vdev devices again\r\n");
}
while (!ret);

LPRINTF("Stopping application...\r\n");
platform_cleanup(platform);
Expand Down
39 changes: 34 additions & 5 deletions examples/legacy_apps/machine/xlnx/zynqmp_r5/platform_info.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <metal/sys.h>
#include <metal/utilities.h>
#include <openamp/rpmsg_virtio.h>
#include <openamp/virtio.h>

#include "platform_info.h"
#include "rsc_table.h"
Expand Down Expand Up @@ -414,20 +415,42 @@ platform_create_rpmsg_vdev(void *platform, unsigned int vdev_index,

int platform_poll(void *priv)
{
struct rpmsg_virtio_device *rpmsg_vdev;
struct remoteproc_virtio *rproc_vdev;
struct remoteproc *rproc = priv;
struct remoteproc_priv *prproc;
struct metal_list *node;
uint8_t vdev_status = 0;
unsigned int flags;
int ret;

prproc = rproc->priv;
while(1) {
/* If vdev reset is issued, then stop polling */
node = metal_list_first(&rproc->vdevs);
if (!node) {
metal_err("failed to get rproc_vdev node\n");
return -EINVAL;
}

rproc_vdev = metal_container_of(node,
struct remoteproc_virtio,
node);
rpmsg_vdev = rproc_vdev->vdev.priv;
if (!rpmsg_vdev) {
metal_err("failed to get rpmsg_vdev\n");
return -ENODEV;
}
if (!rpmsg_vdev->vdev) {
metal_err("failed to get virtio device\n");
return -ENODEV;
}
do {
#ifdef RPMSG_NO_IPI
if (metal_io_read32(prproc->kick_io, 0)) {
ret = remoteproc_get_notification(rproc,
RSC_NOTIFY_ID_ANY);
if (ret)
return ret;
break;
}
(void)flags;
#else /* !RPMSG_NO_IPI */
Expand All @@ -438,12 +461,18 @@ int platform_poll(void *priv)
RSC_NOTIFY_ID_ANY);
if (ret)
return ret;
break;
}
system_suspend();
metal_irq_restore_enable(flags);
system_suspend();
#endif /* RPMSG_NO_IPI */
}

ret = virtio_get_status(rpmsg_vdev->vdev, &vdev_status);
if (ret) {
metal_err("failed to get vdev status %d\n", ret);
return ret;
}
} while (vdev_status);
Copy link
Copy Markdown
Collaborator

@arnopo arnopo Apr 10, 2026

Choose a reason for hiding this comment

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

Except if I miss something this breaks the virtio spec protocol. The virtio driver must not set the status to 0.

2.4 Device Reset
The driver may want to initiate a device reset at various times; notably, it is required to do so during device initialization and device cleanup.

The mechanism used by the driver to initiate the reset is transport specific.

2.4.1 Device Requirements: Device Reset
A device MUST reinitialize device status to 0 after receiving a reset.

A device MUST NOT send notifications or interact with the queues after indicating completion of the reset by reinitializing device status to 0, until the driver re-initializes the device.

2.4.2 Driver Requirements: Device Reset
The driver SHOULD consider a driver-initiated reset complete when it reads device status as 0.

here the sequence should be

  • a reset request should come from the other side
  • The machine detect the reset in platform_poll() function and a a specif error ( perhaps -EPIPE).
  • generic application stop the rpmsg and restart it

Copy link
Copy Markdown
Collaborator Author

@tnmysh tnmysh Apr 10, 2026

Choose a reason for hiding this comment

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

So as per this virtio standard, this behavior is already broken in the linux kernel side. Because today we see virtio status set to 0 on detach() call. Perhaps rproc_reset_rsc_table_on_detach this function is restoring the default resource table in the linux kernel which sets vdev_status to 0 on detach.

Copy link
Copy Markdown
Collaborator

@arnopo arnopo Apr 10, 2026

Choose a reason for hiding this comment

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

Yes you are right, the Linux should not clean the vdev, status

Copy link
Copy Markdown
Collaborator Author

@tnmysh tnmysh Apr 10, 2026

Choose a reason for hiding this comment

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

@arnopo

How about this. We keep polling up until DRIVER_OKAY bit is set by the linux. If it's not setup anymore, then we stop polling. I think that's not breaking the virtio standard.

Copy link
Copy Markdown
Collaborator Author

@tnmysh tnmysh Apr 10, 2026

Choose a reason for hiding this comment

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

@arnopo

How about this. We keep polling up until DRIVER_OKAY bit is set by the linux. If it's not setup anymore, then we stop polling. I think that's not breaking the virtio standard.

Actually, that's not a good idea too. Because as per the spec, driver can't choose specific bits to reset.

Instead to reset the device, driver will set the status 0. So, are we not resetting the device when we issue detach() command?

Copy link
Copy Markdown
Collaborator

@arnopo arnopo Apr 13, 2026

Choose a reason for hiding this comment

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

From my point of view, we should think with a system-level approach rather than going with a temporary solution.

What about a solution based on a new status in the resource table, as discussed during the last OpenAMP RP call?

We should probably discuss the solution on the Linux kernel side before moving forward with this PR. The fact that Linux does not respect the virtio specification should help us reach a solution.

Copy link
Copy Markdown
Collaborator Author

@tnmysh tnmysh Apr 13, 2026

Choose a reason for hiding this comment

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

@arnopo Yes that's what I was thinking too.

The only problem is the backward compatibility from the linux standpoint.

If we implement the new status based on the resource table, and the remote firmware is polling based on current vdev status, then we may break the existing firmwares.

So, I was thinking in the linux side:

if (new_vdev_status in rsc tbl) then:
do not reset vdev status on detach, instead rely on fw to reset vdev status
else:
reset vdev status, and notify firmware.
endif

We can discuss this in the next system-reference or rp call. Which comes first.


return 0;
}

Expand Down