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: Features change to 64 bit in all virtio_dispatch #616

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wyr-7
Copy link
Contributor

@wyr-7 wyr-7 commented Sep 19, 2024

change all 32 bit features to 64 bit for adapt more features

change all 32 bit features to 64 bit for adapt more features

Signed-off-by: Yongrong Wang <wangyongrong@xiaomi.com>
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.

regarding virtio spec 64 bits could be not enough. If we update this I would be in favour of having a "features word selection index" as done in virtio mmio transport would be more reliable.
This imply to create new APIs and deprecate exiting ones...

For instance

static inline uint32_t
rpmsg_virtio_get_features_by_idx(struct rpmsg_virtio_device *rvdev, unint32 word_idx)

@CV-Bowen
Copy link
Contributor

@arnopo Hi, from the virtio spec v1.3, the 64 bits is enough for now and linux use uint64_t to represent the feature bit too. Does Linux plan to update to "features word selection index" APIs in the future?

@arnopo
Copy link
Collaborator

arnopo commented Sep 26, 2024

I don't know if Linux plans to do it. What I can see is that the virtio spec defines the feature bits in this way:

  • 0 to 23, and 50 to 127
    Feature bits for the specific device type

  • 24 to 40
    Feature bits reserved for extensions to the queue and feature negotiation mechanisms

  • 41 to 49, and 128 and above
    Feature bits reserved for future extensions.

The virtio MMIO transport layer also supports an infinite number of feature bits

So as this PR update the API i would prefer to directly address it instead of implementing an intermediate step that update the API

I don't know if Linux plan to do it, what I can see is that it is that the virtio spec define the feature bits in this way:

  • 0 to 23, and 50 to 127
    Feature bits for the specific device type

  • 24 to 40
    Feature bits reserved for extensions to the queue and feature negotiation mechanisms

  • 41 to 49, and 128 and above
    Feature bits reserved for future extensions.

The virtio MMIO transport layer also supports an infinite number of feature bits.

So, as this PR updates the API, I would prefer to directly address it instead of implementing an intermediate step that also need an update of the API.

@CV-Bowen
Copy link
Contributor

CV-Bowen commented Oct 4, 2024

@arnopo Thanks, as you said, it would be better to use "features word selection index" method to support feature bits larger than 64 bits and we will update this PR.

@arnopo arnopo requested review from edmooring and tnmysh November 5, 2024 10:28
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 Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants