[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 eu citrix com>
> >>> ---
> >>>  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>


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]