NVMe keepalive feature #210

yupavlen-ms · 2024-10-29T19:39:17Z

NVMe keepalive feature implementation. The goal is to keep attached NVMe devices intact during OpenVMM servicing (e.g. reloading new OpenVMM binary from the host OS).

The memory allocated for NVMe queues and buffers must be preserved during servicing, including queue head/tail pointers. The memory region must be marked as reserved so VTL2 Linux kernel will not try to use it, a new device tree property dma-preserve-pages will be passed from Host to indicate the preserved memory size (the memory is extracted from vtl2 available memory, it is not additional memory).
The NVMe device should not go through any resets, from physical device point of view there was no change during servicing.
New OpenHCL module gets saved state from host and continues from there.

mattkur · 2024-10-29T19:59:13Z

Thanks for submitting this Yuri. I'm starting to take a look.

vm/devices/user_driver/src/memory.rs

vm/devices/storage/nvme/src/pci.rs

support/sparse_mmap/src/unix.rs

vm/devices/storage/disk_nvme/nvme_driver/src/queues.rs

jstarks · 2024-10-29T20:18:14Z

vm/devices/storage/disk_nvme/nvme_driver/src/queues.rs

+    #[mesh(7)]
+    pub base_mem: u64,
+    #[mesh(8)]
+    pub pfns: Vec<u64>,


The queues are always linear in physical memory, so we should just need the base gpa here.

Yes, agree.

Updating my reply: when we call mmap() in LockedMemorySpawner, the VA region is contiguous but underlying PFNs are not contiguous.

But since we're moving to mshv driver in fixed pool allocator, this might not be the case anymore and it will be safe to assume sequential PFNs. TODO: check.

vm/devices/storage/disk_nvme/nvme_driver/src/queue_pair.rs

jstarks · 2024-10-29T20:24:34Z

vm/loader/loader_defs/src/shim.rs

@@ -88,6 +88,8 @@ open_enum! {
        VTL0_MMIO = 5,
        /// This range is mmio for VTL2.
        VTL2_MMIO = 6,
+        /// Memory preserved during servicing.
+        VTL2_PRESERVED = 7,


How do these entries make it into the tables? Does this mean we tell the host about these ranges somehow?

Or does the host just know what the preserved range is? Is this some new IGVM concept?

Host knows what the DMA preserved size should be but does not calculate the ranges. Host provides size through device tree, boot shim selects the top of vtl2 memory range and marks it as reserved with the new type.

I think we need some way to enforce that the next version of ohcl selects the same size + location as previous. Outside the scope of this PR, but i'll incorporate it in some changes I'm making moving forward.

vm/devices/storage/disk_nvme/nvme_driver/src/namespace.rs

vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs

jstarks · 2024-10-29T20:40:05Z

vm/devices/storage/disk_nvme/nvme_driver/src/queue_pair.rs

+                command: FromBytes::read_from_prefix(cmd.command.as_bytes()).unwrap(),
+                respond: send,
+            };
+            pending.push((cmd.cid as usize, pending_command));


It seems like pending commands need to save the fact that they allocated memory, somehow.

Specifically, any PRP list allocation needs to be saved/restored. Is this done somewhere?

We also need to re-lock any guest memory/re-reserve any double buffer memory that was in use.

And of course, all this needs to be released after the pending command completes.

If PRP points to the bounce buffer (vtl2 buffer) then its GPN should not change and it still points to the same physical page which is preserved during servicing.
Same should be true if PRP points to a region in vtl0 memory as we don't touch vtl0 during servicing.

However, maybe I missed the part on where we allocate PRP list itself, if there are multiple entries. If it is not a part of preserved memory then such change needs to be added.

vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs

jstarks · 2024-10-29T20:55:54Z

vm/devices/storage/disk_nvme/nvme_driver/src/queue_pair.rs

+    }
+
+    /// Restore queue data after servicing.
+    pub fn restore(&mut self, saved_state: &QueuePairSavedState) -> anyhow::Result<()> {


A key thing to reason about is what happens to requests that were in flight at save. It seems that the design is that we will keep those requests in flight (i.e., we won't wait for them to complete before saving/after restoring), right? And so storvsp's existing restore path will reissue any IOs that were in flight, but they won't wait for old IOs.

I think this is a problem, because new IOs can race with the original IO and cause data loss. Here's an example of what can go wrong:

Before servicing, the guest writes "A" to block 0x1000 with CID 1.

Before CID 1 completes, a servicing operation starts. CID 1 makes it into the saved state.

At restore, storvsp reissues the IO, so it writes "A" to block 0x1000 with CID 2.

Due to reordering in the backend, CID 2 completes before CID 1.

Storvsp completes the original guest IO.

The guest issues another IO, this time writing "B" to the same block 0x1000. Our driver issues this with CID 3.

CID 3 completes before CID 1.

Finally, some backend code gets around to processing CID 1. It writes "A" to block 0x1000.

So the guest thought it wrote "A" and then "B", but actually what happens is "A" then "B" then "A". Corruption.

There are variants on this, where the guest changes the memory that CID 1 was using and ASAP DMAs it late, after the IO was completed to the guest. This could even affects reads by corrupting guest memory.

The most reasonable solution, in my opinion, is to avoid issuing any more commands to the SQ until all IOs that were in flight at save time have completed. This should be simple to implement and to reason about. A more complicated solution would be to reason about what the in flight commands are doing and only block IOs that alias those pages. I don't think that's necessary for v1.

Did I miss somewhere where this is handled?

The most reasonable solution, in my opinion, is to avoid issuing any more commands to the SQ until all IOs that were in flight at save time have completed. This should be simple to implement and to reason about.

I think we cut off receiving mesh commands once servicing save request is received, let me double check that and at which point this is done.

Regarding draining of the outstanding (in-flight) I/Os - eventually we want to bypass this but for v1 it makes sense to drain them, for simplicity. Let me confirm the path. The draining happens for non-keepalive path.

I think two things are important in the conversation above:

It's okay to drain IOs before issuing new IOs. But: unlike the non-keepalive path, the overall servicing operation should not block on outstanding IOs. This means that the replay will need to remain asynchronous.

I agree with John's analysis. It is unsafe to complete any IOs back to the guest until all outstanding IOs (IOs in progress at the save time to the underlying nvme device) complete from the NVMe device to the HCL environment. I think it is fine to start replaying the saved IOs while the in flight IOs are still in progress. This means you have two choices: (a) wait to begin replaying IOs until everything in flight completes, or (b) build some logic in the storvsp layer to not notify the guest until all the in flight IOs complete. (a) seems simpler to me.

In future changes, I think we will need to consider some sort of filtering (e.g. hold new IOs from guest that overlap LBA ranges, as John suggested in an offline conversation).

So the in-flight IOs will be executed twice if my understanding is correct?

We are saving Pending Commands slab so we can correctly find a CID for outstanding responses once we're back from blackout. We do not resubmit those commands.

I mean in these steps "Before CID 1 completes, a servicing operation starts. CID 1 makes it into the saved state.
At restore, storvsp reissues the IO, so it writes "A" to block 0x1000 with CID 2." the same IO was issued twice with CID 1 and CID 2. Is it still true?

That is true, I can observe that in the instrumented FW that same command may be issued twice - before and after servicing. Let me double check if CID is still same, as there is recent change to randomize CID in NVMe queue.

jstarks · 2024-10-31T16:32:58Z

vm/devices/user_driver/src/lockmem.rs

-            addr, len,
-        )?))
+        // TODO: With the recent change it may not work. Review.
+        Ok(crate::memory::MemoryBlock::new(LockedMemory::new(len)?))


We need to just fail here, right? Because we can't restore the requested GPAs.

Yes. Is it ok to do it in two waves?
1 - Fail here
2 - Return an error and then reinitialize everything as on 1st boot?

E.g. will we prioritize VM health over error discovery? And I mean to defer #2 as future enhancement.

(ignore the link, that is just bullet point)

If this fails, then presumably we no longer know to where a device is sending DMA. I think that means that said DMA will corrupt arbitrary memory. This is based on what I understand so far, so please feel free to correct me. But, if my understanding is correct, then I think you need to fail the restore. The VM will need to be power cycled.

I am still waiting for this path to become a failure.

openhcl/fixed_pool_alloc/Cargo.toml

openhcl/host_fdt_parser/src/lib.rs

openhcl/openhcl_boot/src/main.rs

vm/devices/user_driver/vfio_sys/src/lib.rs

vm/devices/user_driver/Cargo.toml

daprilik · 2024-10-31T18:31:31Z

vm/devices/storage/nvme_spec/src/nvm.rs

@@ -16,7 +16,7 @@ use zerocopy::LE;
 use zerocopy::U16;

 #[repr(C)]
-#[derive(Debug, AsBytes, FromBytes, FromZeroes, Inspect)]
+#[derive(Debug, AsBytes, FromBytes, FromZeroes, Inspect, Clone)]


can you share what fails to compile (if anything) if you remove Clone?

Fails in namespace.rs line 85 - when we restore namespace object if Identify structure was provided from saved state

lots of code has changed in 2 weeks - bumping to check if this is still necessary (and if so - what code requires the Clone bound)

jstarks · 2024-11-21T22:42:00Z

vm/devices/user_driver/vfio_sys/src/lib.rs

+        // SAFETY: The file descriptor is valid and a correctly constructed struct is being passed.
+        unsafe {
+            let id = CString::new(device_id.to_owned())?;
+            ioctl::vfio_group_set_keep_alive(self.file.as_raw_fd(), id.as_ptr())


Why did we put the keep alive IOCTL on the group instead of the device?

Oh, I see, because it needs to be set before we create the device, is that right?

Correct. A future rework for iommufd will eliminate this, but right now we need to send ioctl before the device created.

jstarks · 2024-11-21T22:42:53Z

vm/devices/user_driver/src/vfio.rs

@@ -100,6 +115,10 @@ impl VfioDevice {
        }

        container.set_iommu(IommuType::NoIommu)?;
+        if keepalive {
+            // Prevent physical hardware interaction when restoring.
+            group.set_keep_alive(path.file_name().unwrap().to_str().unwrap())?;


path.file_name().unwrap().to_str().unwrap() is just pci_id, right?

/sys/bus/pci/devices/<pci_id>

yes, file_name() should be just pci_id. Let me test it

jstarks · 2024-11-21T22:45:06Z

vm/devices/user_driver/src/vfio.rs

@@ -410,6 +434,10 @@ impl DeviceRegisterIo for MappedRegionWithFallback {
            self.write_to_file(offset, &data.to_ne_bytes());
        })
    }
+
+    fn base_va(&self) -> u64 {


This appears to be unused.

jstarks · 2024-11-21T22:45:12Z

vm/devices/user_driver/src/vfio.rs

@@ -328,6 +348,10 @@ impl DeviceRegisterIo for vfio_sys::MappedRegion {
    fn write_u64(&self, offset: usize, data: u64) {
        self.write_u64(offset, data)
    }
+
+    fn base_va(&self) -> u64 {


jstarks · 2024-11-21T22:46:36Z

vm/devices/user_driver/src/lib.rs

@@ -58,8 +58,13 @@ pub trait DeviceRegisterIo: Send + Sync {
    fn write_u32(&self, offset: usize, data: u32);
    /// Writes a `u64` register.
    fn write_u64(&self, offset: usize, data: u64);
+    /// Returns base virtual address.
+    fn base_va(&self) -> u64;


jstarks · 2024-11-21T22:47:18Z

vm/devices/user_driver/src/emulated.rs

+    }
+
+    fn restore_dma_buffer(&self, len: usize, _pfns: &[u64]) -> anyhow::Result<MemoryBlock> {
+        self.create_dma_buffer(len)


Same as below, you have to fail this operation.

jstarks · 2024-11-21T22:51:54Z

vm/devices/storage/nvme_spec/src/lib.rs

@@ -126,20 +131,33 @@ pub struct Aqa {
 }

 #[repr(C)]
-#[derive(Copy, Clone, Debug, AsBytes, FromBytes, FromZeroes, Inspect)]
+#[derive(Copy, Clone, Debug, AsBytes, FromBytes, FromZeroes, Inspect, Protobuf)]
+#[mesh(package = "underhill")]


This is not the right approach. We shouldn't be saving our "interpreted" version of Command, we should just save the raw bytes. If we need to save a Command, use ZeroCopyEncoding, which will handle this automatically:

E.g.,

struct MySavedState { #[mesh(encoding = "mesh::payload::encoding::ZeroCopyEncoding")] command: Command, }

There was another comment from another reviewer to go this way instead.... okay, will revert / improve.

More generally - its almost never the right move to add mesh as a dependency to a _spec or _protocol crate, when the actual spec/protocol being encoded is based on C-structs. If you need to take those base protocol types and encode them in protobuf (or JSON, or anything else really) - that should take the form of a type conversion / adapter layer outside the _spec/_protocol crate itself.

This might be a good thing to encode in the Coding Conventions part of our guide.

Agree, doesn't look good to me either. This change was a response to an earlier comment on why we need impl From<> for this type. Since @jstarks suggested a third way, by using ZeroCopyEncoding, I will try that.

jstarks · 2024-11-21T22:55:58Z

vm/devices/storage/disk_nvme/nvme_driver/src/queue_pair.rs

+            sq_state: self.sq.save(),
+            cq_state: self.cq.save(),
+            pending_cmds: self.commands.save(),
+            cpu: 0,       // Will be updated by the caller.


This is an error-prone technique. Can we just nest this structure to avoid this "caller has to remember to fill stuff in" behavior?

jstarks · 2024-11-21T22:59:56Z

vm/devices/storage/disk_nvme/nvme_driver/src/queue_pair.rs

+    /// Save pending commands into a buffer.
+    pub fn save(&self) -> PendingCommandsSavedState {
+        let mut commands = Vec::new();
+        // Convert Slab into Vec.


This is just self.commands.iter().map(|(_index, value)|).collect()

jstarks · 2024-11-21T23:00:37Z

vm/devices/storage/disk_nvme/nvme_driver/src/queue_pair.rs

+            };
+            // Remove high CID bits to be used as a key.
+            let cid = cmd.cdw0.cid() & Self::CID_KEY_MASK;
+            commands.push((cid as usize, pending_command));


Use .collect() for this path, too. Then you don't need to first make a vec and then make a slab.

jstarks · 2024-11-21T23:01:15Z

vm/devices/storage/disk_nvme/nvme_driver/src/queue_pair.rs

+                respond: send,
+            };
+            // Remove high CID bits to be used as a key.
+            let cid = cmd.cdw0.cid() & Self::CID_KEY_MASK;


Should we include this mask in the saved state (or perhaps the max # of CIDs, and recompute the mask) so that we have the flexibility to change this?

jstarks · 2024-11-21T23:01:39Z

vm/devices/storage/disk_nvme/nvme_driver/src/queue_pair.rs

 }

 impl QueuePair {
    pub const MAX_SQSIZE: u16 = (PAGE_SIZE / 64) as u16; // Maximum SQ size in entries.
    pub const MAX_CQSIZE: u16 = (PAGE_SIZE / 16) as u16; // Maximum CQ size in entries.

+    /// Return size in bytes for Submission Queue.
+    fn sq_size() -> usize {


Why is this a function?

jstarks · 2024-11-21T23:01:55Z

vm/devices/storage/disk_nvme/nvme_driver/src/queue_pair.rs

+    }
+
+    /// Return size in bytes for DMA memory.
+    fn dma_data_size() -> usize {


jstarks · 2024-11-21T23:03:32Z

vm/devices/storage/disk_nvme/nvme_driver/src/namespace.rs

+
+/// Save/restore NVMe namespace data.
+#[derive(Protobuf, Clone, Debug)]
+#[mesh(package = "underhill")]


Let's use the "nvme_driver" package name.

Also, as mentioned elsewhere in a previous review, all the saved states pieces need to be in save_restore modules so that it's easy to find the state.

And I'd prefer that we just have one top-level module for nvme_driver, so that we can see all the saved state together in one place.

For all nvme_driver related, except for nvme_manager and fixed_pool, ok.

jstarks · 2024-11-21T23:04:57Z

vm/devices/user_driver/src/memory.rs

+    }
+}
+
+pub mod save_restore {


This module should be in fixed_pool_alloc.

I did that at first, but then can not resolve circular dependencies. I had to find a place for it where it can be accessed by LoadedVm module and also thinking if this can become more generic save/restore for other memory types.

I don't see the circular dependency. This is only used by underhill_core and fixed_pool_alloc.

daprilik · 2024-11-21T23:06:47Z

vm/devices/get/underhill_config/src/lib.rs

@@ -355,6 +355,9 @@ pub enum Vtl2SettingsErrorCode {
    NetworkingAddNicFailed => (Network, Configuration),
    /// Failed to remove NIC
    NetworkingRemoveNicFailed => (Network, Configuration),
+
+    /// Failed to update VTL2 servicing options
+    Vtl2ServicingOptions => (Underhill, Configuration),


looks like this is unused in the latest revision of the PR? at least - based on a quick grep of the codebase

You're right, wondering why clippy didn't flag that.

this is exposed as pub in this crate, and clippy / rust don't do dead-code analysis between crates.

maybe someday we'll have CodeQL rules that will let us catch these sorts of things across the project...

jstarks · 2024-11-21T23:09:21Z

openhcl/underhill_core/src/dispatch/mod.rs

+        if let Some(m) = self.nvme_manager.as_mut() {
+            // Only override if explicitly disabled from host.
+            if !nvme_keepalive {
+                m.override_nvme_keepalive_flag(nvme_keepalive);


Instead of storing a flag in the NvmeManager, let's just take this as a parameter to shutdown.

And instead of having save fail if nvme_keepalive is false, just don't call save() below when you don't want to save.

It used to be as a parameter to shutdown but I had to redo it this way to cover some failing cases. Let me get back to this with details once I review.

jstarks · 2024-11-21T23:12:03Z

vm/devices/storage/disk_nvme/nvme_driver/src/namespace.rs

+            identify,
+        )?;
+
+        // Spawn a task, but detach is so that it doesn't get dropped while NVMe


Don't copy/paste this code. Can this not be in new_from_identify?

Hmmm I think I had issues with async/non-async, let me check again

jstarks · 2024-11-21T23:14:25Z

vm/devices/storage/disk_nvme/nvme_driver/src/namespace.rs

+    #[mesh(2)]
+    pub block_count: u64,
+    #[mesh(3)]
+    pub identify_ns: [u8; 4096],


Suggested change

pub identify_ns: [u8; 4096],

#[mesh(encoding = "mesh::payload::encoding::ZeroCopyEncoding")]

pub identify_ns: nvme_spec::nvm::IdentifyNamespace,

jstarks · 2024-11-21T23:21:02Z

vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs

+            // TODO: Current approach is to re-query namespace data after servicing
+            // and this array will be empty. Once we confirm that we can process
+            // namespace change notification AEN, the restore code will be re-added.
+            this.namespaces.push(Arc::new(Namespace::restore(


If we're not going to do this yet, then let's remove this namespaces field and put #[allow(dead_code)] on Namespace::restore.

jstarks · 2024-11-21T23:22:56Z

vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs

+    pub fn required_dma_size(expect_q_count: usize) -> usize {
+        QueuePair::required_dma_size() * expect_q_count
+    }
+
    /// Change device's behavior when servicing.
    pub fn update_servicing_flags(&mut self, nvme_keepalive: bool) {


This function should just become a parameter to shutdown.

jstarks · 2024-11-21T23:27:05Z

vm/devices/storage/disk_nvme/nvme_driver/src/queues.rs

+
+    /// Restores queue data after servicing.
+    pub fn restore(&mut self, saved_state: &CompletionQueueSavedState) -> anyhow::Result<()> {
+        self.cqid = saved_state.cqid;


We should not be overwriting fields that were initialized in new. Either restore should return Self, or it should only restore fields that were modified.

Additionally, we should be using the destructuring pattern to make sure we don't forget any fields:

let CompletionQueueSavedState {head, committed_head, phase} = saved_state; self.committed_head = committed_head; ...

jstarks · 2024-11-21T23:31:43Z

vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs

+        let admin = self.admin.as_ref().unwrap().save().await?;
+        let mut io: Vec<QueuePairSavedState> = Vec::new();
+        for io_q in self.io.iter() {
+            io.push(io_q.save().await?);


I don't see how this is safe at all. The IO queue workers are still running at this point, right? So if there is anything in their in-memory queues, those items may concurrently, at any point during or after the save, get put into the SQ. But we won't save that fact.

We need to stop all the queues before saving, and leave them stopped. save needs to be a destructive operation, that puts the device in a quiesced state.

Do you agree with this analysis?

save needs to be a destructive operation, that puts the device in a quiesced state

agree, after today's discussion I have same thought.

jstarks · 2024-11-21T23:33:55Z

vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs

+        #[mesh(5)]
+        pub mem_len: usize,
+        #[mesh(6)]
+        pub pfns: Vec<u64>,


Didn't I already give this feedback? 1. Why are we saving the PFN list twice, once in the queue pair and once in the SQ/CQ state? 2. Why are we saving a list given that queues are always contiguous? Can't we just save the CQ base and the SQ base?

jstarks · 2024-11-21T23:34:36Z

vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs

+
+    #[derive(Protobuf, Clone, Debug, FromBytes, FromZeroes)]
+    #[mesh(package = "underhill")]
+    pub struct PendingCommandSavedState {


This is not used.

yupavlen-ms requested review from a team as code owners October 29, 2024 19:39