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.

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.h: add memory operation for virtio device #541

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

CV-Bowen
Copy link
Contributor

@CV-Bowen CV-Bowen commented Dec 4, 2023

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 memory ops in virtio device to make different transport/device can
implement their own share memory management.

@CV-Bowen CV-Bowen force-pushed the virtio-buff branch 3 times, most recently from 3c591de to d31e41a Compare December 4, 2023 12:37
Copy link
Collaborator

@uLipe uLipe left a 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.

lib/include/openamp/virtio.h Outdated Show resolved Hide resolved
lib/include/openamp/virtio.h Outdated Show resolved Hide resolved
lib/include/openamp/virtio.h Outdated Show resolved Hide resolved
return;

rpvdev->free_buf(rpvdev, buf);
}
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link

This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity.

@github-actions github-actions bot added the Stale label Jan 30, 2024
@CV-Bowen CV-Bowen force-pushed the virtio-buff branch 2 times, most recently from 58d5fac to f58cce3 Compare September 14, 2024 15:36
@CV-Bowen CV-Bowen changed the title virtio: add alloc_buf/free_buf in virtio ops virtio.h: add memory operation for virtio device Sep 14, 2024
@arnopo arnopo removed the Stale label Sep 17, 2024
@arnopo
Copy link
Collaborator

arnopo commented Sep 17, 2024

Hi @CV-Bowen,

Your proposal seems reasonable.

For your information, in the meantime, there is a working group initiated by ARM around a new Virtio transport called virtio-msg.

This group aims to propose a new Virtio transport that should support virtualization and AMP systems. One objective is also to address memory allocation challenges.

My current concern with your PR is that Integrating your work now would imply that we probably have two APIs to support or deprecate this one in the short/middle term.

We have to discuss with the OpenAMP team to determine the strategy we want.
I will come back to you with a decision.

@CV-Bowen
Copy link
Contributor Author

@arnopo Thank you for your reply and waiting for your decisions.

@arnopo
Copy link
Collaborator

arnopo commented Oct 4, 2024

@arnopo Thank you for your reply and waiting for your decisions.

We discussed the PR in the OpenAMP meeting. The decision is to postpone this PR to the next release to give more time to verify that this approach will also fit the other virtio transports.
It is probably not convenient for you, but we prefer not to introduce a new API before ensuring that it is generic enough.

@xiaoxiang781216
Copy link
Collaborator

@arnopo Thank you for your reply and waiting for your decisions.

We discussed the PR in the OpenAMP meeting. The decision is to postpone this PR to the next release to give more time to verify that this approach will also fit the other virtio transports. It is probably not convenient for you, but we prefer not to introduce a new API before ensuring that it is generic enough.

when will the next release out? it's always better to merge this patch immediately after the release branch? so the community could try this change as soon as possible.

@arnopo
Copy link
Collaborator

arnopo commented Oct 8, 2024

@arnopo Thank you for your reply and waiting for your decisions.

We discussed the PR in the OpenAMP meeting. The decision is to postpone this PR to the next release to give more time to verify that this approach will also fit the other virtio transports. It is probably not convenient for you, but we prefer not to introduce a new API before ensuring that it is generic enough.

when will the next release out? it's always better to merge this patch immediately after the release branch? so the community could try this change as soon as possible.

That seems reasonable to me , need first reviews from @edmooring and/or @tnmysh

@CV-Bowen
Copy link
Contributor Author

@arnopo @tnmysh Could you take a look again? and could this patch be included in the 2024.10 Release Version.

Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

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

LGTM, few minor comments

lib/include/openamp/virtio.h Show resolved Hide resolved
lib/include/openamp/virtio.h Show resolved Hide resolved
lib/include/openamp/virtio.h Outdated Show resolved Hide resolved
lib/include/openamp/virtio.h Outdated Show resolved Hide resolved
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 memory ops in virtio device to make different transport/device can
implement their own share memory management.

Signed-off-by: Bowen Wang <[email protected]>
Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

Looks good to go.

@arnopo
Copy link
Collaborator

arnopo commented Nov 5, 2024

Let's move forward with this new API. One objective for the next release is to ensure that it also addresses the needs of virtio-mmio and virtio-msg.

@arnopo arnopo merged commit 2181888 into OpenAMP:main Nov 5, 2024
5 checks passed
@arnopo arnopo added this to the Release V2025.04 milestone Nov 5, 2024
@arnopo
Copy link
Collaborator

arnopo commented Nov 5, 2024

I have updated the commit message to fix some typos. commit sha1 is now 4ace354

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants