Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DRAFT] Prerequisite for proper device manager. #4509

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ShadowCurse
Copy link
Contributor

@ShadowCurse ShadowCurse commented Mar 16, 2024

Reason

Currently Firecracker does not have a single DeviceManger but instead has MMIODeviceManager and PortIODeviceManager (only for x86). This design works for now, but with an addition of ACPI devices in #4428 and #4487 it starts to show limitations. The biggest problem is a resource allocation sharing for interrupts and memory ranges.
In order to fix this, this PR clears current device management (mainly MMIO) code in attempt to move all unnecessary code out of it. This will allow creation of a proper DeviceManger which will have all resource allocators (irqs, memory ranges) and will handle their allocation for MMIO and ACPI devices.
This PR does not achieve this goal yet. Next steps would be to move all attach_*_device methods from builder.rs into DeviceManager.

Main idea can be visualized like this:

// current design

struct Vmm {
  ..
  mmio_device_manager ----> struct MMIODevicManger {
  pio_device_manager               irq_allocator,
}                                  mmio_memory_allocator,
                            }

// when we want to add device
// we use methods from `builder.rs` aka `attach_boot_timer_device` ...
// these methods just call specialized `MMIODeviceManager` methods
fn attach_boot_timer_device(...) -> ... {
    let boot_timer = crate::devices::pseudo::BootTimer::new(request_ts);
    vmm.mmio_device_manager
        .register_mmio_boot_timer(boot_timer)
        .map_err(RegisterMmioDevice)?;
}


// new design
struct Vmm {
  ..
  device_manager ----> struct DevicManger {
}                             mmio_device_manager,
                              pio_device_manager,
                              acpi_device_manager,
                              irq_allocator,
                              mmio_memory_allocator,
                       }

// when we want to add device
vmm.device_manager.add_device(...)
// and inside `DeviceManger` there will be special logic for creating devices

As an additional point, with single DeviceManager the storing of devices information in a snapshot will become more manageable, as we will need to store only 1 struct, instead of several.

Changes

TBD

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this
    PR.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • New TODOs link to an issue.
  • Commits meet
    contribution quality standards.

  • This functionality cannot be added in rust-vmm.

Add single entity to controll all devices in the vmm.
There are currently 2 types of devices in
Firecraker: mmio and pio devices. Each type is
managed by it's own device manager. This works fine
in current setup. But with prospect of adding ACPI
there will be 3 types of devices and all interactions
with them will become more entangled than they are
right now. To prevent this from happening single
device manager will controll all devices and have
all device specific logic inside of it.

Signed-off-by: Egor Lazarchuk <[email protected]>
Now registration of KVM resources is not attached to the
mmio_device_manager and can be performed outside of it.

Signed-off-by: Egor Lazarchuk <[email protected]>
Update of cmd line with virtio devices does not need to access
to the mmio_device_manager and can be done outside of it.

Signed-off-by: Egor Lazarchuk <[email protected]>
MMIODeviceManager was aware of different types
of devices like Rtc or BootTimer. Because of this
MMIODeviceManager had specialized methods to deal
with such devices. This commit tries to generalize
addition of such devices, so MMIODeviceManager will
not differentiate between them.

Signed-off-by: Egor Lazarchuk <[email protected]>
SerialDevice is always used with stdin as an input
source. Setting generic parameter of SerialDevice
to always be Stdin removes duplication of it's
definitions all over codebase. Also it allows to make a
simple `new` method to create SerialDevice instead
of specifying each field separately.

Signed-off-by: Egor Lazarchuk <[email protected]>
MMIODeviceInfo now handles registration of irqfd.
This registration can happen outside of MMIODeviceManager
in the future.

Signed-off-by: Egor Lazarchuk <[email protected]>
Rename allocate_mmio_resources into allocate_device_info
to better specify the method output.

Signed-off-by: Egor Lazarchuk <[email protected]>
Remove special method for serial device from MMIODeviceManager.
Now MMIODeviceManager does not have any device specific
methods.

Signed-off-by: Egor Lazarchuk <[email protected]>
setup_serial_device was only used for x86_64 PortIODeviceManager
creation. Moving creation of serial device near creation
of PortIODeviceManager removes a need to jump through the code
to see how serial device is created.

Signed-off-by: Egor Lazarchuk <[email protected]>
MMIODeviceManager's implementation was spread across 2 modules:
mmio.rs and persist.rs. By moving all implementation into mmio
module we wil have an easir time when it comes to add more
device managers (e.g. ACPI device_manager).

Signed-off-by: Egor Lazarchuk <[email protected]>
Before the inner `MMIODeviceManager` was responsible for state of
devices. With this commit it will be easier to save states of other
device types (e.g. ACPI devices).

Signed-off-by: Egor Lazarchuk <[email protected]>
New `add_device_with_info` method plays a bit better with other new
`add_*` methods.

Signed-off-by: Egor Lazarchuk <[email protected]>
@ShadowCurse ShadowCurse self-assigned this Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant