virtio_mmio: Remove unneeded use of libmetal device #598
Conversation
A virtual metal_device is created, then the needed IO regions are created and added to this device. Immediately we extract these same regions back out and make use of them. There is no reason to do this, instead simply use the created IO regions. This also removes the need to have struct metal_device defined to have more than one IO region (METAL_MAX_DEVICE_REGIONS), which is not the default and can change per-platform. If the libmetal library was built with the default value for METAL_MAX_DEVICE_REGIONS, then this would have led to runtime failures. Signed-off-by: Andrew Davis <[email protected]>
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sure, seems reasonable, adding a follow up patch to switch to static allocations here.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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?
Make the metal_io_region for cfg_io and shm_io into full member structs instead of just pointers. This means they will be allocated along with the parent virtio_mmio_device struct and not need dynamically allocated later. Signed-off-by: Andrew Davis <[email protected]>
|
@danmilea : ok for you with the static allocation? |
A virtual metal_device is created, then the needed IO regions are created and added to this device. Immediately we extract these same regions back out and make use of them. There is no reason to do this, instead simply use the created IO regions.
This also removes the need to have struct metal_device defined to have more than one IO region (METAL_MAX_DEVICE_REGIONS), which is not the default and can change per-platform. If the libmetal library was built with the default value for METAL_MAX_DEVICE_REGIONS, then this would have led to runtime failures.