On 09/16/2016 05:14 PM, Marek Marczykowski-Górecki wrote:
> On Fri, Sep 16, 2016 at 04:39:23PM -0600, Jim Fehlig wrote:
>> On 08/05/2016 12:05 PM, Marek Marczykowski-Górecki wrote:
>>> Since this is something between PV and HVM, it makes sense to put the
>>> setting in place where domain type is specified.
>>> To enable it, use <os><type machine="xenpvh">...</type></os>. It is
>>> also included in capabilities.xml, for every supported HVM guest type - it
>>> doesn't seems to be any other requirement (besides new enough Xen).
>>>
>>> Signed-off-by: Marek Marczykowski-Górecki <marma...@invisiblethingslab.com>
>>> ---
>>>  src/libxl/libxl_capabilities.c | 40 
>>> +++++++++++++++++++++++++++++++---------
>>>  src/libxl/libxl_conf.c         |  2 ++
>>>  src/libxl/libxl_driver.c       |  6 ++++--
>>>  3 files changed, 37 insertions(+), 11 deletions(-)
>> I didn't investigate, but this patch did not apply cleanly.
>>
>> Does 'xenpvh' need to be added to docs/schema/domaincommon.rng? The schema 
>> looks
>> dated anyhow since it currently contains 'xenpv' and 'xenner'. And perhaps 
>> this
>> value should be added to docs/formatdomain.html.in, along with a sentence 
>> about
>> the possible values for Xen machines.
> After further evaluation[1], PVHv1 is not the thing I wanted here. And
> PVHv2 is going to be significantly different. While this patch do work
> for me, I'm not going to spend more time on PVHv1.

Ok. I'm not a fan of supporting PVHv1 in libvirt anyhow. It came and went before
anyone even used it :-).

>
>>> diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c
>>> index 0145116..c443353 100644
>>> --- a/src/libxl/libxl_capabilities.c
>>> +++ b/src/libxl/libxl_capabilities.c
>>> @@ -45,11 +45,16 @@ VIR_LOG_INIT("libxl.libxl_capabilities");
>>>  /* see xen-unstable.hg/xen/include/asm-x86/cpufeature.h */
>>>  #define LIBXL_X86_FEATURE_PAE_MASK 0x40
>>>  
>>> +enum machine_type {
>>> +    machine_hvm,
>>> +    machine_pvh,
>>> +    machine_pv,
>>> +};
>>>  
>>>  struct guest_arch {
>>>      virArch arch;
>>>      int bits;
>>> -    int hvm;
>>> +    enum machine_type machine;
>>>      int pae;
>>>      int nonpae;
>>>      int ia64_be;
>>> @@ -296,7 +301,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
>>>              /* Search for existing matching (model,hvm) tuple */
>>>              for (i = 0; i < nr_guest_archs; i++) {
>>>                  if ((guest_archs[i].arch == arch) &&
>>> -                    guest_archs[i].hvm == hvm)
>>> +                    guest_archs[i].machine == (hvm ? machine_hvm : 
>>> machine_pv))
>>>                      break;
>>>              }
>>>  
>>> @@ -308,7 +313,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
>>>                  nr_guest_archs++;
>>>  
>>>              guest_archs[i].arch = arch;
>>> -            guest_archs[i].hvm = hvm;
>>> +            guest_archs[i].machine = hvm ? machine_hvm : machine_pv;
>>>  
>>>              /* Careful not to overwrite a previous positive
>>>                 setting with a negative one here - some archs
>>> @@ -320,23 +325,40 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
>>>                  guest_archs[i].nonpae = nonpae;
>>>              if (ia64_be)
>>>                  guest_archs[i].ia64_be = ia64_be;
>>> +
>>> +            /* On Xen >= 4.4 add PVH for each HVM guest, and do it only 
>>> once */
>>> +            if ((ver_info->xen_version_major > 4 ||
>>> +                    (ver_info->xen_version_major == 4 &&
>>> +                     ver_info->xen_version_minor >= 4)) &&
>>> +                    hvm && i == nr_guest_archs-1) {
>>> +                i = nr_guest_archs;
>>> +                /* Too many arch flavours - highly unlikely ! */
>>> +                if (i >= ARRAY_CARDINALITY(guest_archs))
>>> +                    continue;
>>> +                nr_guest_archs++;
>>> +                guest_archs[i].arch = arch;
>>> +                guest_archs[i].machine = machine_pvh;
>>> +            }
>>>          }
>>>      }
>>>      regfree(&regex);
>>>  
>>>      for (i = 0; i < nr_guest_archs; ++i) {
>>>          virCapsGuestPtr guest;
>>> -        char const *const xen_machines[] = {guest_archs[i].hvm ? "xenfv" : 
>>> "xenpv"};
>>> +        char const *const xen_machines[] = {
>>> +            guest_archs[i].machine == machine_hvm ? "xenfv" :
>>> +                (guest_archs[i].machine == machine_pvh ? "xenpvh" : 
>>> "xenpv")};
>>>          virCapsGuestMachinePtr *machines;
>>>  
>>>          if ((machines = virCapabilitiesAllocMachines(xen_machines, 1)) == 
>>> NULL)
>>>              return -1;
>>>  
>>>          if ((guest = virCapabilitiesAddGuest(caps,
>>> -                                             guest_archs[i].hvm ? 
>>> VIR_DOMAIN_OSTYPE_HVM : VIR_DOMAIN_OSTYPE_XEN,
>>> +                                             guest_archs[i].machine == 
>>> machine_hvm ?
>>> +                                              VIR_DOMAIN_OSTYPE_HVM : 
>>> VIR_DOMAIN_OSTYPE_XEN,
>> Is a new VIR_DOMAIN_OSTYPE_XENPVH needed?
> Not sure about this. Wouldn't that require adding `os.type ==
> VIR_DOMAIN_OSTYPE_XEN || os.type == VIR_DOMAIN_OSTYPE_XENPVH` in a lot
> of places? If actual settings are mostly the same, I don't see any
> reason for introducing such value.

As long as PVHv2/HVMlite is just another impl of an existing machine type, I 
agree.

>
>>>                                               guest_archs[i].arch,
>>>                                               LIBXL_EXECBIN_DIR 
>>> "/qemu-system-i386",
>>> -                                             (guest_archs[i].hvm ?
>>> +                                             (guest_archs[i].machine == 
>>> machine_hvm ?
>>>                                                LIBXL_FIRMWARE_DIR 
>>> "/hvmloader" :
>>>                                                NULL),
>>>                                               1,
>>> @@ -375,7 +397,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
>>>                                             0) == NULL)
>>>              return -1;
>>>  
>>> -        if (guest_archs[i].hvm) {
>>> +        if (guest_archs[i].machine != machine_pv) {
>>>              if (virCapabilitiesAddGuestFeature(guest,
>>>                                                 "acpi",
>>>                                                 1,
>>> @@ -390,7 +412,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
>>>              if (virCapabilitiesAddGuestFeature(guest,
>>>                                                 "hap",
>>>                                                 1,
>>> -                                               1) == NULL)
>>> +                                               guest_archs[i].machine == 
>>> machine_hvm) == NULL)
>>>                  return -1;
>>>          }
>>>      }
>>> @@ -409,7 +431,7 @@ libxlMakeDomainOSCaps(const char *machine,
>>>  
>>>      os->supported = true;
>>>  
>>> -    if (STREQ(machine, "xenpv"))
>>> +    if (STREQ(machine, "xenpv") || STREQ(machine, "xenpvh"))
>>>          return 0;
>>>  
>>>      capsLoader->supported = true;
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index 5202ca1..aa06586 100644
>>> --- a/src/libxl/libxl_conf.c
>>> +++ b/src/libxl/libxl_conf.c
>>> @@ -173,6 +173,8 @@ libxlMakeDomCreateInfo(libxl_ctx *ctx,
>>>          }
>>>      } else {
>>>          c_info->type = LIBXL_DOMAIN_TYPE_PV;
>>> +        if (STREQ(def->os.machine, "xenpvh"))
>>> +            libxl_defbool_set(&c_info->pvh, true);
>> I assume this won't change with HVMlite, aka pvh2?
> It will, unfortunately. HVMlite is enabled by setting device model to
> none.
>
>>>      }
>>>  
>>>      if (VIR_STRDUP(c_info->name, def->name) < 0)
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index 4957072..fa58346 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -6321,9 +6321,11 @@ libxlConnectGetDomainCapabilities(virConnectPtr conn,
>>>          emulatorbin = "/usr/bin/qemu-system-x86_64";
>>>  
>>>      if (machine) {
>>> -        if (STRNEQ(machine, "xenpv") && STRNEQ(machine, "xenfv")) {
>>> +        if (STRNEQ(machine, "xenpv") &&
>>> +                STRNEQ(machine, "xenpvh") &&
>>> +                STRNEQ(machine, "xenfv")) {
>>>              virReportError(VIR_ERR_INVALID_ARG, "%s",
>>> -                           _("Xen only supports 'xenpv' and 'xenfv' 
>>> machines"));
>>> +                           _("Xen only supports 'xenpv', 'xenpvh' and 
>>> 'xenfv' machines"));
>>>              goto cleanup;
>>>          }
>>>      } else {
>> WRT domain capabilities, should pvh be treated like pv? I.e. do they both 
>> have
>> the same max vcpus, etc?
> Yes, PVH behave like PV. But PVHv2 like HVM.

Ok, that matches my understanding of PVHv2. It is an alternative HVM impl.

Regards,
Jim

>
>> Also, supporting a new knob in the XML usually means supporting conversion of
>> that knob to xl.cfg. Can you add domXML <-> xl.cfg conversion for pvh? And a
>> test case for the conversion too please?
> I'll add this for PVHv2...
>
> [1] http://markmail.org/message/c7o7qsc3chkigdzv
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to