On 2/20/24 23:06, Praveen K Paladugu wrote:
> From: Praveen K Paladugu <prapal@xxxxxxxxxxxxxxxxxxx>
>
> Cloud-Hypervisor is capable of running VMs with kvm or mshv as the
> hypervisor on Linux Host. Guest to hypevisor ABI with mshv hypervisor is
> the same as in the case of VIR_DOMAIN_VIRT_HYPERV. So, VIR_DOMAIN_VIRT_HYPERV
> type will be reused to represent the config with Linux Host and mshv as the
> hypervisor.
>
> While initializing ch driver, check if either of /dev/kvm or /dev/mshv
> device is present on the host. Before starting ch domains, check if the
> requested hypervisor device is present on the host.
>
> Users can specify hypervisor in ch guests's domain definitions like
> below:
>
> <domain type='kvm'>
>
> _or_
>
> <domain type='hyperv'>
>
> Signed-off-by: Praveen K Paladugu <prapal@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Praveen K Paladugu <praveenkpaladugu@xxxxxxxxx>
> ---
> src/ch/ch_conf.c | 2 ++
> src/ch/ch_driver.c | 7 +++++++
> src/ch/ch_process.c | 35 +++++++++++++++++++++++++++++++++++
> 3 files changed, 44 insertions(+)
>
> diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c
> index f421af5121..1911ae8f8b 100644
> --- a/src/ch/ch_conf.c
> +++ b/src/ch/ch_conf.c
> @@ -69,6 +69,8 @@ virCaps *virCHDriverCapsInit(void)
>
> virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM,
> NULL, NULL, 0, NULL);
> + virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_HYPERV,
> + NULL, NULL, 0, NULL);
1: This sets support for both virtTypes unconditionally even though only
one might be supported. Problem with this approach is: I, as an user,
check for supported virtTypes (e.g. via 'virsh capabilities') find
hyperv supported only to get an error when trying to start such domain.
> return g_steal_pointer(&caps);
> }
>
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index 96de5044ac..d6294c76ee 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -32,6 +32,7 @@
> #include "viraccessapicheck.h"
> #include "virchrdev.h"
> #include "virerror.h"
> +#include "virfile.h"
> #include "virlog.h"
> #include "virobject.h"
> #include "virtypedparam.h"
> @@ -876,6 +877,12 @@ static int chStateInitialize(bool privileged,
> return -1;
> }
>
> + if (!(virFileExists("/dev/kvm") || virFileExists("/dev/mshv"))) {
> + virReportError(VIR_ERR_DEVICE_MISSING, "%s",
VIR_ERR_DEVICE_MISSING code should be used for cases where a device is
looked up in domain config but it's not found (e.g. on hotunplug).
Though it's used in one other (unrelated?) case too:
virMediatedDeviceNew() - which is transitively called from domain
handling code.
But more importantly, this check needs to go to caps init [1] and here
we should merely check whether caps has at least one of the virtTypes
set (e.g. via virCapabilitiesDomainSupported()).
> + _("/dev/kvm and /dev/mshv. ch driver failed to initialize."));
> + return VIR_DRV_STATE_INIT_ERROR;
> + }
> +
> ch_driver = g_new0(virCHDriver, 1);
>
> if (virMutexInit(&ch_driver->lock) < 0) {
> diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c
> index 3bde9d9dcf..640f72a9ca 100644
> --- a/src/ch/ch_process.c
> +++ b/src/ch/ch_process.c
> @@ -637,6 +637,37 @@ chProcessAddNetworkDevices(virCHDriver *driver,
> return 0;
> }
>
> +/**
> + * virCHProcessStartValidate:
> + * @vm: domain object
> + *
> + * Checks done before starting a VM.
> + *
> + * Returns 0 on success or -1 in case of error
> + */
> +static int virCHProcessStartValidate(virDomainObj *vm)
> +{
> + if (vm->def->virtType == VIR_DOMAIN_VIRT_KVM) {
> + VIR_DEBUG("Checking for KVM availability");
> + if (!virFileExists("/dev/kvm")) {
We should check capabilities instead, just like I'm suggesting above.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Domain requires KVM, but it is not available. Check that virtualization is enabled in the host BIOS, and host configuration is setup to load the kvm modules."));
> + return -1;
> + }
> + } else if (vm->def->virtType == VIR_DOMAIN_VIRT_HYPERV) {
> + VIR_DEBUG("Checking for mshv availability");
> + if (!virFileExists("/dev/mshv")) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Domain requires MSHV device, but it is not available. Check that virtualization is enabled in the host BIOS, and host configuration is setup to load the mshv modules."));
> + return -1;
> + }
> + } else {
> + return -1;
This signals an error but doesn't set an error message.
> + }
> + return 0;
> +
> +}
> +
> /**
> * virCHProcessStart:
> * @driver: pointer to driver structure
> @@ -664,6 +695,10 @@ virCHProcessStart(virCHDriver *driver,
> return -1;
> }
>
> + if (virCHProcessStartValidate(vm) < 0) {
> + return -1;
> + }
> +
> if (!priv->monitor) {
> /* And we can get the first monitor connection now too */
> if (!(priv->monitor = virCHProcessConnectMonitor(driver, vm))) {
Anyway, the idea is sound. So I'm rewriting the code per my suggestions
and merging. Would you please post a follow up patch that adds a note
about this into NEWS.rst? I think it's something that might interest users.
Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
Michal
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
