Skip to content

Enhance rpmsg buf config #684

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 1 commit into
base: main
Choose a base branch
from
Open

Enhance rpmsg buf config #684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 16 additions & 1 deletion lib/include/openamp/rpmsg_virtio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <metal/io.h>
#include <metal/mutex.h>
#include <metal/cache.h>
#include <metal/compiler.h>
#include <openamp/rpmsg.h>
#include <openamp/virtio.h>

Expand All @@ -29,6 +30,7 @@ extern "C" {

/* The feature bitmap for virtio rpmsg */
#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
#define VIRTIO_RPMSG_F_BUFSZ 1 /* fw provides tx and rx single buf size */

#if defined(VIRTIO_USE_DCACHE)
#define BUFFER_FLUSH(x, s) metal_cache_flush(x, s)
Expand Down Expand Up @@ -59,7 +61,20 @@ struct rpmsg_virtio_shm_pool {
* This structure is used by the RPMsg virtio host to configure the virtiio
* layer.
*/
METAL_PACKED_BEGIN
struct rpmsg_virtio_config {
/** version of this struct */
uint8_t version;

uint8_t reserved;

/** size of the config space */
uint16_t size;

uint16_t alignment;

uint16_t reserved1;

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

Expand All @@ -68,7 +83,7 @@ struct rpmsg_virtio_config {

/** The flag for splitting shared memory pool to TX and RX */
bool split_shpool;
};
} METAL_PACKED_END;

/** @brief Representation of a RPMsg device based on virtio */
struct rpmsg_virtio_device {
Expand Down
34 changes: 31 additions & 3 deletions lib/rpmsg/rpmsg_virtio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ struct vbuff_reclaimer_t {
#if VIRTIO_ENABLED(VIRTIO_DRIVER_SUPPORT)
#define RPMSG_VIRTIO_DEFAULT_CONFIG \
(&(const struct rpmsg_virtio_config) { \
.version = 1, \
.size = sizeof(struct rpmsg_virtio_config) - sizeof(bool), \
.alignment = 0, \
.h2r_buf_size = RPMSG_BUFFER_SIZE, \
.r2h_buf_size = RPMSG_BUFFER_SIZE, \
.split_shpool = false, \
Expand Down Expand Up @@ -741,8 +744,10 @@ int rpmsg_virtio_get_tx_buffer_size(struct rpmsg_device *rdev)

int rpmsg_virtio_get_rx_buffer_size(struct rpmsg_device *rdev)
{
struct rpmsg_virtio_config vdev_config = {0};
struct rpmsg_virtio_device *rvdev;
int size = 0;
int size = 0, ret;
uint32_t features = 0;

if (!rdev)
return RPMSG_ERR_PARAM;
Expand All @@ -762,9 +767,24 @@ int rpmsg_virtio_get_rx_buffer_size(struct rpmsg_device *rdev)
/*
* If other core is host then buffers are provided by it,
* so get the buffer size from the virtqueue.
* If virtio device has BUFSZ feature, then the rx buffer is
* provided by the vdev config space in the resource table.
*/
size = (int)virtqueue_get_desc_size(rvdev->rvq) -
sizeof(struct rpmsg_hdr);
features = 0;
ret = virtio_get_features(rvdev->vdev, &features);
if (ret) {
metal_mutex_release(&rdev->lock);
return RPMSG_ERR_DEV_STATE;
}

if (features & (1 << VIRTIO_RPMSG_F_BUFSZ)) {
virtio_read_config(rvdev->vdev, 0, &vdev_config,
sizeof(vdev_config));
size = vdev_config.r2h_buf_size - sizeof(struct rpmsg_hdr);
} else {
size = (int)virtqueue_get_desc_size(rvdev->rvq) -
sizeof(struct rpmsg_hdr);
}
}

if (size <= 0)
Expand Down Expand Up @@ -830,6 +850,14 @@ int rpmsg_init_vdev_with_config(struct rpmsg_virtio_device *rvdev,
}

if (VIRTIO_ROLE_IS_DEVICE(vdev)) {
status = virtio_get_features(vdev, &features);
if (status)
return status;

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

@arnopo arnopo Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@nathalie-ckc nathalie-ckc Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@arnopo arnopo Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

  1. the device provides a static configuration space (in the resource table).
  2. the driver can update it until FEATURES_OK is set
  3. the device read the configuration space when DRIVER_OK is set


/* wait synchro with the host */
status = rpmsg_virtio_wait_remote_ready(rvdev);
if (status)
Expand Down
Loading