Take the recent thread refactor further. Bundle a device's worker
JoinHandles together with the kill event that stops them into a single
WorkerThreads value, owned by VirtioCommon. Its Drop signals the workers
to exit, unparks any parked for migration, and joins them.
This makes a detached/leaked worker unrepresentable. reset(),
wait_for_epoll_threads() and VhostUserCommon::shutdown() now happen when
dropping the WorkerThreads, and the unpark-before-join teardown now
lives in one place.
Assisted-by: Claude:Opus-4.8
Signed-off-by: Dylan Reid <dgreid@fb.com>
Signed-off-by: Rob Bradford <rbradford@meta.com>
Replace use of spawn_virtio_thread() helper with the new method on
VirtioCommon to handle thread management as well as spawning. As a
result this cleanly handles reset if it fails to spawn the thread.
Assisted-by: Claude:Opus-4.7
Signed-off-by: Rob Bradford <rbradford@meta.com>
Drop the borrowed iovec AsyncIo entry points now that all callers use
owned operations. Rename the transitional owned batch and completion
methods to the final trait names and remove the borrowed submission
helpers from the queue wrappers.
This removes a bunch of known safety foot-guns so future-us don't
accidentally use them.
Assisted-by: Codex:GPT-5.5
Signed-off-by: Dylan Reid <dgreid@fb.com>
Switch virtio-blk request construction and the users to the owned
AsyncIo data path added in the series. Read bounce buffers now return
through AsyncIoCompletion before being copied back to guest memory.
This makes the main virtio async block I/O path use retained request
memory. qcow still has raw-iovec fallback paths at this point; those
are removed in follow-up commits.
Leave the legacy borrowed iovec trait methods in place for a follow-up
cleanup commit to minimize single-commit churn.
Signed-off-by: Dylan Reid <dgreid@fb.com>
I started by looking at all `Option<Vec<T>>` values in config.rs and
vm_config.rs, and replaced them with `Option<Box<[T]>>`. This has the
advantage that one now can see at a glance if this field will ever
resize during operation or not, reducing cognitive load and increasing
maintainability. All fields that need the properties of a Ver or where
this change was not trivial are kept intact.
On-behalf-of: SAP philipp.schuster@sap.com
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
The bounce buffer for an unaligned descriptor was allocated in
execute_async and leaked on error paths, even though, for the sync case
the kernel already had a pointer to the buffer.
Clean this up by moving ownership of the buffer to the AlignedOperation
type. To make it actually safe, stop stashing a guest memory pointer for
the duration of the op. Instead, save the guest address and pass guest
memory back to the complete function.
Signed-off-by: Dylan Reid <dgreid@fb.com>
For non-batch backends execute_async submits the kernel I/O inline
before returning. An early return while processing before inserting in
inflight_requests, meant the request went untracked, the local batch
list was never appended to inflight_requests, even though the request is
pending in the kernel.
To track it, insert into self.inflight_requests as soon as execute_async
returns Ok. The completion path's find_inflight_request now matches the
orphan and the bounce buffer is freed only after the kernel signals it
is done.
Signed-off-by: Dylan Reid <dgreid@fb.com>
A malicious or buggy guest can violate virtio by making the same
descriptor head available twice before the first chain has been placed
on the used ring. The submit path pushed both chains onto the
VecDeque-backed inflight_requests keyed by head_index, and on completion
find_inflight_request() returned the first linear match. That Request's
complete_async() freed its bounce buffer while the other chain's
io_uring op was still targeting it, producing a use-after-free the
kernel could then scribble into.
Signed-off-by: Dylan Reid <dgreid@fb.com>
Signed-off-by: Bo Chen <bchen@crusoe.ai>
When no image_type is specified, sector 0 writes are disabled as a
safety measure for autodetected raw images. Extend this protection
to autodetected fixed VHD images, which carry metadata in the last
sector and are equally susceptible to accidental overwrites of the
first sector when the format is not explicitly acknowledged.
Update the corresponding warning in the virtio block worker to be
format agnostic.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
block::Error implements Display via thiserror, so the user facing
log lines do not need the Debug formatter.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
With virtqueue iterator errors now killing the worker and
spawn_virtio_thread marking NEEDS_RESET centrally, the per
handler needs_reset() gate on process_queue_submit and
process_queue_complete is unreachable.
Drop needs_reset(), the two early returns, the unused
device_status field on BlockEpollHandler and its initializer,
and the device_needs_reset import.
No functional change.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
queue.iter() errors used to be swallowed by
handle_queue_iterator_error(), which marked the device as
NEEDS_RESET and returned Ok so the worker kept running while
disabled. spawn_virtio_thread now does the NEEDS_RESET marking
when the worker exits with an error.
Propagate the iterator error as Error::QueueIterator and escalate
it to EpollHelperError::HandleEvent in
process_queue_submit_and_signal so the worker exits. Per request
errors stay logged. Drop the now unused
handle_queue_iterator_error helper.
No functional change for the guest. NEEDS_RESET is still set and
the config interrupt is still raised on virtqueue corruption.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Worker threads spawned through spawn_virtio_thread previously wrote to
exit_evt on any clean Err return, taking the whole VMM down on a single
failed device worker. A guest induced fault in any virtio device thus
propagated into a host wide failure.
Route the Err return through the shared mark_device_needs_reset helper
instead. The helper sets the DEVICE_NEEDS_RESET bit on device_status
and triggers a config change interrupt, so the device goes idle and
the guest is informed. The thread exits cleanly without killing the
rest of the VMM.
The panic and the seccomp filter apply paths keep writing to exit_evt.
A panicked worker may have left poisoned locks or partially mutated
state, so a hard exit remains the right policy there.
spawn_virtio_thread now takes the device_status and the interrupt
callback. Every native virtio and vhost-user call site is updated to
pass them in.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Both block and net implement the same DEVICE_NEEDS_RESET bookkeeping
when a corrupted virtqueue request is detected. They set the bit,
trigger a config change interrupt and log a warning. Move that logic
into shared device_needs_reset and mark_device_needs_reset helpers in
lib.rs and update both call sites to use them.
No functional change.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
The guest can cause submit and completion failures with malformed chains
or invalid addresses. However, this shouldn't permanently stall the
device and terminate the worker.
Genuine reset-worthy failures set needs_reset and return `Ok` anyways
and will more cleanly reset the worker.
Signed-off-by: Dylan Reid <dgreid@fb.com>
process_queue_submit's drain loop builds a fresh queue.iter() per
iteration, which re-reads the guest avail index on every call and has
no per-call cap (the per-iter gap check in virtio-queue only protects
against avail_idx jumping more than queue_size between two reads).
In theory, a malicous or buggy guest could keep adding descriptors and
cause this loop to overflow the iouring submit queue.
Cap a single drain at queue_size. A spec-compliant driver never
produces more than queue_size outstanding entries simultaneously, so
the cap is invisible to well-behaved guests.
Signed-off-by: Dylan Reid <dgreid@fb.com>
Previously the interrupt was created in VirtioPciDevice, moved via the
Option::take() to the VirtioPciDeviceActivator and then moved to the
VirtioDevice upon activation. On reset it would be moved back ready for
reactivation.
Since this already an Arc type remove the wrapping Option and instead
refcount it such that the VirtioPciDevice can continue to hold onto it
for later activations.
This significantly simplifies the reset() logic as there is no need to
hand back the interrupt.
A few devices used whether the interrupt was Some to make triggering an
interrupt a no-op. However the MSI-X interrupt routing already drops the
interrupt if the driver hasn't yet configured the vector so it is safe
to trigger the interrupt before device activation (e.g. balloon resize
request before driver loaded).
VirtioCommon still retains an Option<..> for the interrupt as the
interrupt is not known until activation time (after this has been
created). A helper VirtioCommon::trigger_interrupt() has been added to
handle this.
Signed-off-by: Rob Bradford <rbradford@meta.com>
All disk format backends now implement AsyncFullDiskFile directly.
The DiskBackend enum that dispatched between Legacy and Next arms
is no longer needed since the factory returns trait objects and vmm
no longer constructs format types manually.
Replace DiskBackend with Box<dyn AsyncFullDiskFile> in the Block
struct and its constructor. Remove the DiskBackend::Next wrapping
in device_manager and the fuzz target.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
There is no reason for most of the Request struct to be writable from
anywhere in the codebase. Encapsulate it.
Use getter functions for access outside the request module. Replace the
trivial setter for the writeback field with direct assignment.
No functional change intended.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
The new_ prefix in Rust conventionally denotes constructors that return
Self (e.g. Vec::new(), File::new()). AsyncDiskFile::new_async_io does
not return Self. It is a factory method that constructs and returns a
Box<dyn AsyncIo> worker bound to the disk file descriptor and
metadata. The create_ prefix communicates this: the caller receives
a freshly constructed object of a different type.
This rename touches every format backend in block plus two external
callers in virtio-devices and performance-metrics. Every change is a
mechanical s/new_async_io/create_async_io/ substitution. No functional
change.
Ref: #7877 (task 3.2.8)
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
The driver needs to be notified with the number of bytes written by the
device. Ensure that the correct number of bytes is reported.
Signed-off-by: Rob Bradford <rbradford@meta.com>
Rename from iommu to access_platform_enabled. The original name was
iommu as this feature was exposed for devices behind an IOMMU however
this feature is also now used for confidential VMs so adopt a more
general name.
Signed-off-by: Rob Bradford <rbradford@meta.com>
This forwards through to the VirtioCommon implementation and can be used
to simplify the virtio PCI access code.
Signed-off-by: Rob Bradford <rbradford@meta.com>
Use the new virtio feature gated accessor when creating the handlers for
the virtio devices. This now means that the translations via the
accessor will only be applied if the feature is acked in accordance with
the spec.
Signed-off-by: Rob Bradford <rbradford@meta.com>
Virtio v1.2 says that if CONFIG_WCE is negotiated
but FLUSH is not, the device must initialize writeback to 0.
It also says that if CONFIG_WCE was not negotiated but FLUSH
was, the driver should assume presence of a writeback cache.
Introduce a pure is_writeback_enabled helper and a
set_writeback_mode helper. This makes the two call flows
explicit:
* write_config resolves the guest requested mode against the
negotiated features before storing it back
* activate starts from the default writeback preference and then
resolves it against the negotiated features
* reset restores the initial writeback state
This keeps the config space value and the runtime writeback flag
in sync and makes the spec driven fallback easier to follow.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
PR #7852 fixed the missing VirtioBlockConfig fields but did not
change the feature advertisement logic. The condition
`sparse || disk_image.supports_zero_flag()` causes qcow2 to
advertise DISCARD even with sparse=false, because qcow2 can
mark clusters as zero (supports_zero_flag() returns true).
Windows viostor BSODs (DRIVER_IRQL_NOT_LESS_OR_EQUAL) when
DISCARD is advertised on qcow2 backends, making sparse=off
ineffective as a workaround for qcow2 images.
Restrict DISCARD to explicit sparse=true only. WRITE_ZEROES
remains available for all sparse-capable backends.
Fixes#7849
Signed-off-by: CMGS <ilskdw@gmail.com>
The sync fallback path in process_queue_submit() hardcoded
VIRTIO_BLK_S_IOERR for all errors. This caused unsupported
request errors to report IOERR to the guest instead of the
correct VIRTIO_BLK_S_UNSUPP.
Use ExecuteError::status() to return the appropriate status
code for each error variant.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Use logical_size() instead of physical_size() for the byte-range
advisory lock. physical_size() returns st_blocks*512 which is the
actual host allocation and can be smaller than the guest visible
extent on sparse files, leaving part of the range unprotected.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
When configuring multiple queues for a virtio device, the guest can
activate between 1 and the configured amount of queues. The firmware,
for example, may activate only one queue, while a Linux guest would
likely activate all available queues.
The constructor of virtio-blk initializes the `paused_sync` barrier
using the configured queue count (plus one for the main thread). This
can be wrong if the guest enable a different number of queues at
activation time, which can make pause hang. Thus, we now recompute the
barrier size from the queues that are actually activated.
On-behalf-of: SAP sebastian.eydam@sap.com
Signed-off-by: Sebastian Eydam <sebastian.eydam@cyberus-technology.de>
Split the data length check into two conditions:
- reject descriptors shorter than one
virtio_blk_discard_write_zeroes segment, and
- reject payloads exceeding MAX_DISCARD_WRITE_ZEROES_SEG segments
Previously only the minimum length was checked and extra segments
were silently dropped.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Change Block to hold DiskBackend instead of
Box<dyn async_io::DiskFile>. In device_manager, existing formats
(raw, vhd, vhdx) are wrapped in DiskBackend::Legacy while
QcowDiskSync uses DiskBackend::Next. The fuzz target is updated
accordingly.
The Error::DiskResize variant now carries BlockError instead of
DiskFileError, matching the BlockResult return type of
DiskBackend::resize().
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Set discard_sector_alignment from the logical block size
reported by the backend topology instead of hardcoding it
to 1 sector. This gives the guest accurate alignment hints
so it can avoid sub block discards that the filesystem
might silently ignore.
For example, on a 4K block filesystem the alignment is now
8 sectors (4096/512) instead of 1.
For image formats with their own allocation units (QCOW2
clusters, VHD/VHDX block sizes), the ideal alignment would
be derived from the format cluster/block size. This is
left for a followup that surfaces allocation granularity
through DiskTopology.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
When VIRTIO_BLK_F_DISCARD or VIRTIO_BLK_F_WRITE_ZEROES features
are advertised, the virtio spec v1.2, sections 5.2.4 and
5.2.6.1, requires the corresponding VirtioBlockConfig fields
to contain valid, non zero values. Leaving them at zero causes
strictly behaved drivers to either reject the features or crash.
Populate max_discard_sectors, max_discard_seg,
discard_sector_alignment, max_write_zeroes_sectors,
max_write_zeroes_seg and write_zeroes_may_unmap after
feature advertisement so drivers can safely negotiate
these features.
Fixes: #7849
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
We ran the Microsoft Windows installer in CHV (via network + VNC) and
the installation always failed when the installer wanted to write the
partition table. Since recently, for very good reason, sector 0 writes
are disabled if the image type is not set explicitly and only
implicitly auto-detected as raw [0].
To ease troubleshooting, I've added a descriptive log message. It is a
little spammy, but it is what is required to help users to troubleshoot.
[0] 6ecdf90e22
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com
Signal the guest before processing queue submissions to enable
earlier guest side completion event handling, reducing end-to-end
latency for block device operations.
FIO benchmarks show up to 7.4% bandwidth improvement at 16
iodepth and 4k block size with NVMe devices.
Signed-off-by: wuxinyue <wuxinyue.wxy@antgroup.com>
Add a per-disk lock_granularity parameter that lets users choose
between byte-range OFD locks and whole-file OFD locks:
--disk path=/foo.img,lock_granularity=byte-range
--disk path=/bar.img,lock_granularity=full
Byte-range is the default and matches QEMU behavior, working
best with storage backends where whole-file OFD locks are treated
as mandatory. The full option restores the original whole-file
locking for environments that depend on it.
The LockGranularityChoice enum and its FromStr impl live in the
block crate alongside the existing LockGranularity type. The
Block device converts the user-facing choice to the internal
LockGranularity at lock time, keeping device_manager.rs simple.
Closes: #7553
Signed-off-by: Victor Vieux <vieux@repl.it>
Rename the transport feature bit constant from
VIRTIO_F_IOMMU_PLATFORM to VIRTIO_F_ACCESS_PLATFORM across
the entire virtio-devices crate.
The virtio specification as of v1.1 carries bit 33 as
VIRTIO_F_ACCESS_PLATFORM. The Linux kernel UAPI header
<linux/virtio_config.h> carries VIRTIO_F_IOMMU_PLATFORM
only as a backward-compatible alias.
This is a pure rename with no functional or behavioral
change.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
As well as rejecting writes to sector 0 in the case of raw files where
the user hasn't specified the image_type also reject virtio requests of
type discard and write_zeroes.
Signed-off-by: Rob Bradford <rbradford@meta.com>
If the disk image was autodetected to raw (not specified with image_type
= 0) then in the virtio-block subsystem generate errors for writes to
block 0 (treat as if read-only). This gives an immediate error vs using
the image implementations in the block subsystem.
Signed-off-by: Rob Bradford <rbradford@meta.com>
Unlike most virtio feature bits, VIRTIO_BLK_F_RO is not optional.
It indicates that the host is refusing to permit write operations, and
the guest must not be allowed to override it.
However, the block device currently does not enforce this. If the guest
does not negotiate VIRTIO_BLK_F_RO, the block device will think the
device is writable and forward write requests to the backend.
This is not a security problem right now because the backing device of a
read-only device is always opened read-only. The kernel will thus
reject the write operations with EBADF. If support is added for
receiving the backing device file descriptor via SCM_RIGHTS (#7704),
it will be possible to have a read-only block device backed by a
writable file descriptor. This would make the bug a genuine security
vulnerability.
Fix the bug by explicitly checking if VIRTIO_BLK_F_RO was offered but
not negotiated. In this case, log a warning and proceed as if the guest
did acknowledge the feature. This always indicates a guest driver bug.
Fixes: #7697
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
OVMF sends FLUSH requests to read-only virtio-block devices. Refusing
these requests prevents OVMF from accessing the EFI System Partition and
therefore makes VMs unable to boot. Accept these requests instead.
them.
Ignoring these requests is possible, but inconsistent with fsync(2)
which honors them.
Fixes: #7698
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
Add supports_zero_flag() to DiskFile trait to indicate whether a disk
format can mark clusters/blocks as reading zeros without deallocating
storage.
QCOW2 supports this via the zero flag in L2 entries. VHDX also has
PAYLOAD_BLOCK_ZERO state for this, though it's not yet implemented in
cloud-hypervisor.
This enables DISCARD to be advertised even with sparse=false for formats
with zero-flag support, since they can mark regions as zeros (keeps
storage allocated) instead of requiring full deallocation.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Add sparse boolean configuration option to DiskConfig with a default
value of true to control disk space allocation behavior.
When sparse is true, the disk uses sparse allocation where deallocated
blocks are returned to the filesystem, and the DISCARD feature is
advertised to the guest.
When sparse is false, disk space is kept fully allocated and DISCARD
is not advertised.
WRITE_ZEROES is always advertised when the backend supports it,
regardless of the sparse setting.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Add capability query to DiskFile trait to check backend
support for sparse operations (punch hole, write zeroes,
discard). Only advertise VIRTIO_BLK_F_DISCARD and
VIRTIO_BLK_F_WRITE_ZEROES when the backend supports these
operations.
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
