src/ch/ch_conf.c | 2 ++ src/ch/ch_driver.c | 7 +++++++ src/ch/ch_process.c | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+)
From: Praveen K Paladugu <prapal@linux.microsoft.com>
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@linux.microsoft.com>
Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com>
---
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);
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",
+ _("/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")) {
+ 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;
+ }
+ 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))) {
--
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On 2/20/24 23:06, Praveen K Paladugu wrote:
> From: Praveen K Paladugu <prapal@linux.microsoft.com>
>
> 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@linux.microsoft.com>
> Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com>
> ---
> 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@redhat.com>
Michal
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On 3/8/2024 6:06 AM, Michal Prívozník wrote:
> On 2/20/24 23:06, Praveen K Paladugu wrote:
>> From: Praveen K Paladugu <prapal@linux.microsoft.com>
>>
>> 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@linux.microsoft.com>
>> Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com>
>> ---
>> 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.
As 'mshv' and 'kvm' can be configured to be loadable modules, don't you
think there is value in checking if these devices still exist before
starting a CH process?
Of course, if libvirt doesn't check here, CH process will fail to start.
It will eventually fail anyway.
>
>> + 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.
Thanks Michal. Sure, I will send a patch to update NEWS.rst.
>
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>
> Michal
>
--
Regards,
Praveen
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On 3/8/24 16:55, Praveen K Paladugu wrote:
>
>
> On 3/8/2024 6:06 AM, Michal Prívozník wrote:
>> On 2/20/24 23:06, Praveen K Paladugu wrote:
>>> From: Praveen K Paladugu <prapal@linux.microsoft.com>
>>>
>>> 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@linux.microsoft.com>
>>> Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com>
>>> ---
>>> 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.
>
> As 'mshv' and 'kvm' can be configured to be loadable modules, don't you
> think there is value in checking if these devices still exist before
> starting a CH process?
Good point. I thought capabilities are refreshed when a domain is to be
created, but that's not the case for CH driver.
And comparing the QEMU and CH code:
1) the fact whether system is capable of KVM is stored in virQEMUCaps
and only when virCaps are constructed/refreshed this information is
mirrored into the latter,
2) virQEMUCaps is stored in a cache (virFileCache) and whenever a
reference to virQEMUCaps is requested, the cache is validated, i.e.
basic checks are run (virQEMUCapsIsValid()) , like has mtime of the qemu
binary changed? is the kvm module still loaded and so on.
3) We don't have such checks in CH driver, so if the virchd is started
and then the cloud-hypervisor binary is upgraded the daemon doesn't
reconstruct (version based!) capabilities.
In order to fix this, we should mimic more what QEMU driver is doing.
Are you willing to work on that or should I (since I broke it)?
Michal
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On 3/11/2024 6:46 AM, Michal Prívozník wrote:
> On 3/8/24 16:55, Praveen K Paladugu wrote:
>>
>>
>> On 3/8/2024 6:06 AM, Michal Prívozník wrote:
>>> On 2/20/24 23:06, Praveen K Paladugu wrote:
>>>> From: Praveen K Paladugu <prapal@linux.microsoft.com>
>>>>
>>>> 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@linux.microsoft.com>
>>>> Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com>
>>>> ---
>>>> 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.
>>
>> As 'mshv' and 'kvm' can be configured to be loadable modules, don't you
>> think there is value in checking if these devices still exist before
>> starting a CH process?
>
> Good point. I thought capabilities are refreshed when a domain is to be
> created, but that's not the case for CH driver.
>
> And comparing the QEMU and CH code:
> 1) the fact whether system is capable of KVM is stored in virQEMUCaps
> and only when virCaps are constructed/refreshed this information is
> mirrored into the latter,
>
> 2) virQEMUCaps is stored in a cache (virFileCache) and whenever a
> reference to virQEMUCaps is requested, the cache is validated, i.e.
> basic checks are run (virQEMUCapsIsValid()) , like has mtime of the qemu
> binary changed? is the kvm module still loaded and so on.
>
> 3) We don't have such checks in CH driver, so if the virchd is started
> and then the cloud-hypervisor binary is upgraded the daemon doesn't
> reconstruct (version based!) capabilities.
>
> In order to fix this, we should mimic more what QEMU driver is doing.
> Are you willing to work on that or should I (since I broke it)?
>
I did notice the caching layer around QemuCaps. I will investigate the
caching layer further and fix this behavior in ch driver.
Praveen
> Michal
>
--
Regards,
Praveen
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Fri, Mar 08, 2024 at 01:06:34PM +0100, Michal Prívozník wrote:
> On 2/20/24 23:06, Praveen K Paladugu wrote:
> > From: Praveen K Paladugu <prapal@linux.microsoft.com>
> >
> > 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@linux.microsoft.com>
> > Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com>
> > ---
> > 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()).
Also we should return 'VIR_DRV_STATE_INIT_SKIP' rather than
'ERROR'. THe latter is for a fatal non-recoverable problem
on a host that has the supported features. The former is
for when a host does not support the features needed for
the driver.
>
>
> > + _("/dev/kvm and /dev/mshv. ch driver failed to initialize."));
> > + return VIR_DRV_STATE_INIT_ERROR;
> > + }
> > +
> > ch_driver = g_new0(virCHDriver, 1);
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
ping.. to get this simple patch reviewed.
Regards,
Praveen
On 2/20/2024 4:06 PM, Praveen K Paladugu wrote:
> From: Praveen K Paladugu <prapal@linux.microsoft.com>
>
> 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@linux.microsoft.com>
> Signed-off-by: Praveen K Paladugu <praveenkpaladugu@gmail.com>
> ---
> 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);
> 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",
> + _("/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")) {
> + 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;
> + }
> + 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))) {
--
Regards,
Praveen
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
© 2016 - 2024 Red Hat, Inc.