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

vmm: support starting stratovirt lightweight vm sandbox #93

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

Vanient
Copy link
Member

@Vanient Vanient commented Sep 12, 2023

vmm-sandboxer support starting stratovirt lightweight vm sandbox, which uses lightweight microvm mainboard and mmio bus.

@Vanient Vanient requested a review from a team as a code owner September 12, 2023 07:05
const ROOTFS_KERNEL_PARAMS: &str = " root=/dev/vda ro rootfstype=ext4";
#[cfg(target_arch = "aarch64")]
const ROOTFS_KERNEL_PARAMS: &str = " root=/dev/vda1 ro rootfstype=ext4";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it different before? and is it unified now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is unified now, i used an unofficial image in arm before, which would divide a vda1 patition, now i use the image provided by stratovirt to start vm, it will not devide vda1.

@@ -85,17 +85,25 @@ impl VMFactory for StratoVirtVMFactory {
// create pcie.0 root bus object
vm.pcie_root_bus = create_pcie_root_bus();

let mut transport = Transport::Pci;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the transport is strongly related to the machine type, maybe we can add a transport() in Machine to determine the transport by the machine type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

// set the pcie slot status to Occupied
self.pcie_root_bus.bus.slots[slot_index].status = SlotStatus::Occupied(device.id());
device.set_device_addr(slot_index);
if !self.config.machine.r#type.contains(MACHINE_TYPE_MICROVM) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes the machine type of microvm a special case, can we just make the pcie_root_bus of StratoVirtVM an Option so that we just need to check if it is a None here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@Vanient Vanient force-pushed the main branch 3 times, most recently from cf51efb to 74e939c Compare October 9, 2023 06:18
@@ -450,11 +458,14 @@ impl StratoVirtVM {
&mut self,
mut device: T,
) -> Result<()> {
// get the empty slot index
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now microVM support a new virtual bus typemmio, so attatch_to_pcie_rootbus func name is not accurate, maybe attach_to_bus is more clear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines +461 to +468
if self.pcie_root_bus.is_some() {
// get the empty slot index
let slot_index = self.get_empty_pcie_slot_index()?;
// set the pcie slot status to Occupied
self.pcie_root_bus.as_mut().unwrap().bus.slots[slot_index].status =
SlotStatus::Occupied(device.id());
device.set_device_addr(slot_index);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if self.pcie_root_bus.is_some() {
// get the empty slot index
let slot_index = self.get_empty_pcie_slot_index()?;
// set the pcie slot status to Occupied
self.pcie_root_bus.as_mut().unwrap().bus.slots[slot_index].status =
SlotStatus::Occupied(device.id());
device.set_device_addr(slot_index);
}
if let Some(&mut pcie_root_bus) = self.pcie_root_bus {
...
pcie_root_bus.bus.slots[slot_index].status =
SlotStatus::Occupied(device.id());
...
}

I am not sure does it work? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried but it did not work well...
move occurs because pcie_root_bus does not implement the Copy trait, but self.get_empty_pcie_slot_index needs to use it next

@@ -147,8 +153,10 @@ impl VMFactory for StratoVirtVMFactory {
share_fs_path.as_str(),
);

// set pcie-root-ports for hotplugging
vm.create_pcie_root_ports(PCIE_ROOTPORT_CAPACITY)?;
if vm.config.machine.r#type.split(',').next().unwrap() != MACHINE_TYPE_MICROVM {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful with unwrap(). How about collect() substrings after split(), and handle the string array.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@Vanient Vanient force-pushed the main branch 2 times, most recently from b33d300 to 2a70820 Compare October 20, 2023 07:10
vmm-sandboxer support starting stratovirt lightweight vm sandbox,
which uses lightweight microvm mainboard and mmio bus.

Signed-off-by: Vanient <xiadanni1@huawei.com>
@flyflypeng flyflypeng self-requested a review October 23, 2023 06:12
Copy link
Member

@Burning1020 Burning1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@flyflypeng flyflypeng merged commit 7b6c933 into kuasar-io:main Nov 8, 2023
13 checks passed
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.

4 participants