vmm: move logic for creating vcpus #1004
Conversation
dianpopa
requested review from
alxiord,
andreeaflorescu,
alexandruag,
acatangiu and
dhrgit
vmm/src/vmm_config/instance_info.rs
Outdated
| @@ -101,6 +101,8 @@ pub enum StartMicrovmError { | |||
| VcpuConfigure(vstate::Error), | |||
| /// vCPUs were not configured. | |||
| VcpusNotConfigured, | |||
| /// vCPUs were not created prior to starting them. | |||
| VcpusNotCreated, | |||
There was a problem hiding this comment.
Is it possible to ever get this error? At least in current implementation, it seems start_vcpus is invoked right after create_vcpus, so they're always created before being started.
There was a problem hiding this comment.
You can get this error if start_vcpus is called before create_vcpus (programming error) but this misuse will also be caught by the assert_eq!(vcpus.len(), vcpu_count as usize, "The number of vCPU fds is corrupted!" );
so I will only leave this assert and remove this error from this enum which indeed should only keep around user triggered errors.
There was a problem hiding this comment.
Removed the error. Take a look.
| .map_err(|e| VmmActionError::StartMicrovm(ErrorKind::Internal, e))?; | ||
|
|
||
| self.start_vcpus(vcpus) |
There was a problem hiding this comment.
Is there anything missing here between calling create_vcpus and start_vcpus? It seems the IRQ chip still gets set up as part of init_microvm for all architectures, and init_microvm gets called before these two functions (line 1275).
There was a problem hiding this comment.
Creating the irqchip after calling create_vcpus is mandatory for aarch64 not x86_64 which is the reason behind carving out the create_vcpus logic from start_vcpus. No aarch64 logic was added yet. That will come in following PRs. So, all this PR does is move away create_vcpu logic for aarch64 specific purposes.
There was a problem hiding this comment.
We agreed to move to a separate function the setup of the interrupt controller for x86_64 to make more obvious the order of operations that need to be respected when initializing the microvm.
There was a problem hiding this comment.
Created setup_interrupt_controller function and did some additional refactoring.
|
@aghecenco @alexandruag please re-review. |
On aarch64 KVM_CREATE_VCPU needs to be called before setting up the IRQCHIP, while on x86 it needs to be called after setting up the IRQCHIP. Consequently the configuration of the vcpus was moved to a different function. Also, architecture specific labeling was added. Signed-off-by: Diana Popa <[email protected]>
dianpopa
force-pushed
the
i757e
branch
2 times, most recently
from
e1fedf2 to
34a9bd3
Compare
* separate function for seting up the interrupt controller * init_devices was renamed to attach_virtio_devices * init_microvm was removed since its functionality was moved to the relevant functions * created new functions for attaching legacy devices * added unit tests Signed-off-by: Diana Popa <[email protected]>
On aarch64 KVM_CREATE_VCPU needs to be called before setting up the
IRQCHIP, while on x86 it needs to be called after setting up the
IRQCHIP. Consequently the configuration of the vcpus was moved to a
different function.
Also, architecture specific labeling was added.
Signed-off-by: Diana Popa [email protected]
Issue #, if available: #757
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.