I/O virtual memory (IOMMU) support #327
Conversation
|
I know why the code coverage CI check fails, because I (purposefully) don’t have unit tests for the new code. Why the other tests failed, I don’t know; but I suspect it’s because I force-pushed an update (fixing the CHANGELOG.md link) while the tests where running, maybe SIGTERM-ing them. Looking at the timelines, they all failed (finished) between 16:27:02.985 and 16:27:02.995. (Except the coverage one, which is an actual failure.) |
|
Pushed an update without actual changes (just re-committing the top commit) to trigger a CI re-run. This time, only the coverage check failed (as expected). |
With virtual memory, seemingly consecutive I/O virtual memory regions may actually be fragmented across multiple pages in our userspace mapping. Existing `descriptor_utils::Reader::new()` (and `Writer`) implementations (e.g. in virtiofsd or vm-virtio/virtio-queue) use `GuestMemory::get_slice()` to turn guest memory address ranges into valid slices in our address space; but with this fragmentation, it is easily possible that a range no longer corresponds to a single slice. To fix this, we can instead use `try_access()` to collect all slices, but to do so, its region argument needs to have the correct lifetime so we can collect the slices into a `Vec<_>` outside of the closure. Signed-off-by: Hanna Czenczek <[email protected]>
read() and write() must not ignore the `count` parameter: The mappings passed into the `try_access()` closure are only valid for up to `count` bytes, not more. Signed-off-by: Hanna Czenczek <[email protected]>
When we switch to a (potentially) virtual memory model, we want to compact the interface, especially removing references to memory regions because virtual memory is not just split into regions, but pages first. The one memory-region-referencing part we are going to keep is `try_access()` because that method is nicely structured around the fragmentation we will have to accept when it comes to paged memory. `to_region_addr()` in contrast does not even take a length argument, so for virtual memory, using the returned region and address is unsafe if doing so crosses page boundaries. Therefore, switch `Bytes::load()` and `store()` from using `to_region_addr()` to `try_access()`. Signed-off-by: Hanna Czenczek <[email protected]>
The existing `GuestMemory` trait is insufficient for representing virtual memory, as it does not allow specifying the required access permissions. Its focus on all guest memory implementations consisting of a relatively small number of regions is also unsuited for paged virtual memory with a potentially very lage set of non-continuous mappings. The new `IoMemory` trait in contrast provides only a small number of methods that keep the implementing type’s internal structure more opaque, and every access needs to be accompanied by the required permissions. Signed-off-by: Hanna Czenczek <[email protected]>
Rust only allows us to give one trait the blanket implementations for `Bytes` and `GuestAddressSpace`. We want `IoMemory` to be our primary external interface becaue it has users specify the access permissions they need, and because we can (and do) provide a blanket `IoMemory` implementation for all `GuestMemory` types. Therefore, replace requirements of `GuestMemory` by `IoMemory` instead. Signed-off-by: Hanna Czenczek <[email protected]>
The Iommu trait defines an interface for translating virtual addresses into addresses in an underlying address space. It is supposed to do so by internally keeping an instance of the Iotlb type, updating it with mappings whenever necessary (e.g. when actively invalidated or when there’s an access failure) from some internal data source (e.g. for a vhost-user IOMMU, the data comes from the vhost-user front-end by requesting an update). In a later commit, we are going to provide an implementation of `IoMemory` that can use an `Iommu` to provide an I/O virtual address space. Note that while I/O virtual memory in practice will be organized in pages, the vhost-user specification makes no mention of a specific page size or how to obtain it. Therefore, we cannot really assume any page size and have to use plain ranges with byte granularity as mappings instead. Signed-off-by: Hanna Czenczek <[email protected]>
XanClic
force-pushed
the
iommu
branch
2 times, most recently
from
5ee020b to
4104d5f
Compare
XanClic
changed the title
XanClic
requested review from
alexandruag,
bonzini,
jiangliu,
roypat and
ShadowCurse
as code owners
|
Added tests for the new functionality, and rebased on the main branch. |
|
cc @germag |
This `IoMemory` type provides an I/O virtual address space by adding an IOMMU translation layer to an underlying `GuestMemory` object. Signed-off-by: Hanna Czenczek <[email protected]>
The vhost-user-backend crate will need to be able to modify all existing memory regions to use the VMM user address instead of the guest physical address once the IOMMU feature is switched on, and vice versa. To do so, it needs to be able to modify regions’ base address. Because `GuestMemoryMmap` stores regions wrapped in an `Arc<_>`, we cannot mutate them after they have been put into the `GuestMemoryMmap` object; and `MmapRegion` itself is by its nature not clonable. So to modify the regions’ base addresses, we need some way to create a new `GuestRegionMmap` referencing the same `MmapRegion` as another one, but with a different base address. We can do that by having `GuestRegionMmap` wrap its `MmapRegion` in an `Arc`, and adding a method to return a reference to that `Arc`, and a method to construct a `GuestRegionMmap` object from such a cloned `Arc.` Signed-off-by: Hanna Czenczek <[email protected]>
Without an IOMMU, we have direct access to guest physical addresses (GPAs). In order to track our writes to guest memory (during migration), we log them into dirty bitmaps, and a page's bit index is its GPA divided by the page size. With an IOMMU, however, we no longer know the GPA, instead we operate on I/O virtual addresses (IOVAs) and VMM user-space addresses (VUAs). Here, the dirty bitmap bit index is the IOVA divided by the page size. `IoMemory` types contain an internal "physical" memory type that operates on these VUAs (`IoMemory::PhysicalMemory). Any bitmap functionality that this internal type may already have (e.g. `GuestMemoryMmap` does) cannot be used for dirty bitmap tracking with an IOMMU because they would use the VUA, but we need to use the IOVA, and this information is not available on that lower layer. Therefore, `IoMemory` itself needs to support bitmaps separately from its inner `PhysicalMemory`, which will be used when the IOMMU is in use. Add an associated `IoMemory::Bitmap` type and add a bitmap object to `IommuMemory`. Ensure that writes to memory dirty that bitmap appropriately: - In `try_access()`, if write access was requested, dirty the handled region of the bitmap after the access is done. - In `get_slice()`, replace the `VolatileSlice`'s bitmap (which comes from the inner `PhysicalMemory`) by the correct slice of our IOVA bitmap before returning it. Signed-off-by: Hanna Czenczek <[email protected]>
|
Added support for bitmaps in I/O virtual address space (required for migration). |
This commit also adds the iommu feature to the coverage_config feature list. (I left the aarch64 coverage value unchanged; I cannot find out how to get the current value on my system, and it isn’t include in CI.) Signed-off-by: Hanna Czenczek <[email protected]>
Document in DESIGN.md how I/O virtual memory is handled. Signed-off-by: Hanna Czenczek <[email protected]>
Signed-off-by: Hanna Czenczek <[email protected]>
| /// the I/O virtual address space that `IommuMemory` provides into that underlying physical address | ||
| /// space. | ||
| #[derive(Debug, Default)] | ||
| pub struct IommuMemory<M: GuestMemory, I: Iommu> { |
There was a problem hiding this comment.
I am not sure I understand methods such as inner_replaced. Maybe what you want is a GuestAddressSpace here, and physical_memory() returns a GuestAddressSpace::T?
Maybe with that change you don't need the Arc<I> anymore, but you can just use &I?
There was a problem hiding this comment.
As discussed, yep, I’ll try making it a GuestAddressSpace instead to have the user handle the physical memory object management.
(And I’ll see whether I can make it I instead of Arc<I>.)
There was a problem hiding this comment.
Yep, something like IommuMemory<M: GuestAddressSpace, I: Deref<Target = Iommu>> would do. Then the Clone implementation can be conditional on having Clone on both M and I.
The main disadvantage would be an extra clone() in try_access(). But it could be optimized with a method like
fn snapshot(&self) -> IommuMemory<&M::M, &I> {
IommuMemory {
inner: &*self.inner.memory(),
iommu: &*self.iommu,
enabled_iommu: self.enabled_iommu
}
}
There was a problem hiding this comment.
After trying to implement it, I’m not sure how to make it work with the slice lifetimes. Both try_access() and get_slice() return[1] objects with the same lifetime as self, which only works if self owns the memory. With GuestAddressSpace, the lifetime would be limited to the memory() snapshot we can take.
The simplest way to do this would be for the caller to take that snapshot and pass it into get_slice()/try_access(). At that point, I suppose we could completely remove any reference to the GuestMemory from IommuMemory altogether. I wouldn’t like that very much, though.
Besides my personal preferences, I also don’t know how to make it work at least with users like descriptor_utils::Reader (and Writer): That new() function collects slices in memory[2], and returns those in an object whose lifetime is bound to that of its memory parameter. If we used GuestAddressSpace for the physical memory, someone in descriptor_utils would need to create a snapshot, store it somewhere, and the slices’ lifetimes would be bound to that snapshot’s lifetime. I don’t know how to express that, other than to have the Reader::new() caller in turn create that snapshot and pass it into that function. That may indeed be doable, but it doesn’t really feel worth it to me…
So at this point, I’d prefer for IommuMemory to own the inner physical memory, and consider IoMemory a snapshot like GuestMemory is a snapshot.
[1] In case of try_access(), “return” means passing it to the closure.
[2] Note that currently it uses GuestMemory::find_region() + GuestMemoryRegion::get_slice() – that will need to be changed into an IoMemory::try_access(|_, len, region_addr, region| region.get_slice(region_addr, len)), basically.
| @@ -87,6 +93,8 @@ impl std::ops::BitAnd for Permissions { | |||
| pub trait IoMemory { | |||
| /// Underlying `GuestMemory` type. | |||
| type PhysicalMemory: GuestMemory; | |||
| /// Dirty bitmap type for tracking writes to the IOVA address space. | |||
| type Bitmap: Bitmap; | |||
There was a problem hiding this comment.
I am not sure I like the design here. The IOVA address space, being virtual, can have aliases. Could you still store the bitmap in the low-level memory region, but with accesses going through IOMMU translation?
What does vhost-user require? Are dirty bitmap accesses done in IOVA or GPA space?
There was a problem hiding this comment.
Are dirty bitmap accesses done in IOVA or GPA space?
They’re done in IOVA space, which is why I think we need the bitmap on this level.
(If we continued to store it in e.g. GuestRegionMmap/MmapRegion, it would need a reverse translation, and then keep a mapping of address ranges to bitmap slices instead of just a single bitmap slice linearly covering the entire region.)
There was a problem hiding this comment.
They’re done in IOVA space, which is why I think we need the bitmap on this level.
I see. Maybe you could add a bitmap::BS<'a, Self::Bitmap> as another argument that try_access() passes back to the callback? And then IommuMemory::get_slice() can pass that argument to replace_bitmap().
This way, the Iommu controls whether dirty tracking is done in IOVA or GPA space.
There was a problem hiding this comment.
You mean putting the responsibility on the try_access() callback to dirty a bitmap slice given to it if it has written anything?
I’m not sure, that does sound more than reasonable, but would require more changes in the callers than just to add the Permissions flag. 🙂
There was a problem hiding this comment.
You mean putting the responsibility on the try_access() callback to dirty a bitmap slice given to it if it has written anything?
No, I confused try_access and get_slice sorry. For IoMemory, dirtying is already done it in try_access itself; for Iommu the slice could be returned by translate, that is it would be part of the Iotlb entry?
There was a problem hiding this comment.
I’m sorry, I don’t quite follow; currently, dirtying is done only by IommuMemory (in its IoMemory implementation), neither IoMemory nor Iommu.
<IommuMemory as IoMemory>::try_access() dirties the bitmap itself, right.
<IommuMemory as IoMemory>::get_slice() has the VolatileSlice do the work. For this, it replaces the VolatileSlice’s internal bitmap slice (which is initially set by the GuestMemoryRegion) by the right slice in the IOVA space.
I don’t think the IOMMU object should do any of this, and I don’t think it should return slices (if I understand correctly). I think it should only do the translation and not care about the actual memory.
There was a problem hiding this comment.
::get_slice() has the VolatileSlice do the work. For this, it replaces the VolatileSlice’s internal bitmap slice (which is initially set by the GuestMemoryRegion) by the right slice in the IOVA space.
To clarify, I was saying it should not be the IommuMemory that decides the address space used by the bitmap - whether IOVA space as vhost wants or GPA space as everyone else (?) wants. I thought Iommu would return the slice in the Iotlb object, but I agree that probably Iommu isn't the right place either.
That said I don't care much, because as long as IommuMemory is hidden behind a feature, it's just a convenience or a sample implementation (and QEMU probably will not use IommuMemory, only IoMemory). Maybe just add a comment that says "NOTE: IommuMemory can only be used if the dirty bitmap tracks accesses in IOVA space".
There was a problem hiding this comment.
Maybe just add a comment that says "NOTE: IommuMemory can only be used if the dirty bitmap tracks accesses in IOVA space".
To be precise, it depends on the IOMMU enabled setting: With it enabled, it’s IOVA space, otherwise it’s whatever the address space of the underlying GuestMemory is (for vhost, that would be VUA).
I could speculate on how it works for non-vhost (specifically in-hypervisor) cases; maybe the underlying memory will use GPA in that case, and therefore we can (and have to) track dirty accesses in GPA then. To make that work, we would only need a separate flag to control whether the IOVA dirty bitmap is supposed to be used or not.
But I think the best would be for me not to speculate and just leave this as it is – if such a separate flag would be enough, we can just add it later. For the time being, as you suggest, a comment that specifies the dirty bitmap behavior should be enough.
There was a problem hiding this comment.
Overall this is good stuff. It also has a lot of overlap with the experimental changes that were necessary in order to use vm-memory in QEMU, which is a very good thing.
I have two main questions:
- do we really need an
IoMemoryor can we changeGuestMemory? - if we need an
IoMemory, I think thePhysicalMemory: GuestMemorytype instead be anAS: GuestAddressSpace? (and likewise forphysical_memory()?
I'm marking this as "request changes" because there would be changes either way.
|
+1 to changing |
No, I don't think that is a good idea. IOVA and GPA are the same concept, though in different address spaces. My hope when leaving the review was that no changes are needed outside vm-memory, and the interpretation of GuestAddress can be left to whoever creates the GuestMemory. |
The main practical problem with using different types (as stated in the opening comment) is that users generally don’t even know whether a given address is a GPA or an IOVA. For example, the addresses you get from vrings and vrings descriptors can be either, it just depends on whether the IOMMU is enabled or not. Users should be able to use the same code whether it is on or off, which (I think) wouldn’t really be possible with different types.
I hope Paolo will add a comment to that effect (we just had a talk); we agreed that adding I don’t like keeping (Paolo (maybe half-jokingly) suggested making the implementation of that a linking-time error by accessing undefined references in a hypothetical |
Yes, GuestMemory is a nice interface for implementation but clients should switch to |
Different question about this: Do we really need the |
Doesn’t sound bad, but would be a more “radical change”: Intermediate users like virtio-queue currently use memory types via If we change patterns like |
Mh, potentially this wouldn't be that much churn, because we could have Somewhat related question, since I'm seeing the |
In theory yes, in practice it's appearing that there are some design decisions that are made by In general, vm-memory uses traits because VMMs may have their own memory backends. Considering that QEMU is able to use vm-memory designed 5 years ago almost-unmodified, even without IOMMU support (and hopefully, unmodified once there is |
My concern mainly stems from feedback like this, where the existing traits were already a big struggle for people trying to use vm-memory. That said, I'm not fundamentally opposed to adding a
In this case, IMO it'd be less complex to have the compat layer be done through a new type instead of a blanket impl (e.g. add Paolo, do you have any pointers towards code for how QEMU would like to use this, so it's a bit easier for me to wrap my head around everything? |
Trait confusion
In my very humble opinion, especially for the bitmaps, that’s not least because of the trait names. Intuitively, it is absolutely unclear what differentiates In contrast, Blanket impl of
|
|
I see, thanks for taking the time to explain all of this! I was not aware that One thing I just realized is that we can use this entire infrastructure to clean up the Xen stuff, by re-framing it as IoMemory that disallows access to the underlying GuestMemory implementation, which would be most joyous (in this light, maybe we should think of a name other than |
| <M as GuestMemory>::address_in_range(self, addr) | ||
| && <M as GuestMemory>::address_in_range(self, GuestAddress(end)) |
There was a problem hiding this comment.
Mh, what if there's a hole in the physical address range (MMIO gap for example)? I think this might need to be some try_access magic (just .get_slice() won't cut it in case of the consecutive memory being split into different memslots I think?)
There was a problem hiding this comment.
Sure! I think most callers didn’t care so far (which is why GuestMemory only has address_in_range(), callers just used to test the end address), but it would definitely be better to do it right.
Absolutely! |
Yeah, that would be nice... and maybe we can do it?!? Almost all users use For now I'd say the two main contendents are |
The "problem" with this is the lack of function overloading in Rust. It would mean changing the whole API depending on whether you want the IOMMU indirection or not.
For now we have only an RFC implementation here. But you can see that it needs to apply some changes to vm-memory in order to add a direction (read/write) argument to Because QEMU has its backend in the C code, the natural thing for QEMU is to forgo |
mh, you're right, grepping Firecracker and cloud-hypervisor for GuestMemory doesn't show much more than imports and a handful of functions generic over GuestMemory.
Potentially, we could have less churn for downstream if we do the rename straight away. But only if downstream would upgrade to the new interfaces straight away, instead of dealing with the rename by first updating trait bounds to be Maybe we should start having a GitHub milestone for 1.0 tasks.
grr, true. Forgot about that as well.
Thanks, this is really helpful as a reference! |
Summary of the PR
This MR adds support for an IOMMU, and thus I/O virtual memory handling.
New Memory Trait:
IoMemoryHandling I/O virtual memory requires a new interface to access guest memory:
GuestMemorydoes not allow specifying the required access permissions, whichis necessary when working with MMU-guarded memory.
We could add memory access methods with such a permissions parameter to
GuestMemory, but I prefer to provide a completely new trait instead. Thisensures that users will only use the interface that actually works when working
with (potentially) I/O virtual memory, i.e.:
generally assumes that regions are long, continuous, and any address in a
given range will be in the same memory region. This is absolutely no longer
the case with virtual memory, which is heavily fragmented into pages.
That is, adding a new trait (
IoMemory) allows to catch a lot of potentialmistakes at compile time, which I feel is much better than finding out at
runtime that some place forgot to specify the access permissions.
Unfortunately, this is an incompatible change because we need to decide on a
single guest memory trait that we expect users to primarily use: We can only
have one blanket implementation of e.g.
Bytes, and this MR changes thatblanket implementation to be on
IoMemoryinstead ofGuestMemorybecause wewant to prefer
IoMemorywith its permissions-including interface.While this MR does provide a blanket implementation of
IoMemoryfor allGuestMemory, Rust isn’t fully transitive here, so just because we have ablanket
impl IoMemory for GuestMemoryand a blanketimpl Bytes for IoMemorydoesn’t really implicitly give us an
impl Bytes for GuestMemory.What this means can be seen in virtio-queue (in vm-virtio): It uses trait bounds
like
M: GuestMemoryonly, but then expects to be able to use theBytestrait. This is no longer possible, the trait bound must be extended to
M: GuestMemory + Bytesor replaced byM: IoMemory(the latter is what wewant).
Guest Address Type
Another consideration is that I originally planned to introduce new address
types.
GuestAddresscurrently generally refers to a guest physical address(GPA); but we now also need to deal with I/O virtual addresses (IOVAs), and an
IOMMU generally doesn’t translate those into GPAs, but VMM user space addresses
(VUAs) instead, so now there’s three kinds of addresses. Ideally, all of those
should get their own type; but I felt like:
IoMemoryobject is anIOVA or a GPA. It depends on whether the IOMMU is enabled or not, which is
generally a runtime question.
Therefore, I kept
GuestAddressas the only type, and it may refer to any ofthe three kinds of addresses (GPAs, IOVAs, VUAs).
Async Accesses
I was considering whether to also make memory accesses optionally
async. Thevhost-user IOMMU implementation basically needs two vhost-user socket roundtrips
per IOTLB miss, which can make guest memory accesses quite slow. An
asyncimplementation could allow mitigating that.
However, I decided against it (for now), because this would also require
extensive changes in all of our consuming crates to really be useful: Anything
that does a guest memory access should then be
async.I think if we want to add this functionality later, it should be possible in a
compatible manner.
Changes Necessary in Other Crates
vm-virtio
Implementation: https://gitlab.com/hreitz/vm-virtio/-/commits/iommu
As stated above, places that bind
M: GuestMemorybut expect theBytestraitto also be implemented need to be changed to
M: GuestMemory + BytesorM: IoMemory. I opted for the latter approach, and basically replaced allGuestMemoryinstances byIoMemory.(That is what we want because dropping
GuestMemoryin favor ofIoMemoryensures that all vm-virtio crates can work with virtual memory.)
vhost
Implementation: https://gitlab.com/hreitz/vhost/-/commits/iommu
Here, the changes that updating vm-memory necessitates are quite marginal, and
have a similar cause: But instead of requiring the
Bytestrait, it’s theGuestAddressSpacetrait. The resolution is the same: Switch from requiringGuestMemorytoIoMemory.The rest of the commits concerns itself with implementing
VhostUserIommuandallowing users to choose to use
IommuMemory<GuestMemoryMmap, VhostUserIommu>instead of only
GuestMemoryMmap.virtiofsd (as one user)
Implementation: https://gitlab.com/hreitz/virtiofsd-rs/-/commits/iommu
This is an example of an actual user. Updating all crates to IOMMU-supporting
versions actually does not require any changes to the code, but enabling the
'iommu' feature does: This feature makes the vhost-user-backend crate require
the
VhostUserBackend::Memoryassociated type (because associated type defaultsare not stable yet), so this single line of code must be added (which sets the
type to
GuestMemoryMmap<BitmapMmapRegion>).Actually enabling IOMMU support is then a bit more involved, as it requires
switching away from
GuestMemoryMmaptoIommuMemoryagain.However, to me, this shows that end users working with concrete types do not
seem to be affected by the incompatible
IoMemorychange until they want to optin to it. That’s because
GuestMemoryMmapimplements bothGuestMemoryandIoMemory(thanks to the blanket impl), so can transparently be used whereverthe updated crates expect to see an
IoMemorytype.Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafecode is properly documented.