[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



Daniel P. Berrange wrote:
> 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>
>   

Ah, nice!  Thanks for the clarification.

Regards,
Jim


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