Re: [libvirt] [PATCH 1/2] libxl: expose multiple emulators per guest in the capabilities XML

On Mon, Apr 29, 2013 at 11:01:04PM -0600, Jim Fehlig wrote: > Daniel P. Berrange wrote: > >> On Mon, Apr 29, 2013 at 12:18:56PM -0600, Jim Fehlig wrote: >> >> >>> David Scott wrote: >>> >>> >>>> libxl allows users to choose between two standard emulators: >>>> 1. (default in xen-4.2): qemu "traditional" (aka "qemu-dm") >>>> 2. (default in xen-4.3): qemu "upstream" (aka "qemu-system-i386") >>>> >>>> The person who builds and packages xen gets to choose which >>>> emulators are built. We examine the filesystem for the emulators >>>> at runtime and expose them as separate "domains" within the same >>>> "guest" architecture. >>>> >>>> Signed-off-by: David Scott <dave.scott(a)eu.citrix.com&gt; >>>> --- >>>> src/libxl/libxl_conf.c | 87 ++++++++++++++++++++++++++++++++++++----------- >>>> 1 files changed, 66 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c >>>> index 7e0753a..472d116 100644 >>>> --- a/src/libxl/libxl_conf.c >>>> +++ b/src/libxl/libxl_conf.c >>>> @@ -29,6 +29,8 @@ >>>> #include <libxl.h> >>>> #include <sys/types.h> >>>> #include <sys/socket.h> >>>> +#include <sys/stat.h> >>>> +#include <unistd.h> >>>> >>>> #include "internal.h" >>>> #include "virlog.h" >>>> @@ -50,6 +52,28 @@ >>>> /* see xen-unstable.hg/xen/include/asm-x86/cpufeature.h */ >>>> #define LIBXL_X86_FEATURE_PAE_MASK 0x40 >>>> >>>> +enum emulator_type { >>>> + emulator_traditional = 0, >>>> + emulator_upstream = 1, >>>> + emulator_last = 2, >>>> + /* extend with specific qemu versions later */ >>>> +}; >>>> >>>> >>>> >>> Do you think this will need to be extended in the future? As >>> 'qemu-traditional' goes by way of the Dodo, this won't be needed right? >>> >>> >>> >>>> + >>>> +#define EMULATOR_LIB64 "/usr/lib64/xen/bin/" >>>> +#define EMULATOR_LIB32 "/usr/lib/xen/bin/" >>>> + >>>> +#define EMULATOR_TRADITIONAL "qemu-dm" >>>> +#define EMULATOR_UPSTREAM "qemu-system-i386" >>>> >>>> >>>> >>> I think this could be made quite a bit simpler with something like >>> >>> #define LIBXL_EMULATOR_TRADITIONAL_PATH LIBDIR "/xen/bin/qemu-dm" >>> #define LIBXL_EMULATOR_UPSTREAM_PATH LIBDIR "/xen/bin/qemu-sytstem-i386" >>> >>> >>> >>>> + >>>> +static const char* emulator_lib64_path [] = { >>>> + EMULATOR_LIB64 EMULATOR_TRADITIONAL, >>>> + EMULATOR_LIB64 EMULATOR_UPSTREAM, >>>> +}; >>>> + >>>> +static const char* emulator_lib32_path [] = { >>>> + EMULATOR_LIB32 EMULATOR_TRADITIONAL, >>>> + EMULATOR_LIB32 EMULATOR_UPSTREAM, >>>> +}; >>>> >>>> struct guest_arch { >>>> virArch arch; >>>> @@ -68,10 +92,11 @@ static virCapsPtr >>>> libxlBuildCapabilities(virArch hostarch, >>>> int host_pae, >>>> struct guest_arch *guest_archs, >>>> - int nr_guest_archs) >>>> + int nr_guest_archs, >>>> + int emulators_found[]) >>>> { >>>> virCapsPtr caps; >>>> - int i; >>>> + int i, j; >>>> >>>> if ((caps = virCapabilitiesNew(hostarch, 1, 1)) == NULL) >>>> goto no_memory; >>>> @@ -91,12 +116,8 @@ libxlBuildCapabilities(virArch hostarch, >>>> if ((guest = virCapabilitiesAddGuest(caps, >>>> guest_archs[i].hvm ? "hvm" : "xen", >>>> guest_archs[i].arch, >>>> - ((hostarch == VIR_ARCH_X86_64) ? >>>> - "/usr/lib64/xen/bin/qemu-dm" : >>>> - "/usr/lib/xen/bin/qemu-dm"), >>>> - (guest_archs[i].hvm ? >>>> - "/usr/lib/xen/boot/hvmloader" : >>>> - NULL), >>>> + NULL, >>>> + NULL, >>>> 1, >>>> machines)) == NULL) { >>>> virCapabilitiesFreeMachines(machines, 1); >>>> @@ -104,13 +125,21 @@ libxlBuildCapabilities(virArch hostarch, >>>> } >>>> machines = NULL; >>>> >>>> - if (virCapabilitiesAddGuestDomain(guest, >>>> - "xen", >>>> - NULL, >>>> - NULL, >>>> - 0, >>>> - NULL) == NULL) >>>> - goto no_memory; >>>> + for (j = 0; j < emulator_last; ++j) { >>>> + if (emulators_found[j] == -1) /* failure from stat(2) */ >>>> + continue; >>>> + if (virCapabilitiesAddGuestDomain(guest, >>>> + "xen", >>>> + ((hostarch == VIR_ARCH_X86_64) ? >>>> + emulator_lib64_path[j] : >>>> + emulator_lib32_path[j]), >>>> + (guest_archs[i].hvm ? >>>> + "/usr/lib/xen/boot/hvmloader" : >>>> + NULL), >>>> + 0, >>>> + NULL) == NULL) >>>> + goto no_memory; >>>> + } >>>> >>>> >>>> >>> and then just add the emulators here. E.g. >>> >>> if (virFileExists(LIBXL_EMULATOR_TRADITIONAL_PATH) { >>> if (virCapabilitiesAddGuestDomain(guest, >>> "xen", >>> LIBXL_EMULATOR_TRADITIONAL_PATH >>> (guest_archs[i].hvm ? >>> "/usr/lib/xen/boot/hvmloader" : >>> NULL), >>> 0, >>> NULL) == NULL) >>> goto no_memory; >>> } >>> if (virFileExists(LIBXL_EMULATOR_UPSTREAM_PATH) { >>> if (virCapabilitiesAddGuestDomain(guest, >>> "xen", >>> LIBXL_EMULATOR_UPSTREAM_PATH >>> (guest_archs[i].hvm ? >>> "/usr/lib/xen/boot/hvmloader" : >>> NULL), >>> 0, >>> NULL) == NULL) >>> goto no_memory; >>> } >>> >>> >> NB, for any single (arch, domain, os_type) triple, we should only >> report one <guest> in the capabilities XML. IIUC, your code above >> report cause us to have two entries for the same triple. >> > FYI, I advised David on this approach after observing the qemu driver > behavior with both qemu and kvm packages installed. On this system, the > capabilities for a x86-86, hvm guest contain > > <guest> > <os_type>hvm</os_type> > <arch name='x86_64'> > <wordsize>64</wordsize> > <emulator>/usr/bin/qemu-system-x86_64</emulator> > <machine>pc-1.1</machine> > <machine canonical='pc-1.1'>pc</machine> > <machine>pc-1.0</machine> > <machine>pc-0.15</machine> > <machine>pc-0.14</machine> > <machine>pc-0.13</machine> > <machine>pc-0.12</machine> > <machine>pc-0.11</machine> > <machine>pc-0.10</machine> > <machine>isapc</machine> > <domain type='qemu'> > </domain> > <domain type='kvm'> > <emulator>/usr/bin/qemu-kvm</emulator> > <machine>pc-1.1</machine> > <machine canonical='pc-1.1'>pc</machine> > <machine>pc-1.0</machine> > <machine>pc-0.15</machine> > <machine>pc-0.14</machine> > <machine>pc-0.13</machine> > <machine>pc-0.12</machine> > <machine>pc-0.11</machine> > <machine>pc-0.10</machine> > <machine>isapc</machine> > </domain> > </arch> > <features> > <cpuselection/> > <deviceboot/> > <acpi default='on' toggle='yes'/> > <apic default='on' toggle='no'/> > </features> > </guest> > > After a second look, the <domain type='kvm'> and <domain type='qemu'> > elements and their subelements aren't symmetrical. Seems <domain > type='qemu'> should include the emulator and supported machines, e.g. > No, there's no need for that. The semantics are that at the <arch> level, we should the default machines for that (ostype,arch) combination. There are then one oro more <domain> sub-elements. The sub-elements only need to include the <machine> information, *if* they have a dedicated binary for that machine type. So for example, if the main /usr/bin/qemu-system-x86_64 supported KVM and QEMU, then the above capabilities would be <arch name='x86_64'> <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <machine>pc-1.1</machine> <machine canonical='pc-1.1'>pc</machine> <machine>pc-1.0</machine> <machine>pc-0.15</machine> <machine>pc-0.14</machine> <machine>pc-0.13</machine> <machine>pc-0.12</machine> <machine>pc-0.11</machine> <machine>pc-0.10</machine> <machine>isapc</machine> <domain type='qemu'> </domain> <domain type='kvm'> </domain> </arch>