Re: [PATCH v2] ch: Enable hyperv hypervisor

On 2/20/24 23:06, Praveen K Paladugu wrote: > From: Praveen K Paladugu <prapal(a)linux.microsoft.com&gt; > > 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(a)linux.microsoft.com&gt; > Signed-off-by: Praveen K Paladugu <praveenkpaladugu(a)gmail.com&gt; > --- > 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(a)redhat.com&gt; Michal