-
Notifications
You must be signed in to change notification settings - Fork 273
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.
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: add alloc_buf/free_buf in virtio ops #541
base: main
Are you sure you want to change the base?
Conversation
ecfdbd4
to
3c591de
Compare
Buffer management is different for different transport layer: For MMIO transport layer, the buffer can direclty malloced from the gust os heap beacase the hypervisor can access all the memmory own by guest os. For remoteproc transpor layer, the buffer should be malloced from the share memory region to make sure the remote core can access this buffer too. So add alloc_buf/free_buf in virtio ops to make different transport can implement their own share memory management: 1. add alloc_buf/free_buf api in virtio ops; 2. support the alloc_buf/free_buf for remoteproc transport layer; Signed-off-by: Bowen Wang <wangbowen6@xiaomi.com>
@@ -39,6 +39,11 @@ extern "C" { | |||
/* define vdev notification function user should implement */ | |||
typedef int (*rpvdev_notify_func)(void *priv, uint32_t id); | |||
|
|||
/* Define remoteproc virtio memory management function */ | |||
struct remoteproc_virtio; | |||
typedef void *(*rpvdev_alloc_buf)(struct remoteproc_virtio *rpvdev, size_t size, size_t align); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we replace struct remoteproc_virtio *rpvdev
with void *priv
like other callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some cosmetics questions plus minor comment for this first round.
void *ptr = virtio_alloc_buf(vdev, size, align); | ||
|
||
if (ptr) | ||
memset(ptr, 0, size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IHMO this line adds a unnecessary dependency of the platform/guest/host libc, since most of the host/OS offers calloc besides plain malloc, it would not be better to drop this function and let's the transport implementor to decide if it would return initialized or unitialized buffer?
I don't see right now an use case when we need a calloc/zalloc operation.
Please let me know if I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will remove this function.
static inline void *virtio_alloc_buf(struct virtio_device *vdev, | ||
size_t size, size_t align) | ||
{ | ||
return vdev->func->alloc_buf(vdev, size, align); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check argumentts:
- vdev -> return -EINVAL if NULL;
- alloc_buf -> function may be not supported, check if it exists first and return -ENXIO if NULL
*/ | ||
static inline void virtio_free_buf(struct virtio_device *vdev, void *buf) | ||
{ | ||
vdev->func->free_buf(vdev, buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check argumentts:
- vdev -> return -EINVAL if NULL;
- free_buf -> function may be not supported, check if it exists first and return -ENXIO if NULL
return; | ||
|
||
rpvdev->free_buf(rpvdev, buf); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should call the public functions from virtio top layer interface and let it to redirect to vdev specific alloc/free function instead of invoking the dispatch functions directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your suggestion, I'm a little confused, my motivation is that different platforms may have different memory management strategies for remoteproc transport layer, so I introduce function rproc_virtio_set_mm_callback()
to let the platform set their own share memory allocate and free functions.
Do you mean we should add a memory operations to the virtio device and then we can make different virtio devices implement their own share memory management strategies?
|
||
/** Free buffer to transport layer heap */ | ||
void (*free_buf)(struct virtio_device *vdev, void *buf); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me that it is not the good place to add these functions
The memory allocation/free is independent from the virtio layer as addressed directly to the transport layer.
The virtio_dispatch
is used for communication between the virtio layer and the transport layer
What about a new structure similar to virtio_dispatch
? virtio_mem_ops
or a better name...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems better, I will try this, Thanks.
This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity. |
Buffer managerment is different for different transport layer:
For MMIO transport layer, the buffer can direclty malloced from the gust os heap beacase the hypervisor can access all the memory owned by guest os.
For remoteproc transpor layer, the buffer should be malloced from the share memory region to make sure the remote core can access this buffer too.
So add alloc_buf/free_buf in virtio ops to make different transport can implement their own share memory management: