Adding virtio support for arm #223
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramyak-mehra
requested review from
alexandruag,
andreeaflorescu,
gsserge and
lauralt
as code owners
ramyak-mehra
force-pushed
the
virtio-aarch64
branch
from
ddbd011 to
5eb8e12
Compare
Member
andreeaflorescu
left a comment
andreeaflorescu
left a comment
There was a problem hiding this comment.
Can you add more details in the commit message? I am expecting to see stuff like:
- a description of what is needed for adding these devices on arm (i.e. the fdt)
- what other changes were required for this to be possible (i.e. the updates on the Mock interface, why were they needed? because otherwise you cannot register_irqfd)
- any changes that you did as part of this commit that are not directly related to the support (i.e. the
mmio_from_rangereceives a reference now).
This subtleties are not evident from reading the code and there is no other way of understanding why this commit introduces the changes that it does.
gsserge
reviewed
Jul 13, 2022
src/arch/src/lib.rs
Outdated
| num_vcpus: Option<u32>, | ||
| serial_console: Option<(u64, u64)>, | ||
| rtc: Option<(u64, u64)>, | ||
| // It is in the order(addr , size , irq) |
There was a problem hiding this comment.
I think that adding this comment kinda suggests that there should be an internal utility struct named DevConfig or something like that, with the fields addr, size and irq. It gets confusing to keep track of things when tuples have more than two values.
ramyak-mehra
force-pushed
the
virtio-aarch64
branch
2 times, most recently
from
3801de7 to
866453f
Compare
ramyak-mehra
changed the title
src/arch/src/lib.rs
Outdated
| assert_eq!(fdt.virtio_devices.len(), 1); | ||
| } | ||
| #[test] | ||
| fn test_virtio_device_len() { |
Member
There was a problem hiding this comment.
This test and the one above seem to be doing the same thing. Can you delete one of them?
gsserge
previously approved these changes
Jul 14, 2022
for adding support of virtio devices on arm fdt needs to be updatedd to handle multiple virtio devices arm specific changes to EnvMock which is used to test virtio devices. Incase of arm we first needed to add a vcpu and register a GIC othervise register_irqfd call would fail. `mmio_from_range` function signature is also changed it now expects reference to `RangeInclusive` instead of taking its ownership, because we need it for adding virtio device to fdt. Closes rust-vmm#210 Signed-off-by: Ramyak mehra <rmehra_be19@thapar.edu>
ramyak-mehra
force-pushed
the
virtio-aarch64
branch
from
866453f to
c2ed329
Compare
andreeaflorescu
approved these changes
Jul 14, 2022
gsserge
approved these changes
Jul 14, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #210
Signed-off-by: Ramyak mehra rmehra_be19@thapar.edu
Summary of the PR
Please summarize here why the changes in this PR are needed.
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commitmessage has max 60 characters for the summary and max 75 characters for each
description line.
test.
unsafecode is properly documented.