On 08/05/2018 03:48 PM, Marek Marczykowski-Górecki wrote:

Specifically, list sub-elements and where they can be used. In addition, describe supported machine types for Xen.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- docs/formatcaps.html.in | 20 +++++++++++++++++++- docs/formatdomain.html.in | 8 ++++---- 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/docs/formatcaps.html.in b/docs/formatcaps.html.in index 41a9a3a..34a74a9 100644 --- a/docs/formatcaps.html.in +++ b/docs/formatcaps.html.in @@ -91,7 +91,25 @@ </dd>

<dt><code>arch</code></dt> - <dd>This element brings some information on supported guest architecture.</dd> + <dd>This element brings some information on supported guest + architecture. Possible subelements are: + <dl> + <dt><code>wordsize</code></dt><dd>Size of CPU word in bits, for example 64.</dd> + <dt><code>emulator</code></dt><dd>Emulator (device model) path, for + use in <a href="formatdomain.html#elementEmulator">emulator</a> + element of domain XML.</dd> + <dt><code>loader</code></dt><dd>Loader path, for use in + <a href="formatdomain.html#elementLoader">loader</a> element of domain + XML.</dd> + <dt><code>machine</code></dt><dd>Machine type, for use in + <a href="formatdomain.html#attributeOSTypeMachine">machine</a> + attribute of os/type element in domain XML. For example Xen + supports <code>xenfv</code> for HVM or <code>xenpv</code> for + PV.</dd> + <dt><code>domain</code></dt><dd>Supported domain type, named by the + <code>type</code> attribute.</dd>

This last one wasn't very clear to me at first reading. What do you think of rewording it to the following? <dt><code>domain</code></dt><dd>The <code>type</code> attribute of this element specifies the type of hypervisor required to run the domain.</dd> Similar to the other descriptions it would be nice to have a "for use in" reference to the domain@type attribute. Regards, Jim

+ </dl> + </dd>

<dt><code>features</code></dt> <dd>This optional element encases possible features that can be used diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8043c7c..9a342b2 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -148,11 +148,11 @@ refers to an OS that supports the Xen 3 hypervisor guest ABI. There are also two optional attributes, <code>arch</code> specifying the CPU architecture to virtualization, - and <code>machine</code> referring to the machine - type. The <a href="formatcaps.html">Capabilities XML</a> + and <a id="attributeOSTypeMachine"><code>machine</code></a> referring + to the machine type. The <a href="formatcaps.html">Capabilities XML</a> provides details on allowed values for these. <span class="since">Since 0.0.1</span></dd> - <dt><code>loader</code></dt> + <dt><a id="elementLoader"><code>loader</code></a></dt> <dd>The optional <code>loader</code> tag refers to a firmware blob, which is specified by absolute path, used to assist the domain creation process. It is used by Xen @@ -2560,7 +2560,7 @@ ...</pre>

<dl> - <dt><code>emulator</code></dt> + <dt><a id="elementEmulator"><code>emulator</code></a></dt> <dd> The contents of the <code>emulator</code> element specify the fully qualified path to the device model emulator binary.

On 08/05/2018 03:48 PM, Marek Marczykowski-Górecki wrote: I added a short sentence to the commit message: Replace the long dead 'xenner' with 'xenfv'.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- docs/schemas/domaincommon.rng | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index ac04af5..eded1ca 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -340,7 +340,7 @@ <attribute name="machine"> <choice> <value>xenpv</value> - <value>xenner</value> + <value>xenfv</value> </choice> </attribute> </optional>

Reviewed-by: Jim Fehlig <jfehlig@suse.com> I've pushed this patch since it is essentially a bug fix independent of your PVH work. Regards, Jim

On 08/05/2018 03:48 PM, Marek Marczykowski-Górecki wrote:

Otherwise starting PVH guest will result in "arch_setup_bootlate: mapping shared_info failed (pfn=..., rc=-1, errno: 12): Internal error".

After this change the behavior is the same as in `xl`.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_conf.c | 10 +++++----- tests/libxlxml2domconfigdata/basic-pv.json | 1 + tests/libxlxml2domconfigdata/multiple-ip.json | 1 + 3 files changed, 7 insertions(+), 5 deletions(-)

Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 998029d..476bcbe 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -634,11 +634,6 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, return -1; } #endif - - /* Allow libxl to calculate shadow memory requirements */ - b_info->shadow_memkb = - libxl_get_required_shadow_memory(b_info->max_memkb, - b_info->max_vcpus); } else { /* * For compatibility with the legacy xen toolstack, default to pygrub @@ -692,6 +687,11 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, } }

+ /* Allow libxl to calculate shadow memory requirements */ + b_info->shadow_memkb = + libxl_get_required_shadow_memory(b_info->max_memkb, + b_info->max_vcpus); + return 0; }

diff --git a/tests/libxlxml2domconfigdata/basic-pv.json b/tests/libxlxml2domconfigdata/basic-pv.json index 0f846da..b71c3b0 100644 --- a/tests/libxlxml2domconfigdata/basic-pv.json +++ b/tests/libxlxml2domconfigdata/basic-pv.json @@ -14,6 +14,7 @@ ], "max_memkb": 524288, "target_memkb": 524288, + "shadow_memkb": 8192, "sched_params": {

}, diff --git a/tests/libxlxml2domconfigdata/multiple-ip.json b/tests/libxlxml2domconfigdata/multiple-ip.json index 80dca82..2db98b8 100644 --- a/tests/libxlxml2domconfigdata/multiple-ip.json +++ b/tests/libxlxml2domconfigdata/multiple-ip.json @@ -14,6 +14,7 @@ ], "max_memkb": 524288, "target_memkb": 524288, + "shadow_memkb": 8192, "sched_params": {

},

On 08/05/2018 03:48 PM, Marek Marczykowski-Górecki wrote:

If available, use b_info->nested_hvm instead of b_info->u.hvm.nested_hvm. This will make nested HVM config available also for PVH domains.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_conf.c | 13 ++++++++++++- tests/libxlxml2domconfigdata/fullvirt-cpuid.json | 2 +- tests/libxlxml2domconfigdata/vnuma-hvm.json | 2 +- 3 files changed, 14 insertions(+), 3 deletions(-)

Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 476bcbe..e2bfa2f 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -455,7 +455,18 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, } } } - libxl_defbool_set(&b_info->u.hvm.nested_hvm, hasHwVirt); +#ifdef LIBXL_HAVE_BUILDINFO_NESTED_HVM + libxl_defbool_set(&b_info->nested_hvm, hasHwVirt); +#else + if (hvm) { + libxl_defbool_set(&b_info->u.hvm.nested_hvm, hasHwVirt); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported nested HVM setting for %s machine on this Xen version"), + def->os.machine); + return -1; + } +#endif }

if (def->cpu && def->cpu->mode == VIR_CPU_MODE_CUSTOM) { diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid.json b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json index cdc8b98..d46b464 100644 --- a/tests/libxlxml2domconfigdata/fullvirt-cpuid.json +++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json @@ -21,11 +21,11 @@ ], "sched_params": { }, + "nested_hvm": "False", "type.hvm": { "pae": "True", "apic": "True", "acpi": "True", - "nested_hvm": "False", "nographic": "True", "vnc": { "enable": "False" diff --git a/tests/libxlxml2domconfigdata/vnuma-hvm.json b/tests/libxlxml2domconfigdata/vnuma-hvm.json index 3b2fc5f..02c10a9 100644 --- a/tests/libxlxml2domconfigdata/vnuma-hvm.json +++ b/tests/libxlxml2domconfigdata/vnuma-hvm.json @@ -109,11 +109,11 @@ "sched_params": {

}, + "nested_hvm": "True", "type.hvm": { "pae": "True", "apic": "True", "acpi": "True", - "nested_hvm": "True", "vga": { "kind": "cirrus" },

On Mon, Sep 10, 2018 at 03:44:33PM -0600, Jim Fehlig wrote:

On 08/05/2018 03:48 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 <marmarek@invisiblethingslab.com> --- docs/formatcaps.html.in | 4 +-- docs/schemas/domaincommon.rng | 1 +- src/libxl/libxl_capabilities.c | 23 ++++++++++++++++++-- src/libxl/libxl_conf.c | 41 ++++++++++++++++++++++++++++++----- src/libxl/libxl_driver.c | 6 +++-- 5 files changed, 64 insertions(+), 11 deletions(-)

diff --git a/docs/formatcaps.html.in b/docs/formatcaps.html.in index 34a74a9..1f17aa9 100644 --- a/docs/formatcaps.html.in +++ b/docs/formatcaps.html.in @@ -104,8 +104,8 @@ <dt><code>machine</code></dt><dd>Machine type, for use in <a href="formatdomain.html#attributeOSTypeMachine">machine</a> attribute of os/type element in domain XML. For example Xen - supports <code>xenfv</code> for HVM or <code>xenpv</code> for - PV.</dd> + supports <code>xenfv</code> for HVM, <code>xenpv</code> for + PV, or <code>xenpvh</code> for PVHv2.</dd> <dt><code>domain</code></dt><dd>Supported domain type, named by the <code>type</code> attribute.</dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index eded1ca..d32b0cb 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -341,6 +341,7 @@ <choice> <value>xenpv</value> <value>xenfv</value> + <value>xenpvh</value> </choice> </attribute> </optional> diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index 18596c7..e7b26f1 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -57,6 +57,7 @@ struct guest_arch { virArch arch; int bits; int hvm; + int pvh; int pae; int nonpae; int ia64_be; @@ -491,13 +492,29 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) guest_archs[i].nonpae = nonpae; if (ia64_be) guest_archs[i].ia64_be = ia64_be; + + /* On Xen >= 4.9 add PVH for each HVM guest, and do it only once */

I'm having problems understanding this. Do you mean add a PVH for each supported HVM arch, but exclude PAE? E.g. standard xen_caps on x86_64 contains

xen-3.0-x86_64 xen-3.0-x86_32p hvm-3.0-x86_32 hvm-3.0-x86_32p hvm-3.0-x86_64

Given these caps, should a PVH be added that corresponds to the hvm-3.0-x86_32 cap and another for the hvm-3.0-x86_64 cap, but the hvm-3.0-x86_32p cap excluded?

Yes, exactly. Setting PAE (or not) is possible only for HVM, but not PVH. It would be much better if Xen would report support for PVH explicitly...

...

+ if ((ver_info->xen_version_major > 4 || + (ver_info->xen_version_major == 4 && + ver_info->xen_version_minor >= 9)) && + 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].pvh = 1; + }

Without answers to the above questions, I can't really comment on this code. Regardless, since PVH is not advertised in xen_caps shouldn't it be added to guest_archs outside of the loop parsing xen_caps?

This works on assumption that if you have HVM and new enough Xen, then you have PVH. Just having new Xen isn't enough - for example the hardware may lack VT-x.

...

} } regfree(®ex); 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].hvm ? "xenfv" : + (guest_archs[i].pvh ? "xenpvh" : "xenpv")}; virCapsGuestMachinePtr *machines; if ((machines = virCapabilitiesAllocMachines(xen_machines, 1)) == NULL) @@ -557,7 +574,9 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) 1, 0) == NULL) return -1; + } + if (guest_archs[i].hvm || guest_archs[i].pvh) { if (virCapabilitiesAddGuestFeature(guest, "hap", 1, @@ -580,7 +599,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 f3da0ed..2df40ec 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -133,8 +133,11 @@ libxlMakeDomCreateInfo(libxl_ctx *ctx, libxl_domain_create_info_init(c_info); - if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { - c_info->type = LIBXL_DOMAIN_TYPE_HVM; + if (def->os.type == VIR_DOMAIN_OSTYPE_HVM || + (def->os.type == VIR_DOMAIN_OSTYPE_XEN && + STREQ(def->os.machine, "xenpvh"))) { + c_info->type = def->os.type == VIR_DOMAIN_OSTYPE_HVM ? + LIBXL_DOMAIN_TYPE_HVM : LIBXL_DOMAIN_TYPE_PVH; switch ((virTristateSwitch) def->features[VIR_DOMAIN_FEATURE_HAP]) { case VIR_TRISTATE_SWITCH_OFF: libxl_defbool_set(&c_info->hap, false); @@ -276,7 +279,8 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, virDomainClockDef clock = def->clock; libxl_ctx *ctx = cfg->ctx; libxl_domain_build_info *b_info = &d_config->b_info; - int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; + bool hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; + bool pvh = STREQ(def->os.machine, "xenpvh"); size_t i; size_t nusbdevice = 0; @@ -284,6 +288,8 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, if (hvm) libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM); + else if (pvh) + libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PVH); else libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV); @@ -375,7 +381,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, def->mem.cur_balloon = VIR_ROUND_UP(def->mem.cur_balloon, 1024); b_info->max_memkb = virDomainDefGetMemoryInitial(def); b_info->target_memkb = def->mem.cur_balloon; - if (hvm) { + if (hvm || pvh) { if (caps && def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) { bool hasHwVirt = false; @@ -647,6 +653,31 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, return -1; } #endif + } else if (pvh) { +#ifdef LIBXL_HAVE_BUILDINFO_KERNEL + if (VIR_STRDUP(b_info->cmdline, def->os.cmdline) < 0) + return -1; + if (VIR_STRDUP(b_info->kernel, def->os.kernel) < 0) + return -1; + if (VIR_STRDUP(b_info->ramdisk, def->os.initrd) < 0) + return -1; +#else + /* + * Shouldn't happen as LIBXL_HAVE_BUILDINFO_KERNEL is there since Xen + * 4.5, but PVHv2 since 4.9. + */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PVH guest type not supported")); +#endif

I guess this is needed else the build will fail on Xen < 4.5?

Yes, exactly.

Maybe it is time to bump the minimum supported Xen version to 4.6 :-). I say that a bit jokingly, but I did propose it a few months back.

IMO good idea, since Xen < 4.6 is not supported anymore.

...

+#ifdef LIBXL_HAVE_BUILDINFO_BOOTLOADER + if (VIR_STRDUP(b_info->bootloader, def->os.bootloader) < 0) + return -1; + if (def->os.bootloaderArgs) { + if (!(b_info->bootloader_args = + virStringSplit(def->os.bootloaderArgs, " \t\n", 0))) + return -1; + } +#endif } else { /* * For compatibility with the legacy xen toolstack, default to pygrub @@ -1230,7 +1261,7 @@ libxlMakeNic(virDomainDefPtr def, STRNEQ(l_nic->model, "netfront")) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("only model 'netfront' is supported for " - "Xen PV domains")); + "Xen PV(H) domains")); return -1; } if (VIR_STRDUP(x_nic->model, l_nic->model) < 0) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5a5e792..052a0da 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -6271,9 +6271,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 {

-- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?

On Mon, Sep 10, 2018 at 04:45:50PM -0600, Jim Fehlig wrote:

On 09/10/2018 04:02 PM, Marek Marczykowski-Górecki wrote:

...

On Mon, Sep 10, 2018 at 03:44:33PM -0600, Jim Fehlig wrote:

...

On 08/05/2018 03:48 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 <marmarek@invisiblethingslab.com> --- docs/formatcaps.html.in | 4 +-- docs/schemas/domaincommon.rng | 1 +- src/libxl/libxl_capabilities.c | 23 ++++++++++++++++++-- src/libxl/libxl_conf.c | 41 ++++++++++++++++++++++++++++++----- src/libxl/libxl_driver.c | 6 +++-- 5 files changed, 64 insertions(+), 11 deletions(-)

diff --git a/docs/formatcaps.html.in b/docs/formatcaps.html.in index 34a74a9..1f17aa9 100644 --- a/docs/formatcaps.html.in +++ b/docs/formatcaps.html.in @@ -104,8 +104,8 @@ <dt><code>machine</code></dt><dd>Machine type, for use in <a href="formatdomain.html#attributeOSTypeMachine">machine</a> attribute of os/type element in domain XML. For example Xen - supports <code>xenfv</code> for HVM or <code>xenpv</code> for - PV.</dd> + supports <code>xenfv</code> for HVM, <code>xenpv</code> for + PV, or <code>xenpvh</code> for PVHv2.</dd> <dt><code>domain</code></dt><dd>Supported domain type, named by the <code>type</code> attribute.</dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index eded1ca..d32b0cb 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -341,6 +341,7 @@ <choice> <value>xenpv</value> <value>xenfv</value> + <value>xenpvh</value> </choice> </attribute> </optional> diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index 18596c7..e7b26f1 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -57,6 +57,7 @@ struct guest_arch { virArch arch; int bits; int hvm; + int pvh; int pae; int nonpae; int ia64_be; @@ -491,13 +492,29 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) guest_archs[i].nonpae = nonpae; if (ia64_be) guest_archs[i].ia64_be = ia64_be; + + /* On Xen >= 4.9 add PVH for each HVM guest, and do it only once */

I'm having problems understanding this. Do you mean add a PVH for each supported HVM arch, but exclude PAE? E.g. standard xen_caps on x86_64 contains

xen-3.0-x86_64 xen-3.0-x86_32p hvm-3.0-x86_32 hvm-3.0-x86_32p hvm-3.0-x86_64

Given these caps, should a PVH be added that corresponds to the hvm-3.0-x86_32 cap and another for the hvm-3.0-x86_64 cap, but the hvm-3.0-x86_32p cap excluded?

Yes, exactly. Setting PAE (or not) is possible only for HVM, but not PVH.

It would be much better if Xen would report support for PVH explicitly...

...

...

+ if ((ver_info->xen_version_major > 4 || + (ver_info->xen_version_major == 4 && + ver_info->xen_version_minor >= 9)) && + hvm && i == nr_guest_archs-1) {

How about checking for hvm && !pae instead of i == nr_guest_archs-1?

At this time it should be ok. The i == nr_guest_archs-1 is based directly on if (i == nr_guest_archs) nr_guest_archs++ earlier up. So, it is indeed added only when given arch was added to guest_archs, regardless of what conditions were used for that. Maybe, use something like this instead: bool new_arch_added; if ((new_arch_added = (i == nr_guest_archs))) nr_guest_archs++ ... if (... hvm && new_arch_added) ?

...

...

...

+ 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].pvh = 1; + }

Without answers to the above questions, I can't really comment on this code. Regardless, since PVH is not advertised in xen_caps shouldn't it be added to guest_archs outside of the loop parsing xen_caps?

This works on assumption that if you have HVM and new enough Xen, then you have PVH. Just having new Xen isn't enough - for example the hardware may lack VT-x.

...

...

} } regfree(®ex); 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].hvm ? "xenfv" : + (guest_archs[i].pvh ? "xenpvh" : "xenpv")}; virCapsGuestMachinePtr *machines; if ((machines = virCapabilitiesAllocMachines(xen_machines, 1)) == NULL) @@ -557,7 +574,9 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) 1, 0) == NULL) return -1; + } + if (guest_archs[i].hvm || guest_archs[i].pvh) { if (virCapabilitiesAddGuestFeature(guest, "hap", 1, @@ -580,7 +599,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 f3da0ed..2df40ec 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -133,8 +133,11 @@ libxlMakeDomCreateInfo(libxl_ctx *ctx, libxl_domain_create_info_init(c_info); - if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { - c_info->type = LIBXL_DOMAIN_TYPE_HVM; + if (def->os.type == VIR_DOMAIN_OSTYPE_HVM || + (def->os.type == VIR_DOMAIN_OSTYPE_XEN && + STREQ(def->os.machine, "xenpvh"))) { + c_info->type = def->os.type == VIR_DOMAIN_OSTYPE_HVM ? + LIBXL_DOMAIN_TYPE_HVM : LIBXL_DOMAIN_TYPE_PVH; switch ((virTristateSwitch) def->features[VIR_DOMAIN_FEATURE_HAP]) { case VIR_TRISTATE_SWITCH_OFF: libxl_defbool_set(&c_info->hap, false); @@ -276,7 +279,8 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, virDomainClockDef clock = def->clock; libxl_ctx *ctx = cfg->ctx; libxl_domain_build_info *b_info = &d_config->b_info; - int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; + bool hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; + bool pvh = STREQ(def->os.machine, "xenpvh"); size_t i; size_t nusbdevice = 0; @@ -284,6 +288,8 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, if (hvm) libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM); + else if (pvh) + libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PVH); else libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV); @@ -375,7 +381,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, def->mem.cur_balloon = VIR_ROUND_UP(def->mem.cur_balloon, 1024); b_info->max_memkb = virDomainDefGetMemoryInitial(def); b_info->target_memkb = def->mem.cur_balloon; - if (hvm) { + if (hvm || pvh) { if (caps && def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) { bool hasHwVirt = false; @@ -647,6 +653,31 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, return -1; } #endif + } else if (pvh) { +#ifdef LIBXL_HAVE_BUILDINFO_KERNEL + if (VIR_STRDUP(b_info->cmdline, def->os.cmdline) < 0) + return -1; + if (VIR_STRDUP(b_info->kernel, def->os.kernel) < 0) + return -1; + if (VIR_STRDUP(b_info->ramdisk, def->os.initrd) < 0) + return -1; +#else + /* + * Shouldn't happen as LIBXL_HAVE_BUILDINFO_KERNEL is there since Xen + * 4.5, but PVHv2 since 4.9. + */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PVH guest type not supported")); +#endif

I guess this is needed else the build will fail on Xen < 4.5?

Yes, exactly.

...

Maybe it is time to bump the minimum supported Xen version to 4.6 :-). I say that a bit jokingly, but I did propose it a few months back.

IMO good idea, since Xen < 4.6 is not supported anymore.

BTW, here is a link to my earlier series to drop support for 4.4 and 4.5

https://www.redhat.com/archives/libvir-list/2018-March/msg01704.html

I'll rebase and resend that soon.

Regards, Jim

-- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?

On 8/5/18 3:48 PM, Marek Marczykowski-Górecki wrote:

builder="hvm" is deprecated since Xen 4.10, new syntax is type="hvm" (or type="pv", which is default). Since the old one is still supported, still use it when writing native config, so the config will work on older Xen too (and will also not complicate tests).

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- "type" option is the only syntax for specifying PVH guest, coming in next patch. --- src/xenconfig/xen_common.c | 18 +++++++++++++--- tests/xlconfigdata/test-fullvirt-type.cfg | 21 +++++++++++++++++++- tests/xlconfigdata/test-fullvirt-type.xml | 27 ++++++++++++++++++++++++- tests/xlconfigdata/test-paravirt-type.cfg | 13 ++++++++++++- tests/xlconfigdata/test-paravirt-type.xml | 25 ++++++++++++++++++++++- tests/xlconfigtest.c | 2 ++- 6 files changed, 103 insertions(+), 3 deletions(-) create mode 100644 tests/xlconfigdata/test-fullvirt-type.cfg create mode 100644 tests/xlconfigdata/test-fullvirt-type.xml create mode 100644 tests/xlconfigdata/test-paravirt-type.cfg create mode 100644 tests/xlconfigdata/test-paravirt-type.xml

diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index fdca984..c5d81d1 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -1088,9 +1088,21 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) if (xenConfigGetUUID(conf, "uuid", def->uuid) < 0) goto out;

- if ((xenConfigGetString(conf, "builder", &str, "linux") == 0) && - STREQ(str, "hvm")) - hvm = 1; + if (xenConfigGetString(conf, "type", &str, NULL) == 0 && str) { + if (STREQ(str, "pv")) + hvm = 0; + else if (STREQ(str, "hvm")) + hvm = 1; + else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("type %s is not supported"), str); + return -1; + }

Fails 'make syntax-check': src/xenconfig/xen_common.c:1096: else { maint.mk: if one side of if-else uses {}, both sides must use it Looks good otherwise. Regards, Jim

+ } else { + if ((xenConfigGetString(conf, "builder", &str, "linux") == 0) && + STREQ(str, "hvm")) + hvm = 1; + }

def->os.type = (hvm ? VIR_DOMAIN_OSTYPE_HVM : VIR_DOMAIN_OSTYPE_XEN);

diff --git a/tests/xlconfigdata/test-fullvirt-type.cfg b/tests/xlconfigdata/test-fullvirt-type.cfg new file mode 100644 index 0000000..f8ecc2e --- /dev/null +++ b/tests/xlconfigdata/test-fullvirt-type.cfg @@ -0,0 +1,21 @@ +name = "XenGuest2" +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809" +maxmem = 579 +memory = 394 +vcpus = 1 +pae = 1 +acpi = 1 +apic = 1 +viridian = 0 +rtc_timeoffset = 0 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +device_model = "/usr/lib/xen/bin/qemu-system-i386" +sdl = 0 +vnc = 0 +parallel = "none" +serial = "none" +type = "hvm" +boot = "d" diff --git a/tests/xlconfigdata/test-fullvirt-type.xml b/tests/xlconfigdata/test-fullvirt-type.xml new file mode 100644 index 0000000..da8e360 --- /dev/null +++ b/tests/xlconfigdata/test-fullvirt-type.xml @@ -0,0 +1,27 @@ +<domain type='xen'> + <name>XenGuest2</name> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>592896</memory> + <currentMemory unit='KiB'>403456</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='xenfv'>hvm</type> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='cdrom'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='variable' adjustment='0' basis='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/lib/xen/bin/qemu-system-i386</emulator> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='xen'/> + </devices> +</domain> diff --git a/tests/xlconfigdata/test-paravirt-type.cfg b/tests/xlconfigdata/test-paravirt-type.cfg new file mode 100644 index 0000000..078db99 --- /dev/null +++ b/tests/xlconfigdata/test-paravirt-type.cfg @@ -0,0 +1,13 @@ +name = "XenGuest2" +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809" +type = "pv" +maxmem = 579 +memory = 394 +vcpus = 1 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +kernel = "/tmp/vmlinuz" +ramdisk = "/tmp/initrd" +cmdline = "root=/dev/xvda1 console=hvc0" diff --git a/tests/xlconfigdata/test-paravirt-type.xml b/tests/xlconfigdata/test-paravirt-type.xml new file mode 100644 index 0000000..4357640 --- /dev/null +++ b/tests/xlconfigdata/test-paravirt-type.xml @@ -0,0 +1,25 @@ +<domain type='xen'> + <name>XenGuest2</name> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>592896</memory> + <currentMemory unit='KiB'>403456</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='xenpv'>linux</type> + <kernel>/tmp/vmlinuz</kernel> + <initrd>/tmp/initrd</initrd> + <cmdline>root=/dev/xvda1 console=hvc0</cmdline> + </os> + <clock offset='utc' adjustment='reset'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <console type='pty'> + <target type='xen' port='0'/> + </console> + <input type='mouse' bus='xen'/> + <input type='keyboard' bus='xen'/> + <memballoon model='xen'/> + </devices> +</domain> diff --git a/tests/xlconfigtest.c b/tests/xlconfigtest.c index 36f7699..ad6a964 100644 --- a/tests/xlconfigtest.c +++ b/tests/xlconfigtest.c @@ -281,6 +281,8 @@ mymain(void) DO_TEST_FORMAT("paravirt-cmdline-extra-root", false); DO_TEST_FORMAT("paravirt-cmdline-bogus-extra-root", false); DO_TEST("rbd-multihost-noauth"); + DO_TEST_FORMAT("paravirt-type", false); + DO_TEST_FORMAT("fullvirt-type", false);

#ifdef LIBXL_HAVE_DEVICE_CHANNEL DO_TEST("channel-pty");

On 8/5/18 3:48 PM, Marek Marczykowski-Górecki wrote:

Handle PVH domain type in both directions (xen-xl->xml, xml->xen-xl). And add a test for it.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Does domain_conf.c (virDomainDefFormatInternal) still need to silently convert VIR_DOMAIN_OSTYPE_XEN to VIR_DOMAIN_OSTYPE_LINUX? In case of PVH, VIR_DOMAIN_OSTYPE_LINUX was never reported as a valid option...

I'd prefer that we switch to formatting type as VIR_DOMAIN_OSTYPE_XEN and indicating PV vs PVH with the machine attribute. For back compat we'd need to continue accepting <type>linux</type> when parsing XML. Other than a lot of changes to test suite data files, am I overlooking compatibility problems with such a change?

--- src/xenconfig/xen_common.c | 17 ++++++++++++----- src/xenconfig/xen_xl.c | 5 +++++ tests/testutilsxen.c | 3 ++- tests/xlconfigdata/test-pvh-type.cfg | 13 +++++++++++++ tests/xlconfigdata/test-pvh-type.xml | 25 +++++++++++++++++++++++++ tests/xlconfigtest.c | 1 + 6 files changed, 58 insertions(+), 6 deletions(-) create mode 100644 tests/xlconfigdata/test-pvh-type.cfg create mode 100644 tests/xlconfigdata/test-pvh-type.xml

diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index c5d81d1..3c120a0 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -1081,6 +1081,7 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) virCapsDomainDataPtr capsdata = NULL; const char *str; int hvm = 0, ret = -1; + const char *machine = "xenpv";

if (xenConfigCopyString(conf, "name", &def->name) < 0) goto out; @@ -1089,25 +1090,31 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) goto out;

if (xenConfigGetString(conf, "type", &str, NULL) == 0 && str) { - if (STREQ(str, "pv")) + if (STREQ(str, "pv")) { hvm = 0; - else if (STREQ(str, "hvm")) + } else if (STREQ(str, "pvh")) { + hvm = 0; + machine = "xenpvh"; + } else if (STREQ(str, "hvm")) { hvm = 1; - else { + machine = "xenfv"; + } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("type %s is not supported"), str); return -1; }

Ah, I see you fixed up the braces in this patch. Regardless, 'make syntax-check' should pass with each patch. Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim

} else { if ((xenConfigGetString(conf, "builder", &str, "linux") == 0) && - STREQ(str, "hvm")) + STREQ(str, "hvm")) { hvm = 1; + machine = "xenfv"; + } }

def->os.type = (hvm ? VIR_DOMAIN_OSTYPE_HVM : VIR_DOMAIN_OSTYPE_XEN);

if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type, - VIR_ARCH_NONE, def->virtType, NULL, NULL))) + VIR_ARCH_NONE, def->virtType, NULL, machine))) goto out;

def->os.arch = capsdata->arch; diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 19b6604..f1853bd 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -1285,6 +1285,11 @@ xenFormatXLOS(virConfPtr conf, virDomainDefPtr def)

/* XXX floppy disks */ } else { + if (STREQ(def->os.machine, "xenpvh")) { + if (xenConfigSetString(conf, "type", "pvh") < 0) + return -1; + } + if (def->os.bootloader && xenConfigSetString(conf, "bootloader", def->os.bootloader) < 0) return -1; diff --git a/tests/testutilsxen.c b/tests/testutilsxen.c index c08ec4a..04f8920 100644 --- a/tests/testutilsxen.c +++ b/tests/testutilsxen.c @@ -18,7 +18,8 @@ testXLInitCaps(void) "xenfv" }; static const char *const xen_machines[] = { - "xenpv" + "xenpv", + "xenpvh" };

if ((caps = virCapabilitiesNew(virArchFromHost(), diff --git a/tests/xlconfigdata/test-pvh-type.cfg b/tests/xlconfigdata/test-pvh-type.cfg new file mode 100644 index 0000000..2493535 --- /dev/null +++ b/tests/xlconfigdata/test-pvh-type.cfg @@ -0,0 +1,13 @@ +name = "XenGuest2" +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809" +maxmem = 579 +memory = 394 +vcpus = 1 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +type = "pvh" +kernel = "/tmp/vmlinuz" +ramdisk = "/tmp/initrd" +cmdline = "root=/dev/xvda1 console=hvc0" diff --git a/tests/xlconfigdata/test-pvh-type.xml b/tests/xlconfigdata/test-pvh-type.xml new file mode 100644 index 0000000..96b0e7a --- /dev/null +++ b/tests/xlconfigdata/test-pvh-type.xml @@ -0,0 +1,25 @@ +<domain type='xen'> + <name>XenGuest2</name> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>592896</memory> + <currentMemory unit='KiB'>403456</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='xenpvh'>linux</type> + <kernel>/tmp/vmlinuz</kernel> + <initrd>/tmp/initrd</initrd> + <cmdline>root=/dev/xvda1 console=hvc0</cmdline> + </os> + <clock offset='utc' adjustment='reset'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <console type='pty'> + <target type='xen' port='0'/> + </console> + <input type='mouse' bus='xen'/> + <input type='keyboard' bus='xen'/> + <memballoon model='xen'/> + </devices> +</domain> diff --git a/tests/xlconfigtest.c b/tests/xlconfigtest.c index ad6a964..b9cf939 100644 --- a/tests/xlconfigtest.c +++ b/tests/xlconfigtest.c @@ -283,6 +283,7 @@ mymain(void) DO_TEST("rbd-multihost-noauth"); DO_TEST_FORMAT("paravirt-type", false); DO_TEST_FORMAT("fullvirt-type", false); + DO_TEST("pvh-type");

#ifdef LIBXL_HAVE_DEVICE_CHANNEL DO_TEST("channel-pty");