KVM Dirty log ring interface

[davidkleymann]

Summary of the PR

Add support for dirty page tracking via the Dirty ring interface. Adds KvmDirtyLogRing structure for keeping track of the indices and the base pointer to the shared memory buffer. Implements iterating over dirty pages, thereby harvesting them. Implements reset_dirty_rings on VmFd to trigger recycling of dirty ring buffer elements by the kernel after processing. Adds the dirty_log_ring field to VcpuFd.

This is a draft that needs some review and improvements, I'm hereby asking for suggestions for improving the following remaining weaknesses:

1. If any vCPU is running while harvesting dirty pages, the user of KvmDirtyLogRing needs to call reset_dirty_rings before reading contents of dirty pages. This is currently the users responsibility, which allows for calling reset_dirty_rings after all dirty pages have been read by the VMM for cases where all vCPUs are halted, but this may be a confusing interface for other cases.

More info on the interface:
https://www.kernel.org/doc/html/latest/virt/kvm/api.html#kvm-cap-dirty-log-ring-kvm-cap-dirty-log-ring-acq-rel

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

• All commits in this PR have Signed-Off-By trailers (with
  git commit -s), and the commit message has max 60 characters for the
  summary and max 75 characters for each description line.
• All added/changed functionality has a corresponding unit/integration
  test.
• All added/changed public-facing functionality has entries in the "Upcoming
  Release" section of CHANGELOG.md (if no such section exists, please create one).
• Any newly added unsafe code is properly documented.

[roypat]

please squash your commits so that we do not have later commits be fixups for previous commits.

[roypat]

Also, do we need to do anything about KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP?

[roypat]

Send and Sync are probably not safe on KvmDirtyLogRing because accesses are not stateless, due to the state we need to keep track of in user space (next_dirty)

As for this, next_dirty doesnt imply that it cannot be Send/Sync, because the traits dont mean that multiple threads can access the struct at the same time (thats always unsound in Rust's memory model), just that you can transfer ownership between threads, and that read-only references can be shared between threads. A problem would only arrive if we can also Clone this structure, because then the iterator can cause data races inside the mmap'd area from different Rust threads. But as long as for each vcpu we only allow creating a single instance of the iterator (using safe code that is), there are no problems.

Now, whether Send and Sync will actually be useful is a different matter, because I'm assuming you want them to be able to harvest the dirty ring while the vcpu is still running, but with the current API in this PR, you can only get a &mut reference to the ring buffer structure, which you cannot send to another thread anyway. So in this case, you'd need some API that lets you take ownership of the KvmDirtyRingLog structure (maybe store the one that gets created at vcpu creation time in an option, and then have a function that just wraps .take() on that option, keeping in mind that we must never allow safe code to get two owned versions of it for the same vcpu).

[roypat]

Also sorry for the late response, we were all preparing for / traveling to KVM Forum last week!

[davidkleymann]

Also, do we need to do anything about KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP?

Turns out we do, and if it is supported, we also need to enable KVM_CAP_DIRTY_LOG_RING_ACQ_REL prior to enabling _WITH_BITMAP. Correct me if I'm wrong.

[davidkleymann]

Why is enable_cap not available in aarch64?
#[cfg(any(target_arch = "x86_64", target_arch = "s390x", target_arch = "powerpc"))]

[roypat]

Why is enable_cap not available in aarch64? #[cfg(any(target_arch = "x86_64", target_arch = "s390x", target_arch = "powerpc"))]

according to the docs, it doesn't exist on arm: https://docs.kernel.org/virt/kvm/api.html#kvm-enable-cap . But it seems the docs are wrong there (they're not, there's two ioctls, one for vcpus and one for vms, with the vcpu one being arch-specific, and the VM one documented as "architectures: all"), because there is a generic implementation in the kernel for some capabilities (including the dirty log related ones) which is available on all arches. So kvm-ioctls is doing it wrong (probably whoever wrote it also misread the docs). Can you change Vm::enable_cap to be available always (e.g. without any cfgs) and add a changelog entry under "fixed"?

[davidkleymann]

The enable_cap doctest uses KVM_CAP_SPLIT_IRQCHIP as an example, which is x86 only. This breaks the test and I disabled the enable_cap call in the test on other arches.

[davidkleymann]

Send and Sync are probably not safe on KvmDirtyLogRing because accesses are not stateless, due to the state we need to keep track of in user space (next_dirty)

As for this, next_dirty doesnt imply that it cannot be Send/Sync, because the traits dont mean that multiple threads can access the struct at the same time (thats always unsound in Rust's memory model), just that you can transfer ownership between threads, and that read-only references can be shared between threads. A problem would only arrive if we can also Clone this structure, because then the iterator can cause data races inside the mmap'd area from different Rust threads. But as long as for each vcpu we only allow creating a single instance of the iterator (using safe code that is), there are no problems.

Now, whether Send and Sync will actually be useful is a different matter, because I'm assuming you want them to be able to harvest the dirty ring while the vcpu is still running, but with the current API in this PR, you can only get a &mut reference to the ring buffer structure, which you cannot send to another thread anyway. So in this case, you'd need some API that lets you take ownership of the KvmDirtyRingLog structure (maybe store the one that gets created at vcpu creation time in an option, and then have a function that just wraps .take() on that option, keeping in mind that we must never allow safe code to get two owned versions of it for the same vcpu).

I needed Send and Sync to move the Vcpus to separate threads, but I only implemented what the rust compiler suggested and am still looking for a better solution for this unrelated problem.

[roypat]

Everything should be ready

Please still squash your commits (and there's also conflicts, although GitHub isn't telling me what they are for some reason 🤔)

[davidkleymann]

So you want everything squashed into 1 commit?

[roypat]

So you want everything squashed into 1 commit?

I will tentatively say "yes" (sorry, been a while since I looked at the actual changes here). But please have a look and see if there's anything that can be logically separated into their own commits (e.g. preliminary refactors, or changes to tests).

[davidkleymann]

I did change quite a few things so I already rebased it to seperate the commits logically, but if you require a single large commit I can change it as well. I can't really see the conflicts either, I suspect it is just because the commit introducing the DirtyLogRing cap has already been merged to main, while this PR contains my unmerged one.

[davidkleymann]

@roypat I squashed everything as requested.

[zolutal]

Hi @davidkleymann , thanks for your work on this feature! I was testing it out and noticed that no VcpuExit is currently defined for KVM_EXIT_DIRTY_RING_FULL. Currently you just get a VcpuExit::Unsupported(31) when the ring buffer fills up. If it doesn't make it in this PR I can make one myself to add it, but I figured I'd mention it here.

[roypat]

Hi @davidkleymann , thanks for your work on this feature! I was testing it out and noticed that no VcpuExit is currently defined for KVM_EXIT_DIRTY_RING_FULL. Currently you just get a VcpuExit::Unsupported(31) when the ring buffer fills up. If it doesn't make it in this PR I can make one myself to add it, but I figured I'd mention it here.

that is a good catch, thanks! This should definitely be added in this PR :)

[davidkleymann]

I can't re-request review, but I added the VcpuExit @zolutal @roypat