On 9/10/18 7:10 PM, Marek Marczykowski-Górecki wrote:
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.
Right. This function is a little intense. I had to look at it a third time to see i == nr_guest_archs-1 also means we are adding a new entry in guest_archs :-).
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)
Along with an improved comment, I think this will help me remember what's going on in this code in the event I ever find time to refactor it. What do you think of the below diff? Regards, Jim diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index e7b26f12d8..446fa4ccf9 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -440,6 +440,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) int hvm = STRPREFIX(&token[subs[1].rm_so], "hvm"); virArch arch; int pae = 0, nonpae = 0, ia64_be = 0; + bool new_arch_added = false; if (STRPREFIX(&token[subs[2].rm_so], "x86_32")) { arch = VIR_ARCH_I686; @@ -476,8 +477,10 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) if (i >= ARRAY_CARDINALITY(guest_archs)) continue; /* Didn't find a match, so create a new one */ - if (i == nr_guest_archs) + if (i == nr_guest_archs) { nr_guest_archs++; + new_arch_added = true; + } guest_archs[i].arch = arch; guest_archs[i].hvm = hvm; @@ -493,18 +496,23 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) 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 */ + /* + * Xen 4.9 introduced support for the PVH guest type, which + * requires hardware virtualization support similar to the + * HVM guest type. Add a PVH guest type for each new HVM + * guest type. + */ 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) { + hvm && new_arch_added) { i = nr_guest_archs; - /* Too many arch flavours - highly unlikely ! */ - if (i >= ARRAY_CARDINALITY(guest_archs)) + /* Ensure we have not exhausted the guest_archs array */ + if (nr_guest_archs >= ARRAY_CARDINALITY(guest_archs)) continue; + guest_archs[nr_guest_archs].arch = arch; + guest_archs[nr_guest_archs].pvh = 1; nr_guest_archs++; - guest_archs[i].arch = arch; - guest_archs[i].pvh = 1; } } }