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.

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

virtio_mmio: Remove unneeded use of libmetal device #598

Merged
merged 2 commits into from
Jul 5, 2024
Merged

virtio_mmio: Remove unneeded use of libmetal device #598

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d17dff
virtio_mmio: Remove unneeded use of libmetal device
glneo Mar 18, 2024
2f38b65
virtio_mmio: Statically allocate metal_io_regions
glneo Jun 24, 2024
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
3 changes: 0 additions & 3 deletions lib/include/openamp/virtio_mmio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,6 @@ struct virtio_mmio_device {
/** Pre-shared memory space metal_io_region */
struct metal_io_region *shm_io;

/** Shared memory device */
struct metal_device shm_device;

/** VIRTIO device configuration space */
struct virtio_mmio_dev_mem cfg_mem;

Expand Down
48 changes: 11 additions & 37 deletions lib/virtio_mmio/virtio_mmio_drv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,46 +141,20 @@ const struct virtio_dispatch virtio_mmio_dispatch = {
static int virtio_mmio_get_metal_io(struct virtio_device *vdev, uintptr_t virt_mem_ptr,
uintptr_t cfg_mem_ptr)
{
struct metal_device *device;
int32_t err;
struct virtio_mmio_device *vmdev = metal_container_of(vdev,
struct virtio_mmio_device, vdev);

/* Setup shared memory device */
vmdev->shm_device.regions[0].physmap = (metal_phys_addr_t *)&vmdev->shm_mem.base;
vmdev->shm_device.regions[0].virt = (void *)virt_mem_ptr;
vmdev->shm_device.regions[0].size = vmdev->shm_mem.size;

VIRTIO_ASSERT((METAL_MAX_DEVICE_REGIONS > 1),
"METAL_MAX_DEVICE_REGIONS must be greater that 1");

vmdev->shm_device.regions[1].physmap = (metal_phys_addr_t *)&vmdev->cfg_mem.base;
vmdev->shm_device.regions[1].virt = (void *)cfg_mem_ptr;
vmdev->shm_device.regions[1].size = vmdev->cfg_mem.size;

err = metal_register_generic_device(&vmdev->shm_device);
if (err) {
metal_log(METAL_LOG_ERROR, "Couldn't register shared memory device: %d\n", err);
return err;
}

err = metal_device_open("generic", vmdev->shm_device.name, &device);
if (err) {
metal_log(METAL_LOG_ERROR, "metal_device_open failed: %d", err);
return err;
}

vmdev->shm_io = metal_device_io_region(device, 0);
if (!vmdev->shm_io) {
metal_log(METAL_LOG_ERROR, "metal_device_io_region failed to get region 0");
return err;
}

vmdev->cfg_io = metal_device_io_region(device, 1);
if (!vmdev->cfg_io) {
metal_log(METAL_LOG_ERROR, "metal_device_io_region failed to get region 1");
return err;
}
/* Setup shared memory region */
vmdev->shm_io = metal_allocate_memory(sizeof(*vmdev->shm_io));
metal_io_init(vmdev->shm_io, (void *)virt_mem_ptr,
(metal_phys_addr_t *)&vmdev->shm_mem.base,
vmdev->shm_mem.size, -1, 0, NULL);

/* Setup configuration region */
vmdev->cfg_io = metal_allocate_memory(sizeof(*vmdev->cfg_io));
metal_io_init(vmdev->cfg_io, (void *)cfg_mem_ptr,
(metal_phys_addr_t *)&vmdev->cfg_mem.base,
vmdev->cfg_mem.size, -1, 0, NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm quite puzzled by this. One of the key principles of the OpenAMP library is to support static allocation s wherever possible. If we want to extend the virtio MMIO for AMP systems, we should also support "safe" allocation.

I can see two strategies:

  • Implement dynamic allocation with metal_allocate_memory() to have a robust memory allocator.
  • Declare allocations statically where possible.

The OpenAMP library uses static allocation for the virtio and rpmsg layers. For the virtio transport layer (remoteproc virtio), we have dynamic allocation.

I would be in favor here of implementing static allocation as proposed by @danmilea in danmilea/open-amp@ba16118.

I notice that there is another allocation in the virtio_mmio_register_device() function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, seems reasonable, adding a follow up patch to switch to static allocations here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And for the more general issue of dynamic allocations, I've been looking at the same as it would be a blocker for "safety".

I'd like to drop out those last couple dynamic allocations (move them to the application side, not hide them in the library as done today) but it would take a slight modification to the API. Specifically the allocations in rproc_virtio_create_vdev(). So to do any work on that we would want to make it an internal-only API which it should have been from the start.

Unfortunately as we don't separate internal from external APIs yet, someone used it in a Zephyr project sample. I've got a patch to fix that[0]. After that is in, we should mark that API internal-only so we can clean it up without breaking anyone.

glneo/zephyr@4acc5fc

Copy link
Collaborator

Choose a reason for hiding this comment

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

And for the more general issue of dynamic allocations, I've been looking at the same as it would be a blocker for "safety".

I'd like to drop out those last couple dynamic allocations (move them to the application side, not hide them in the library as done today) but it would take a slight modification to the API. Specifically the allocations in rproc_virtio_create_vdev(). So to do any work on that we would want to make it an internal-only API which it should have been from the start.

Unfortunately as we don't separate internal from external APIs yet, someone used it in a Zephyr project sample. I've got a patch to fix that[0]. After that is in, we should mark that API internal-only so we can clean it up without breaking anyone.

glneo/zephyr@4acc5fc

rproc_virtio_create_vdev() has been used to avoid to embed the remoteproc part of the library.
The use of the remoteproc add around 2kB of code and 200 bytes of data.

Another objective was to separate the remotproc layers from the remoteproc_virtio layer.

That said yes this part need to be reworked to support the resource table in safety context with static allocation.
What about a new API that would replace this one?


return 0;
}
Expand Down
Loading