It takes a pointer to a userspace address that it accesses, so it should
be marked unsafe. This was missed earlier.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
This API passes a u64 to a kernel API that treats the u64 as a userspace
address. Therefore, it should be marked unsafe, but it currently is not
[1]. Wrap the call in an unsafe block to document that invariants must
be upheld to avoid undefined behavior. This causes a compiler warning,
so suppress the warning with #[allow(unused_unsafe)].
[1]: https://github.com/rust-vmm/vfio/issues/100
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
Also drop support for building the TDX code for 32-bit targets. All
CPUs with TDX support are 64-bit so supporting 32-bit targets is not
needed.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
To ensure that struct sizes are the same on 32-bit and 64-bit, various
kernel APIs use __u64 (Rust u64) to represent userspace pointers.
Userspace is expected to cast pointers to __u64 before passing them to
the kernel, and cast kernel-provided __u64 to a pointer before using
them. However, various safe APIs in Cloud Hypervisor took
caller-provided u64 values and passed them to syscalls that interpret
them as userspace addresses. Therefore, passing bad u64 values would
cause memory disclosure or corruption.
Fix the bug by using usize and pointer types as appropriate. To make
soundness of the code easier to reason about, the PCI code gains a new
MmapRegion abstraction that ensures the validity of pointers. The rest
of the code already has an MmapRegion abstraction it can use. To avoid
having to reason about whether something is keeping the MmapRegion
alive, reference counting is added. MmapRegion cannot hold references
to other objects, so the reference counting cannot introduce cycles.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
To ensure that struct sizes are the same on 32-bit and 64-bit, various
kernel APIs use __u64 (Rust u64) to represent userspace pointers.
Userspace is expected to cast pointers to __u64 before passing them to
the kernel, and cast kernel-provided __u64 to a pointer before using
them. However, various safe APIs in Cloud Hypervisor took
caller-provided u64 values and passed them to syscalls that treat them
as userspace addresses. Therefore, passing bad u64 values would cause
memory disclosure or corruption. The memory region APIs are one example
of this, so mark them as unsafe.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
as of rust 1.90, writes to unix sockets use the sendto syscall. This
affects the vcpu threads when vfio_user devices are accessed.
Signed-off-by: Stefan Nürnberger <stefan.nuernberger@cyberus-technology.de>
This better aligns with the rest of the code and makes it clearer
that these tests can run "as is" in a normal hosted environments
without the special test environment.
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
... by just passing the iterator along. For large VMs this bitmap is
gigantic. A 12TB VM has 384MB of dirty bitmap.
With all these optimizations from the previous commits in place, we
see quite the improvement when it comes to scanning the dirty bitmap.
For a bitmap with 1% bits (randomly) set, dirty_log() takes:
Original code: 2166ms (100.0%)
New code: 382ms ( 17.6%)
on my system. The sparser the dirty bitmap the faster. Scanning an
empty bitmap is 100x faster. For a 5% populated bitmap we are still 3x
faster.
If someone wants to play with this, there is a benchmark harness here:
https://github.com/blitz/chv-bitmap-bench
On-behalf-of: SAP julian.stecklina@sap.com
Signed-off-by: Julian Stecklina <julian.stecklina@cyberus-technology.de>
Resignal every 10ms the thread if it has not acknowledged the signal via
setting the atomic when the vCPU thread was acknowledged. Further, avoid
an infinite loop by generating an error if it takes more than 1000ms to
interrupt the thread.
The retry helps mitigate a race condition where the signal is received
between checking the pause atomic and entering KVM_RUN ioctl when
pausing. Hitting this race condition would leave the
wait_untial_signal_acknowledged() method spinning indefinitely.
The timeout error avoids the VMM process being blocked indefinitely.
See: #7427
Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
Consuming `&Arc<T>` as argument is almost always an antipattern as it
hides whether the callee is going to take over (shared) ownership
(by .clone()) or not. Instead, it is better to consume `&dyn T` or
`Arc<dyn T>` to be more explicit. This commit cleans up the code.
The change is very mechanic and was very easy to implement across the
code base.
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
Move the GICv2M MSI frame SPI base from 32 to 128 to avoid conflicts
with platform device interrupts.
While at it, rename the constants to make it explicit that they are
associated with the GICv2m MSI Frame.
Signed-off-by: Anirudh Rayabharam <anrayabh@microsoft.com>
This was added in 7be69edf51 to deal with
changes to the KVM bindings that made run() and set_immediate_exit()
take &mut self. Instead adopt a Box<> value in Vcpu allowing the removal
of this internal Mutex.
Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
Fix a race condition that happens in resume()-pause() cycles.
It is odd that for pause(), the CpuManager waited via `state.paused`
for the vCPU thread to ACK the state change but not for `resume()`.
In the `resume()` case, oddly CpuManager "owned" the state change
in `state.paused`. This commit changes this so that the vCPU ACKs
its state change itself in `state.paused` when it transitions from
pause->run. Further, `CpuManager::resume()` now gracefully waits for
the vCPU to be resumed.
More technical:
This change ensures proper synchronization and prevents situations
in that park() follows right after unpark(), causing deadlocks and
other weird behavior due to race conditions.
Calling resume() now takes slightly longer, very similar to pause().
This is, however, even for 254 vCPUs in the range of less than 10ms,
and ultimately we now have correct behaviour.
## Reproducer
Since [0] is merged, the underlying problem can be tested without this
commit by modifying the pause() API call to run `CpuManager::pause()`
and `CpuManager::resume()` in a loop a thousand times.
`ch-remote --api-socket ... pause`
```patch
diff --git a/vmm/src/vm.rs b/vmm/src/vm.rs
index d7bba25cc..35557d58f 100644
--- a/vmm/src/vm.rs
+++ b/vmm/src/vm.rs
@@ -2687,6 +2687,10 @@ impl Pausable for Vm {
MigratableError::Pause(anyhow!("Error activating pending virtio devices: {:?}", e))
})?;
+ for _ in 0..1000 {
+ self.cpu_manager.lock().unwrap().pause()?;
+ self.cpu_manager.lock().unwrap().resume()?;
+ }
self.cpu_manager.lock().unwrap().pause()?;
self.device_manager.lock().unwrap().pause()?;
```
## Outlook
Decades of experience in VMM development showed us that using
many AtomicBools is a footgun. They are not synchronized with each
other at all. On the long term, we might want to refactor things to
have a single shared AtomicU64 with different bits having different
meanings.
[0] https://github.com/cloud-hypervisor/cloud-hypervisor/pull/7290
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
These bindings actually refer to atomic bool shared across all
vCPUs to instruct single vCPUs with their next action. As there
are already enough Arc<AtomicBool>, this helps while debugging
things to see that different bindings refer to the same atomic
bool.
In other words: This naming really confused us while debugging.
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
Add necessary definitions and RISC-V ACPI tables to enable ACPI feature.
More specifically, this commit add MADT definitions for RISC-V AIA
interrupt chips.
Signed-off-by: Can Zhang <icloud9957@gmail.com>
This provides clearer life-cycle management of resources around vfio,
and aligns better with the kernel behavior as reported below with vfio
legacy mode (with vfio container/group).
Fixes: #7328
Signed-off-by: Bo Chen <bchen@crusoe.ai>
Signed-off-by: Yi Wang <foxywang@tencent.com>
While an IGVM is provided validation fails as there is
no kernel or firmware. This patch fixes the sev_snp boot
failure.
Signed-off-by: Muminul Islam <muislam@microsoft.com>
MSHV (Microsoft Hypervisor) doesn't expose an ITS to guests. Instead it
exposes a GICv2m MSI Frame for mapping MSIs to SPIs.
Update the MADT generation code to create the GIC MSI Frame structure
instead of the ITS structure whenever the guest vGIC is using a GICv2m
frame. This allows booting guests via firmware on MSHV.
Signed-off-by: Anirudh Rayabharam <anrayabh@microsoft.com>
Rust's aliasing rules do not permit mutating a `usize` through a shared
reference. Unsafe blocks and FFI are no exception.
This PR fixes such a violation detected in the AMX support checks by
passing a raw mutable pointer instead of a shared reference in the
syscall.
Signed-off-by: Oliver Anderson <oliver.anderson@cyberus-technology.de>
On-behalf-of: SAP <oliver.anderson@sap.com>
For a graceful resource management of externally provided FDs in Cloud
Hypervisor, corresponding FDs need to be closed on a device removal.
This is the case for virtio-net devices using external FDs, for example.
With the fix introduced in this commit, we allow management software to
properly clean up resources, e.g., libvirt can clean up tap devices.
PS: CHV uses "added" and "removed", which has the same meaning as
hot device attach/hotplug and hot device detach/unplug.
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
Fix clippy warning `uninlined_format_args` reported by rustc rustc
1.89.0 (29483883e 2025-08-04).
```console
warning: variables can be used directly in the `format!` string
--> block/src/lib.rs:649:17
|
649 | info!("{} failed to create io_uring instance: {}", error_msg, e);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args
= note: `#[warn(clippy::uninlined_format_args)]` on by default
help: change this to
|
649 - info!("{} failed to create io_uring instance: {}", error_msg, e);
649 + info!("{error_msg} failed to create io_uring instance: {e}");
|
```
Signed-off-by: Ruoqing He <heruoqing@iscas.ac.cn>
## TL;DR
This unifies error handling, implementation, and logging of config
objects that are populated with additional FDs received as part of
the HTTP API request from the management software. This prevents current
and future code in repetition.
In the future, if we want to support let's say virtio-blk with external
FDs, the AddBlk API handler simply needs a call to `attach_fds_to_cfg()`
and gets all the magic for free.
## Motivation
The motivation of the new abstraction is to provide a verbose, solid,
and bulletproof solution for a complex domain. The interaction between
- the management layer,
- the passing of file descriptors over UNIX domain sockets via
SCM_RIGHTS,
- the attachment of configurations to those FDs,
- and the ability to give new developers clear insights into what
happens under the hood
is not trivial. These factors justify encapsulating the complexity
behind a convenient and well-documented abstraction, making the system
both robust in production and approachable for new developers.
In addition, it allows us to perform unit testing. Further, We get rid
of existing partial code duplication and inconsistencies.
Finally, while this approach may initially result in more code, every
new handler that accepts FDs benefits from reduced duplication and a
correct implementation by relying on the shared abstraction. This will
also enable future functionality, such as virtio-blk devices backed by
FDs which can then be integrated with ease.
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
Deserializing values as `-1` makes sense to prevent errors, so let's
keep it. However, serializing them differently adds confusion. For
example, a `ch-remote info` call should not report `-1` but the actual
FDs.
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
(1) The old messages are missing the "why" part. With this change, users
of Cloud Hypervisor have somehow more context and people looking at
the code perfectly know what's going on.
(2) Using warn! implies that the user should take action, but in this
case, there’s nothing the user can do. If the API is used correctly
and file descriptors are passed via an SCM_RIGHTS message over a
UNIX domain socket, everything works as intended. In that case,
there's no need to issue a warning — debug! is sufficient.
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
Live migration, state save/resume, and hotplug are not trivial when it
comes to virtio-net devices backed by externally provided FDs. As the
mechanism behind it can be considered as quite "multi-step magic" even
for experienced programmers, it makes sense to thoroughly document this
to ease debugging and to improve the mental model of developers working
on this in the future.
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
This change enables easier integration with third-party
tools by removing the requirement for a dummy IP address
when configuring tap devices. The modification applies to
both CLI and API interactions.
Previously, cloud-hypervisor would automatically set a
default static IP address (192.168.249.1) if none was provided.
This could lead to:
* multiple devices without explicit IP configurations
would end up with the same default IP
* unnecessary inclusion of this IP in firewall rules
* the IP address could clash with host networking and
routing
This introduces a new constraint:
When providing an IP, the mask must also be provided.
Removes warnings introduced in #7179.
Closes issue #7083.
Signed-off-by: Maximilian Güntner <code@mguentner.de>
For MSHV customers don't want to make everything
default during partition creation. For example
nested support, some synthetic features could be
controlled from CLI through platform argument.
Create_vm API getting messy after adding more flags.
This patch introduces common data struct to be passed
from vmm crate to hypervisor crate during partition creation.
Signed-off-by: Muminul Islam <muislam@microsoft.com>
This commit includes all simple clippy fixes excluding the
collapsing of nested ifs using the let-chains feature. This
follows in the next commit.
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
These can differ between platforms, so it's better to use centralized
definitions of them. We can't currently do this for the KVM and VFIO
ioctls, because the corresponding crates don't publicly expose the
ioctl numbers.
Signed-off-by: Alyssa Ross <hi@alyssa.is>
Since the user has to explicitly ask for Landlock to be enabled in
Cloud Hypervisor, it's surprising that, even when they've done that,
Landlock will silently not be enabled if the kernel doesn't support
it.
To prevent accidental absence of a desired security protection, exit
with an error if Landlock, or the one feature of it we use in Cloud
Hypervisor (file access) is not supported.
Signed-off-by: Alyssa Ross <hi@alyssa.is>
