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

[Douglas, William]

On Tue, May 11, 2021 at 02:05AM, Daniel P. Berrangé wrote:
> 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.

Lack of testing (and interest in fixing issues if there are any) for
a 32-bit kernel.

>>  * 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.

Not yet as far as I'm aware of inclusion or plans for inclusion in a
distro but that's something I can look at adding.

>> 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 ?

"CH" is the one I see used most frequently and what I've tried to
stick with in the code.

>> 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.

Okay, I'll drop the RPM changes in the next version.

>> 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.

Ah that's a good point, I'll add a check on host_machine.cpu_family
for 'x86_64'. Is that the right way to handle it?

>> +
>> +    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 ?

Oopsie that was just a refactor mistake. I'll take that out.

>> +/* 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

Oh great, I'll update these.

>> +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.

Oh indeed, I'll drop the "Cloud-Hypervisor" from that.

> 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.

Thanks so much for all the helpful advice on getting the driver to
be closer to libvirt's preferred architecture. I'm very much in
agreement on getting this in sooner. I'll send out an updated patch
shortly.