Re: [PATCH v1] Add basic driver for the Cloud-Hypervisor

On Tue, Apr 27, 2021 at 02:39:46PM -0700, William Douglas wrote: > * On a 64-bit host Cloud-Hypervisor needs to support PVH and so can > emulate 32-bit mode but it isn't fully tested (a 64-bit kernel and > 32-bit userspace is fine, a 32-bit kernel is unsupported) When you say "unsupported", do you merely mean lack of testing, or are they known functional issues ? With any full virt hypervisors I've seen 32-on-64 "just works" in general. Is PVH the /only/ boot mode supported, or does it support the traditional/legacy x86 boot sequence starting from real mode.

> * Updated spec file to disable ch driver by default Is cloud-hypervisor present in any Linux OS distributions yet and or are there plans ? It makes it much more likely that someone will test the libvirt side, if it is actually packaged in common distros.

> diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h > index 524a7bf9e8..57986931fd 100644 > --- a/include/libvirt/virterror.h > +++ b/include/libvirt/virterror.h > @@ -136,6 +136,7 @@ typedef enum { > > VIR_FROM_TPM = 70, /* Error from TPM */ > VIR_FROM_BPF = 71, /* Error from BPF code */ > + VIR_FROM_CH = 72, /* Error from Cloud-Hypervisor driver */ Is "CH" the most common abbreviation / shorthand for refering to Cloud-Hypervisor ?

> diff --git a/libvirt.spec.in b/libvirt.spec.in > index da7af2824e..4eeb4dc1b4 100644 > --- a/libvirt.spec.in > +++ b/libvirt.spec.in > @@ -244,6 +244,9 @@ Requires: libvirt-daemon-driver-lxc = %{version}-%{release} > +%if %{with_ch} Nothing enables "with_ch" and AFAIK cloud-hypervisor isn't in either RHEL or Fedora, so we don't need anything in the RPM until it needs to actually be enabled.

> diff --git a/meson.build b/meson.build > index 951da67896..8c33efb25c 100644 > --- a/meson.build > +++ b/meson.build > @@ -1517,6 +1517,48 @@ elif get_option('driver_lxc').enabled() > error('linux and remote_driver are required for LXC') > endif > > +if not get_option('driver_ch').disabled() and host_machine.system() == 'linux' This logic will end up enabling cloud-hypervisor by default on all architectures. You mention earlier it only targets 64-bit. What about the more obscure 64-bit archs like s390x/ppc64 ? This will build cloud-hypervisor on those archs right now. Does this need to have an arch restriction.

> + > + if (virCapabilitiesAddGuestDomain(guest, > + VIR_DOMAIN_VIRT_KVM, > + NULL, > + NULL, > + 0, > + NULL) == NULL) > + goto cleanup; > + > + ch_cmd = g_find_program_in_path(CH_CMD); > + if (!ch_cmd) > + goto cleanup; You're not using this anywhere . Did you mean to pass it as the emulator arg to virCapabilitiesAddGuest a few lines earlier ?

> +/* Function Tables */ > +static virHypervisorDriver chHypervisorDriver = { > + .name = "CH", > + .connectURIProbe = chConnectURIProbe, > + .connectOpen = chConnectOpen, /* 6.7.0 */ > + .connectClose = chConnectClose, /* 6.7.0 */ > + .connectGetType = chConnectGetType, /* 6.7.0 */ > + .connectGetVersion = chConnectGetVersion, /* 6.7.0 */ > + .connectGetHostname = chConnectGetHostname, /* 6.7.0 */ > + .connectNumOfDomains = chConnectNumOfDomains, /* 6.7.0 */ > + .connectListAllDomains = chConnectListAllDomains, /* 6.7.0 */ > + .connectListDomains = chConnectListDomains, /* 6.7.0 */ > + .connectGetCapabilities = chConnectGetCapabilities, /* 6.7.0 */ > + .domainCreateXML = chDomainCreateXML, /* 6.7.0 */ > + .domainCreate = chDomainCreate, /* 6.7.0 */ > + .domainCreateWithFlags = chDomainCreateWithFlags, /* 6.7.0 */ > + .domainShutdown = chDomainShutdown, /* 6.7.0 */ > + .domainShutdownFlags = chDomainShutdownFlags, /* 6.7.0 */ > + .domainReboot = chDomainReboot, /* 6.7.0 */ > + .domainSuspend = chDomainSuspend, /* 6.7.0 */ > + .domainResume = chDomainResume, /* 6.7.0 */ > + .domainDestroy = chDomainDestroy, /* 6.7.0 */ > + .domainDestroyFlags = chDomainDestroyFlags, /* 6.7.0 */ > + .domainDefineXML = chDomainDefineXML, /* 6.7.0 */ > + .domainDefineXMLFlags = chDomainDefineXMLFlags, /* 6.7.0 */ > + .domainUndefine = chDomainUndefine, /* 6.7.0 */ > + .domainUndefineFlags = chDomainUndefineFlags, /* 6.7.0 */ > + .domainLookupByID = chDomainLookupByID, /* 6.7.0 */ > + .domainLookupByUUID = chDomainLookupByUUID, /* 6.7.0 */ > + .domainLookupByName = chDomainLookupByName, /* 6.7.0 */ > + .domainGetState = chDomainGetState, /* 6.7.0 */ > + .domainGetXMLDesc = chDomainGetXMLDesc, /* 6.7.0 */ > + .domainGetInfo = chDomainGetInfo, /* 6.7.0 */ > + .domainIsActive = chDomainIsActive, /* 6.7.0 */ > + .nodeGetInfo = chNodeGetInfo, /* 6.7.0 */ > +}; FWIW, the versions can be bumped just before sending each patch version

> +static virConnectDriver chConnectDriver = { > + .localOnly = true, > + .uriSchemes = (const char *[]){"ch", "Cloud-Hypervisor", NULL}, AFAIK, you only handle ch:// scheme, not Cloud-Hypervisor://, so only the former should be listed.

In general my impression is that this looks like a decent starting point for a libvirt driver. It shows enough to demonstrate the overall architecture, and you've followed normal libvirt coding style and architectural designs pretty well. My preference is to get something merged sooner rather than later, to make it easier to then get on with ongoing incremental dev, without having to carry around a huge out of tree patch.