Backport "fix: typo in Getting started guide" #5409
Closed
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
Currently, the device ids (TYPE_*) are hardcoded in various places. With this change, they are generated from linux headers. (cherry picked from commit ad58102) Signed-off-by: Riccardo Mancini <[email protected]> Signed-off-by: Babis Chalios <[email protected]>
Make the with_virtio_device_with_id function more generic and introduce its variant with error handling try_with_virtio_device_with_id. Then use these new functions in vmm.rs whenever we need to perform an action on a device. To simplify the code, I also moved some balloon device error handling back to the balloon device code and refactored it a little bit. (cherry picked from commit c6987de) Signed-off-by: Riccardo Mancini <[email protected]> Signed-off-by: Babis Chalios <[email protected]>
Remove some error variants that were not used in the code. (cherry picked from commit 3b10bf2) Signed-off-by: Riccardo Mancini <[email protected]> Signed-off-by: Babis Chalios <[email protected]>
The balloon device always returns "Amount of pages requested cannot fit in u32" even if it fails due to the guest memory check. Reword the error to make it more clear. (cherry picked from commit f36767e) Signed-off-by: Riccardo Mancini <[email protected]> Signed-off-by: Babis Chalios <[email protected]>
As the caller of latest_stats() always clones the object, just derive Copy on it and return the copy rather than a reference. (cherry picked from commit 26fd798) Signed-off-by: Riccardo Mancini <[email protected]> Signed-off-by: Babis Chalios <[email protected]>
By defining an associated function to the trait, we can simplify the logic to execute code on a specific device from the VMM, while also statically guaranteeing we are passing the right constant. The downside is that we need both a sized and a dyn implementation for the function. To ensure devices implement them correctly, a utility macro is provided. We cannot do it as a default trait impl as the const_ variant is only defined on Sized. (cherry picked from commit acb79a2) Signed-off-by: Riccardo Mancini <[email protected]> Signed-off-by: Babis Chalios <[email protected]>
Add unit tests that ensure our logic for handling the interaction with guest VirtIO drivers over the PCI transport is correct. Also, drop some dead code and fix some of the handling of various fields. A list of logic fixes we found while writing the tests: * We found some effectively dead code. As VirtIO stipulates that accesses to 64bit configuration fields MUST happen with two individual 32bit reads, so drop the code that was actually handling the 64bit accesses. * There were fields that the code was handling as write-only, when the specification declares them read-write. This was not an issue, since the Linux driver apparently never reads them, but add the read handling, so that we are spec compliant. * The specification mentions that the driver MUST NOT write a value of 0 in the queue_enable field. Enforce it. (cherry picked from commit c91b8a4) Signed-off-by: Babis Chalios <[email protected]>
Remove dead code from the logic that handles setting up a PCI VirtIO device transport. This was either code that we pulled from Cloud Hypervisor and we don't need here or code that we are not currently using. (cherry picked from commit 6323d07) Signed-off-by: Babis Chalios <[email protected]>
The VirtioPciCap type had the wrong structure. This wasn't a problem, because the wrong field was actually operating as part of the padding of which it was taking away the space. Also, fix the initialization of the VirtioPciCfgCap so that it reports the correct length for the capability. (cherry picked from commit 8ee3af1) Signed-off-by: Babis Chalios <[email protected]>
Add more unit tests in for VirtIO PCI capabilities other than the common configuration capability. (cherry picked from commit 2540713) Signed-off-by: Babis Chalios <[email protected]>
The PCI configuration capability gives a way to the guest to access BAR memory for a device without actually mapping the BAR in guest memory. The way this works is that the guest accesses the capability directly (in PCI configuration space) and it programs reads/writes from/to BAR memory. When such an access is detected we redirect certain PCI configuration space access to VirtIO BAR accesses. These accesses happen in chunks of 4 bytes, however the code that handles BAR accesses expects the length of an access to match exactly the length of the field we are accessing, e.g. accessing a u8 field needs a &[u8] slice with length of 1. The emulation logic was not taking that into account so the mechanism wasn't effectively working. Fix that and also get rid off an unsafe `transmute` in favour of an Le32::into<u32>(). Finally, add unit tests that ensure everything works as expected. (cherry picked from commit 54eb06d) Signed-off-by: Babis Chalios <[email protected]>
We always enable MSI-X interrupts for VirtIO devices when using the PCI transport. Currently, we don't offer a fallback if the driver doesn't want to use MSI-X, but VirtIO drivers we are working with always support that. So this should not be a problem for our use-cases in the kernels we support at the moment. On the other hand, despite not supporting legacy interrupts, we still did some maintenance for the ISR status byte which is not used by MSI-X interrupts at all. Drop all this handling, ensure that reads always return 0 and writes have no effect in the state of the device. Also, add warning messages for the cases where the guest tries to access the ISR parts of the BAR. (cherry picked from commit 1dee309) Signed-off-by: Babis Chalios <[email protected]>
Add a unit test that ensures that the Notification VirtIO capability is specification compliant. (cherry picked from commit 0ca3666) Signed-off-by: Babis Chalios <[email protected]>
Add a unit test that ensures the device initialization process works properly. (cherry picked from commit 031fb88) Signed-off-by: Babis Chalios <[email protected]>
PCI configuration capability allows a driver to access a BAR without mapping it in virtual address space. The driver issues reads/writes directly within the PCI configuration space (which should always be addressable either via MMIO or Port IO) which the device translates corresponding BAR accesses. The way this works is that the guests writes the offset and length of a BAR access within the capability structure and then reads/writes data using a 4-bytes dedicated array that also lives in the capability address space. We had a bug in the logic that handles writes where a guest would program a write of a certain length (L) and then try to perform a write using a buffer where buffer.len() < L. Our logic would then try to perform a write using the slice buffer[..L] which would cause Rust to panic with an out of range exception. Fix this by taking into account the buffer's length and using a slice with length min(L, buffer.len()). (cherry picked from commit af1d121) Signed-off-by: Babis Chalios <[email protected]>
Add information in our Documentation regarding how users can enable PCI support for Firecracker microVMs and mention requirements for building the guest kernel, as well as the requirements for kernel command line parameters. Also, add an entry in the CHANGELOG mentioning the addition of PCI support. (cherry picked from commit 431d77e) Signed-off-by: Babis Chalios <[email protected]>
We have a single lock for all devices on the PCI bus that serializes reads and writes on the devices' PCI configuration space and BARs. This should not be a problem at the moment. It should be out of any hot path and only up until we are setting up devices. However, add a comment that mentions the existence of the contention so that we keep it in mind for the futuer (and maybe perform some profiling). (cherry picked from commit 6169ec3) Signed-off-by: Babis Chalios <[email protected]>
bchalios
requested review from
Manciukic,
kalyazin,
pb8o and
xmarcalx
as code owners
|
Wrong branch to open the PR from. Need more coffee :/ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Changes
Backporting fix from #5408
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.