Refactor VRING macros for AMP VIRTIO #505
Conversation
lib/include/openamp/virtio.h
Outdated
| @@ -152,7 +152,7 @@ struct virtio_device_id { | |||
| sizeof(struct vring_used_elem) * n + sizeof(uint16_t)) | |||
|
|
|||
| #define VRING_DECLARE(name, n, align) \ | |||
| static char __vrbuf_##name[VIRTIO_RING_SIZE(n, align)] __aligned(VRING_ALIGNMENT); \ | |||
| static char __vrbuf_##name[VIRTIO_RING_SIZE(n, align)] __aligned(VIRTIO_AMP_VRING_ALIGNMENT); \ | |||
There was a problem hiding this comment.
If i well understood here you expect to reserve vrings memory with a VIRTIO_AMP_VRING_ALIGNMENT Alignement. to guaranty that a 4K page is reserved for one vring.
What about using a parammeter instead of hardcoding it?
And how do you ensure that the host is aligned with the _vrbuf##name address?
As there is not usage yet of this macro, I would be in favor of removing it and reintroduce it when used.
else we should maintain it evenif not usable in a generic way.
There was a problem hiding this comment.
I have removed the define in the newer version of the PR.
lib/virtio_mmio/virtio_mmio_drv.c
Outdated
| @@ -286,7 +286,7 @@ struct virtqueue *virtio_mmio_setup_virtqueue(struct virtio_device *vdev, | |||
|
|
|||
| vring_info->io = vmdev->shm_io; | |||
| vring_info->info.num_descs = virtio_mmio_get_max_elem(vdev, idx); | |||
| vring_info->info.align = VRING_ALIGNMENT; | |||
| vring_info->info.align = VIRTIO_AMP_VRING_ALIGNMENT; | |||
There was a problem hiding this comment.
The align is used by vq_ring_init. That's means that you align the vring_used struct on a new 4k page right?
That would mean that the vring is on two 4k page...
There was a problem hiding this comment.
That's what the math says, but it's not different from using the virtqeue/vring creation API with an explicit alignment of 4096. This is also the value for vring alignment defined in several resource tables (rsc_table.c) for zynqmp_r5, microblaze_generic, zynqmp_r5 or the linux generic machine.
There was a problem hiding this comment.
That's what the math says, but it's not different from using the virtqeue/vring creation API with an explicit alignment of 4096. This is also the value for vring alignment defined in several resource tables (rsc_table.c) for zynqmp_r5, microblaze_generic, zynqmp_r5 or the linux generic machine.
As you mention it is on some resource table but unfortunatly not all. For instance the generic implementation of the resource table on zephyr: https://elixir.bootlin.com/zephyr/v3.5.0-rc1/source/lib/open-amp/resource_table.h#L29.
So that will be need to be adapted to be generic, and could impact virtio_mmio_setup_virtqueue() parameters...
danmilea
force-pushed
the
vring_refactor
branch
from
b68885b to
1c57c00
Compare
lib/virtio_mmio/virtio_mmio_drv.c
Outdated
| @@ -286,7 +286,7 @@ struct virtqueue *virtio_mmio_setup_virtqueue(struct virtio_device *vdev, | |||
|
|
|||
| vring_info->io = vmdev->shm_io; | |||
| vring_info->info.num_descs = virtio_mmio_get_max_elem(vdev, idx); | |||
| vring_info->info.align = VRING_ALIGNMENT; | |||
| vring_info->info.align = VIRTIO_AMP_VRING_ALIGNMENT; | |||
There was a problem hiding this comment.
That's what the math says, but it's not different from using the virtqeue/vring creation API with an explicit alignment of 4096. This is also the value for vring alignment defined in several resource tables (rsc_table.c) for zynqmp_r5, microblaze_generic, zynqmp_r5 or the linux generic machine.
As you mention it is on some resource table but unfortunatly not all. For instance the generic implementation of the resource table on zephyr: https://elixir.bootlin.com/zephyr/v3.5.0-rc1/source/lib/open-amp/resource_table.h#L29.
So that will be need to be adapted to be generic, and could impact virtio_mmio_setup_virtqueue() parameters...
lib/include/openamp/virtio.h
Outdated
| .used = (void *)((unsigned long)__vrbuf_##name + ((n * sizeof(struct vring_desc) + \ | ||
| (n + 1) * sizeof(uint16_t) + align - 1) & ~(align - 1))), \ | ||
| } | ||
| #define VIRTIO_AMP_VRING_ALIGNMENT 4096 |
There was a problem hiding this comment.
VIRTIO_MMIO_VRING_ALIGNMENT seems to be more appropriate since it is currently only used for the virtio mmio.
Renamed AMP VIRTIO macros to avoid conflicts. Signed-off-by: Dan Milea <[email protected]>
danmilea
force-pushed
the
vring_refactor
branch
from
1c57c00 to
45d9ad5
Compare
Renamed AMP VIRTIO macros to avoid conflicts.