firecracker icon indicating copy to clipboard operation
firecracker copied to clipboard

Refactor `vmm` builder code to simplify logic that creates the microVM to boot

Open bchalios opened this issue 1 year ago • 4 comments

Updated on 05/09/2025:

Currently, the logic to create a Vm and a Vmm objects is scattered across vmm/lib.rs and vmm.builder.rs files and it is quite convoluted and some times difficult to follow. Moreover, there is a lot of architecture specific code inserted in arbitrary places which further increases the un-readability.

Currently Vmm object is a mix bag of Vm related fields and miscellaneous management fields. This makes the whole differentiation between Vm and Vmm more difficult and testing more convoluted. The goal of this issue is to move all Vm related fields (vcpus_handles, device_manager and device_manager) into Vm. This also goes for all attach_*_device functions and unit tests from builder.rs The final solution should have approximately this look:

pub fn build_microvm_for_boot(
    instance_info: &InstanceInfo,
    vm_resources: &super::resources::VmResources,
    event_manager: &mut EventManager,
    seccomp_filters: &BpfThreadMap,
) -> Result<Arc<Mutex<Vmm>>, StartMicrovmError> {
    // vm_resources verification

    // All Kvm, Vcpu creation and device attachment can
    // happen here
    let vm = Vm::new(vm_resources, event_manager)?;
    // or move registration into separate function
    let vm = Vm::new(vm_resources)?;
    vm.register_device_events(event_manager)?;

    let vmm = Vmm {
        vm,
        ...
    };

    // GDB configuration

    vmm.vm.start_vcpus(...vcpu seccomp filter)?;
    ...
}

bchalios avatar Apr 09 '24 09:04 bchalios

Hi, may I take this task to practice?

tommady avatar Oct 25 '24 04:10 tommady

Hi @bchalios in the current main branch

the logic to create a Vmm object is scattered across vmm/lib.rs and vmm.builder.rs files

I don’t see any Vmm object returned in the vmm/lib.rs file. Please let me know if I’ve misunderstood.

since we often need to construct dummy versions of objects (like EventManager) even though they are not used in the part of the code we are trying to test.

Could you point me to an example of this? Or are you referring to

event_manager = EventManager::new() 

as the so-called dummy version? and in the create_vmm_and_vcpus function, for instance, event_manager is passed to allow subscription to serial_device. This makes it unclear why event_manager could be omitted.

Could you share your thoughts on how you would approach avoiding the need to pass event_manager, as you mentioned?

After all, may I ask why the create_vmm_and_vcpus function is specified only for aarch64? It seems to be used in several places, even with x86_64.

Otherwise, I completely agree with your points and will work on addressing them.

thanks.

tommady avatar Nov 13 '24 06:11 tommady

I just found this https://github.com/firecracker-microvm/firecracker/issues/4411 seems related

tommady avatar Nov 14 '24 05:11 tommady

Hi @bchalios @roypat I tried to let the architecture-specific cfg contained in

  1. build_microvm_for_boot
  2. build_microvm_from_snapshot

Kindly review the PR at your convenience. Thank you!

tommady avatar Nov 15 '24 22:11 tommady