Re: [libvirt] [PATCH 07/10] libxl: add support for PVH

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(a)invisiblethingslab.com&gt; >>>> --- >>>> 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)