[PATCH] libxl: use b_info->{acpi,acpi} when available

Marek Marczykowski-Górecki posted 1 patch 2 weeks ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200910041801.1016202-1-marmarek@invisiblethingslab.com
src/libxl/libxl_conf.c                              | 13 +++++++++++++
tests/libxlxml2domconfigdata/basic-hvm.json         |  4 ++--
tests/libxlxml2domconfigdata/cpu-shares-hvm.json    |  4 ++--
.../libxlxml2domconfigdata/fullvirt-acpi-slic.json  |  4 ++--
.../fullvirt-cpuid-legacy-nest.json                 |  4 ++--
tests/libxlxml2domconfigdata/fullvirt-cpuid.json    |  4 ++--
.../max-eventchannels-hvm.json                      |  4 ++--
tests/libxlxml2domconfigdata/max-gntframes-hvm.json |  4 ++--
tests/libxlxml2domconfigdata/moredevs-hvm.json      |  4 ++--
.../libxlxml2domconfigdata/variable-clock-hvm.json  |  4 ++--
.../vnuma-hvm-legacy-nest.json                      |  4 ++--
tests/libxlxml2domconfigdata/vnuma-hvm.json         |  4 ++--
12 files changed, 35 insertions(+), 22 deletions(-)

[PATCH] libxl: use b_info->{acpi,acpi} when available

Posted by Marek Marczykowski-Górecki 2 weeks ago
b_info->u.hvm.{acpi,apic} are deprecated. But also, on recent libxl
version (4.14) the old one seems to be broken. While libxl part should
be fixed too, update the usage here and at some point drop support for
the old version.
b_info->acpi was added in Xen 4.8
b_info->apic was added in Xen 4.10
Xen 4.10 is the oldest version that still has security support (until
December 2020).

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 src/libxl/libxl_conf.c                              | 13 +++++++++++++
 tests/libxlxml2domconfigdata/basic-hvm.json         |  4 ++--
 tests/libxlxml2domconfigdata/cpu-shares-hvm.json    |  4 ++--
 .../libxlxml2domconfigdata/fullvirt-acpi-slic.json  |  4 ++--
 .../fullvirt-cpuid-legacy-nest.json                 |  4 ++--
 tests/libxlxml2domconfigdata/fullvirt-cpuid.json    |  4 ++--
 .../max-eventchannels-hvm.json                      |  4 ++--
 tests/libxlxml2domconfigdata/max-gntframes-hvm.json |  4 ++--
 tests/libxlxml2domconfigdata/moredevs-hvm.json      |  4 ++--
 .../libxlxml2domconfigdata/variable-clock-hvm.json  |  4 ++--
 .../vnuma-hvm-legacy-nest.json                      |  4 ++--
 tests/libxlxml2domconfigdata/vnuma-hvm.json         |  4 ++--
 12 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index befd5eb965..5b729a019a 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -508,12 +508,25 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
         libxl_defbool_set(&b_info->u.hvm.pae,
                           def->features[VIR_DOMAIN_FEATURE_PAE] ==
                           VIR_TRISTATE_SWITCH_ON);
+#ifdef LIBXL_HAVE_BUILDINFO_APIC
+        libxl_defbool_set(&b_info->apic,
+                          def->features[VIR_DOMAIN_FEATURE_APIC] ==
+                          VIR_TRISTATE_SWITCH_ON);
+        /*
+         * Strictly speaking b_info->acpi was introduced earlier (Xen 4.8), but
+         * there is no separate #define in libxl.h.
+         */
+        libxl_defbool_set(&b_info->acpi,
+                          def->features[VIR_DOMAIN_FEATURE_ACPI] ==
+                          VIR_TRISTATE_SWITCH_ON);
+#else
         libxl_defbool_set(&b_info->u.hvm.apic,
                           def->features[VIR_DOMAIN_FEATURE_APIC] ==
                           VIR_TRISTATE_SWITCH_ON);
         libxl_defbool_set(&b_info->u.hvm.acpi,
                           def->features[VIR_DOMAIN_FEATURE_ACPI] ==
                           VIR_TRISTATE_SWITCH_ON);
+#endif
 
         /* copy SLIC table path to acpi_firmware */
         if (def->os.slic_table)
diff --git a/tests/libxlxml2domconfigdata/basic-hvm.json b/tests/libxlxml2domconfigdata/basic-hvm.json
index ccd5853854..87f8cb7d8a 100644
--- a/tests/libxlxml2domconfigdata/basic-hvm.json
+++ b/tests/libxlxml2domconfigdata/basic-hvm.json
@@ -21,10 +21,10 @@
         "sched_params": {
 
         },
+        "apic": "True",
+        "acpi": "True",
         "type.hvm": {
             "pae": "True",
-            "apic": "True",
-            "acpi": "True",
             "vga": {
                 "kind": "cirrus"
             },
diff --git a/tests/libxlxml2domconfigdata/cpu-shares-hvm.json b/tests/libxlxml2domconfigdata/cpu-shares-hvm.json
index 2e647eada2..2aa97e88c5 100644
--- a/tests/libxlxml2domconfigdata/cpu-shares-hvm.json
+++ b/tests/libxlxml2domconfigdata/cpu-shares-hvm.json
@@ -21,10 +21,10 @@
         "sched_params": {
             "weight": 1500
         },
+        "apic": "True",
+        "acpi": "True",
         "type.hvm": {
             "pae": "True",
-            "apic": "True",
-            "acpi": "True",
             "vga": {
                 "kind": "cirrus"
             },
diff --git a/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.json b/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.json
index f16b4a971a..a2d46797aa 100644
--- a/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.json
+++ b/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.json
@@ -14,10 +14,10 @@
         "shadow_memkb": 5656,
         "sched_params": {
         },
+        "apic": "True",
+        "acpi": "True",
         "type.hvm": {
             "pae": "True",
-            "apic": "True",
-            "acpi": "True",
             "acpi_firmware": "/path/to/slic.dat",
             "nographic": "True",
             "vga": {
diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid-legacy-nest.json b/tests/libxlxml2domconfigdata/fullvirt-cpuid-legacy-nest.json
index 0944476151..6290655c20 100644
--- a/tests/libxlxml2domconfigdata/fullvirt-cpuid-legacy-nest.json
+++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid-legacy-nest.json
@@ -21,10 +21,10 @@
         ],
         "sched_params": {
         },
+        "apic": "True",
+        "acpi": "True",
         "type.hvm": {
             "pae": "True",
-            "apic": "True",
-            "acpi": "True",
             "nested_hvm": "False",
             "nographic": "True",
             "vga": {
diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid.json b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json
index ddc423bca7..811a4f0ac7 100644
--- a/tests/libxlxml2domconfigdata/fullvirt-cpuid.json
+++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json
@@ -22,10 +22,10 @@
         "sched_params": {
         },
         "nested_hvm": "False",
+        "apic": "True",
+        "acpi": "True",
         "type.hvm": {
             "pae": "True",
-            "apic": "True",
-            "acpi": "True",
             "nographic": "True",
             "vga": {
                 "kind": "none"
diff --git a/tests/libxlxml2domconfigdata/max-eventchannels-hvm.json b/tests/libxlxml2domconfigdata/max-eventchannels-hvm.json
index d5b351beb5..4a5b0ca65f 100644
--- a/tests/libxlxml2domconfigdata/max-eventchannels-hvm.json
+++ b/tests/libxlxml2domconfigdata/max-eventchannels-hvm.json
@@ -22,10 +22,10 @@
         "sched_params": {
 
         },
+        "apic": "True",
+        "acpi": "True",
         "type.hvm": {
             "pae": "True",
-            "apic": "True",
-            "acpi": "True",
             "vga": {
                 "kind": "cirrus"
             },
diff --git a/tests/libxlxml2domconfigdata/max-gntframes-hvm.json b/tests/libxlxml2domconfigdata/max-gntframes-hvm.json
index ef602f09fc..2883d057ff 100644
--- a/tests/libxlxml2domconfigdata/max-gntframes-hvm.json
+++ b/tests/libxlxml2domconfigdata/max-gntframes-hvm.json
@@ -22,10 +22,10 @@
         "sched_params": {
 
         },
+        "apic": "True",
+        "acpi": "True",
         "type.hvm": {
             "pae": "True",
-            "apic": "True",
-            "acpi": "True",
             "vga": {
                 "kind": "cirrus"
             },
diff --git a/tests/libxlxml2domconfigdata/moredevs-hvm.json b/tests/libxlxml2domconfigdata/moredevs-hvm.json
index 474aa2cef6..58cf32a8d4 100644
--- a/tests/libxlxml2domconfigdata/moredevs-hvm.json
+++ b/tests/libxlxml2domconfigdata/moredevs-hvm.json
@@ -23,10 +23,10 @@
         "sched_params": {
 
         },
+        "apic": "True",
+        "acpi": "True",
         "type.hvm": {
             "pae": "True",
-            "apic": "True",
-            "acpi": "True",
             "hpet": "True",
             "vga": {
                 "kind": "cirrus"
diff --git a/tests/libxlxml2domconfigdata/variable-clock-hvm.json b/tests/libxlxml2domconfigdata/variable-clock-hvm.json
index 69be9c64cb..9a25d51da2 100644
--- a/tests/libxlxml2domconfigdata/variable-clock-hvm.json
+++ b/tests/libxlxml2domconfigdata/variable-clock-hvm.json
@@ -23,10 +23,10 @@
         "sched_params": {
 
         },
+        "apic": "True",
+        "acpi": "True",
         "type.hvm": {
             "pae": "True",
-            "apic": "True",
-            "acpi": "True",
             "vga": {
                 "kind": "cirrus"
             },
diff --git a/tests/libxlxml2domconfigdata/vnuma-hvm-legacy-nest.json b/tests/libxlxml2domconfigdata/vnuma-hvm-legacy-nest.json
index 3b2fc5f40f..6cda8d0252 100644
--- a/tests/libxlxml2domconfigdata/vnuma-hvm-legacy-nest.json
+++ b/tests/libxlxml2domconfigdata/vnuma-hvm-legacy-nest.json
@@ -109,10 +109,10 @@
         "sched_params": {
 
         },
+        "apic": "True",
+        "acpi": "True",
         "type.hvm": {
             "pae": "True",
-            "apic": "True",
-            "acpi": "True",
             "nested_hvm": "True",
             "vga": {
                 "kind": "cirrus"
diff --git a/tests/libxlxml2domconfigdata/vnuma-hvm.json b/tests/libxlxml2domconfigdata/vnuma-hvm.json
index 02c10a9deb..f578ccd3d3 100644
--- a/tests/libxlxml2domconfigdata/vnuma-hvm.json
+++ b/tests/libxlxml2domconfigdata/vnuma-hvm.json
@@ -110,10 +110,10 @@
 
         },
         "nested_hvm": "True",
+        "apic": "True",
+        "acpi": "True",
         "type.hvm": {
             "pae": "True",
-            "apic": "True",
-            "acpi": "True",
             "vga": {
                 "kind": "cirrus"
             },
-- 
2.25.4

Re: [PATCH] libxl: use b_info->{acpi,acpi} when available

Posted by Michal Privoznik 1 week ago
On 9/10/20 6:18 AM, Marek Marczykowski-Górecki wrote:
> b_info->u.hvm.{acpi,apic} are deprecated. But also, on recent libxl
> version (4.14) the old one seems to be broken. While libxl part should
> be fixed too, update the usage here and at some point drop support for
> the old version.
> b_info->acpi was added in Xen 4.8
> b_info->apic was added in Xen 4.10
> Xen 4.10 is the oldest version that still has security support (until
> December 2020).
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>   src/libxl/libxl_conf.c                              | 13 +++++++++++++
>   tests/libxlxml2domconfigdata/basic-hvm.json         |  4 ++--
>   tests/libxlxml2domconfigdata/cpu-shares-hvm.json    |  4 ++--
>   .../libxlxml2domconfigdata/fullvirt-acpi-slic.json  |  4 ++--
>   .../fullvirt-cpuid-legacy-nest.json                 |  4 ++--
>   tests/libxlxml2domconfigdata/fullvirt-cpuid.json    |  4 ++--
>   .../max-eventchannels-hvm.json                      |  4 ++--
>   tests/libxlxml2domconfigdata/max-gntframes-hvm.json |  4 ++--
>   tests/libxlxml2domconfigdata/moredevs-hvm.json      |  4 ++--
>   .../libxlxml2domconfigdata/variable-clock-hvm.json  |  4 ++--
>   .../vnuma-hvm-legacy-nest.json                      |  4 ++--
>   tests/libxlxml2domconfigdata/vnuma-hvm.json         |  4 ++--
>   12 files changed, 35 insertions(+), 22 deletions(-)

This looks good to me.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

I'll wait a bit with pushing it though in case Jim wants to chime in.

Michal

Re: [PATCH] libxl: use b_info->{acpi,acpi} when available

Posted by Daniel P. Berrangé 1 week ago
On Wed, Sep 16, 2020 at 11:09:31AM +0200, Michal Privoznik wrote:
> On 9/10/20 6:18 AM, Marek Marczykowski-Górecki wrote:
> > b_info->u.hvm.{acpi,apic} are deprecated. But also, on recent libxl
> > version (4.14) the old one seems to be broken. While libxl part should
> > be fixed too, update the usage here and at some point drop support for
> > the old version.
> > b_info->acpi was added in Xen 4.8
> > b_info->apic was added in Xen 4.10
> > Xen 4.10 is the oldest version that still has security support (until
> > December 2020).
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> >   src/libxl/libxl_conf.c                              | 13 +++++++++++++
> >   tests/libxlxml2domconfigdata/basic-hvm.json         |  4 ++--
> >   tests/libxlxml2domconfigdata/cpu-shares-hvm.json    |  4 ++--
> >   .../libxlxml2domconfigdata/fullvirt-acpi-slic.json  |  4 ++--
> >   .../fullvirt-cpuid-legacy-nest.json                 |  4 ++--
> >   tests/libxlxml2domconfigdata/fullvirt-cpuid.json    |  4 ++--
> >   .../max-eventchannels-hvm.json                      |  4 ++--
> >   tests/libxlxml2domconfigdata/max-gntframes-hvm.json |  4 ++--
> >   tests/libxlxml2domconfigdata/moredevs-hvm.json      |  4 ++--
> >   .../libxlxml2domconfigdata/variable-clock-hvm.json  |  4 ++--
> >   .../vnuma-hvm-legacy-nest.json                      |  4 ++--
> >   tests/libxlxml2domconfigdata/vnuma-hvm.json         |  4 ++--
> >   12 files changed, 35 insertions(+), 22 deletions(-)
> 
> This looks good to me.
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> I'll wait a bit with pushing it though in case Jim wants to chime in.

This broke the build on Ubuntu 1804 due to tests failing

TEST: libxlxml2domconfigtest
      .!!!!.!!!!                               10  FAIL


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] libxl: use b_info->{acpi,acpi} when available

Posted by Michal Prívozník 1 week ago
On 9/18/20 1:31 PM, Daniel P. Berrangé wrote:
> On Wed, Sep 16, 2020 at 11:09:31AM +0200, Michal Privoznik wrote:
>> On 9/10/20 6:18 AM, Marek Marczykowski-Górecki wrote:
>>> b_info->u.hvm.{acpi,apic} are deprecated. But also, on recent libxl
>>> version (4.14) the old one seems to be broken. While libxl part should
>>> be fixed too, update the usage here and at some point drop support for
>>> the old version.
>>> b_info->acpi was added in Xen 4.8
>>> b_info->apic was added in Xen 4.10
>>> Xen 4.10 is the oldest version that still has security support (until
>>> December 2020).
>>>
>>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>>> ---
>>>    src/libxl/libxl_conf.c                              | 13 +++++++++++++
>>>    tests/libxlxml2domconfigdata/basic-hvm.json         |  4 ++--
>>>    tests/libxlxml2domconfigdata/cpu-shares-hvm.json    |  4 ++--
>>>    .../libxlxml2domconfigdata/fullvirt-acpi-slic.json  |  4 ++--
>>>    .../fullvirt-cpuid-legacy-nest.json                 |  4 ++--
>>>    tests/libxlxml2domconfigdata/fullvirt-cpuid.json    |  4 ++--
>>>    .../max-eventchannels-hvm.json                      |  4 ++--
>>>    tests/libxlxml2domconfigdata/max-gntframes-hvm.json |  4 ++--
>>>    tests/libxlxml2domconfigdata/moredevs-hvm.json      |  4 ++--
>>>    .../libxlxml2domconfigdata/variable-clock-hvm.json  |  4 ++--
>>>    .../vnuma-hvm-legacy-nest.json                      |  4 ++--
>>>    tests/libxlxml2domconfigdata/vnuma-hvm.json         |  4 ++--
>>>    12 files changed, 35 insertions(+), 22 deletions(-)
>>
>> This looks good to me.
>>
>> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>>
>> I'll wait a bit with pushing it though in case Jim wants to chime in.
> 
> This broke the build on Ubuntu 1804 due to tests failing
> 
> TEST: libxlxml2domconfigtest
>        .!!!!.!!!!                               10  FAIL

Oh, Ubuntu 18.04 has libxen-dev-4.9.2 and I'm not sure about FreeBSD, 
but probably something old too. So we can't use xen 4.10 APIs even 
though it was released 3 years ago.

Unfortunately, we will have to support Ubuntu 18.04 for quite some time 
because 20.04 was released only a while ago and we have this two year 
transition period:

https://libvirt.org/platforms.html

Marek, are you okay with me reverting the patch?

Michal

Re: [PATCH] libxl: use b_info->{acpi,acpi} when available

Posted by Marek Marczykowski-Górecki 1 week ago
On Fri, Sep 18, 2020 at 05:12:52PM +0200, Michal Prívozník wrote:
> On 9/18/20 1:31 PM, Daniel P. Berrangé wrote:
> > On Wed, Sep 16, 2020 at 11:09:31AM +0200, Michal Privoznik wrote:
> > > On 9/10/20 6:18 AM, Marek Marczykowski-Górecki wrote:
> > > > b_info->u.hvm.{acpi,apic} are deprecated. But also, on recent libxl
> > > > version (4.14) the old one seems to be broken. While libxl part should
> > > > be fixed too, update the usage here and at some point drop support for
> > > > the old version.
> > > > b_info->acpi was added in Xen 4.8
> > > > b_info->apic was added in Xen 4.10
> > > > Xen 4.10 is the oldest version that still has security support (until
> > > > December 2020).
> > > > 
> > > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > > ---
> > > >    src/libxl/libxl_conf.c                              | 13 +++++++++++++
> > > >    tests/libxlxml2domconfigdata/basic-hvm.json         |  4 ++--
> > > >    tests/libxlxml2domconfigdata/cpu-shares-hvm.json    |  4 ++--
> > > >    .../libxlxml2domconfigdata/fullvirt-acpi-slic.json  |  4 ++--
> > > >    .../fullvirt-cpuid-legacy-nest.json                 |  4 ++--
> > > >    tests/libxlxml2domconfigdata/fullvirt-cpuid.json    |  4 ++--
> > > >    .../max-eventchannels-hvm.json                      |  4 ++--
> > > >    tests/libxlxml2domconfigdata/max-gntframes-hvm.json |  4 ++--
> > > >    tests/libxlxml2domconfigdata/moredevs-hvm.json      |  4 ++--
> > > >    .../libxlxml2domconfigdata/variable-clock-hvm.json  |  4 ++--
> > > >    .../vnuma-hvm-legacy-nest.json                      |  4 ++--
> > > >    tests/libxlxml2domconfigdata/vnuma-hvm.json         |  4 ++--
> > > >    12 files changed, 35 insertions(+), 22 deletions(-)
> > > 
> > > This looks good to me.
> > > 
> > > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> > > 
> > > I'll wait a bit with pushing it though in case Jim wants to chime in.
> > 
> > This broke the build on Ubuntu 1804 due to tests failing
> > 
> > TEST: libxlxml2domconfigtest
> >        .!!!!.!!!!                               10  FAIL
> 
> Oh, Ubuntu 18.04 has libxen-dev-4.9.2 and I'm not sure about FreeBSD, but
> probably something old too. So we can't use xen 4.10 APIs even though it was
> released 3 years ago.
> 
> Unfortunately, we will have to support Ubuntu 18.04 for quite some time
> because 20.04 was released only a while ago and we have this two year
> transition period:
> 
> https://libvirt.org/platforms.html
> 
> Marek, are you okay with me reverting the patch?

Technically, the driver code itself should work on both versions (there
is an #ifdef for that). It's only an issue with tests, where we don't have
#ifdef inside json files...

One idea would be to duplicate those affected json files and pick the
right one based on the libxenlight version, but it doesn't sound nice...
Any alternative ideas?

If not, then indeed revert is the simplest solution.

-- 
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?

Re: [PATCH] libxl: use b_info->{acpi,acpi} when available

Posted by Daniel P. Berrangé 1 week ago
On Fri, Sep 18, 2020 at 05:41:21PM +0200, Marek Marczykowski-Górecki wrote:
> On Fri, Sep 18, 2020 at 05:12:52PM +0200, Michal Prívozník wrote:
> > On 9/18/20 1:31 PM, Daniel P. Berrangé wrote:
> > > On Wed, Sep 16, 2020 at 11:09:31AM +0200, Michal Privoznik wrote:
> > > > On 9/10/20 6:18 AM, Marek Marczykowski-Górecki wrote:
> > > > > b_info->u.hvm.{acpi,apic} are deprecated. But also, on recent libxl
> > > > > version (4.14) the old one seems to be broken. While libxl part should
> > > > > be fixed too, update the usage here and at some point drop support for
> > > > > the old version.
> > > > > b_info->acpi was added in Xen 4.8
> > > > > b_info->apic was added in Xen 4.10
> > > > > Xen 4.10 is the oldest version that still has security support (until
> > > > > December 2020).
> > > > > 
> > > > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > > > ---
> > > > >    src/libxl/libxl_conf.c                              | 13 +++++++++++++
> > > > >    tests/libxlxml2domconfigdata/basic-hvm.json         |  4 ++--
> > > > >    tests/libxlxml2domconfigdata/cpu-shares-hvm.json    |  4 ++--
> > > > >    .../libxlxml2domconfigdata/fullvirt-acpi-slic.json  |  4 ++--
> > > > >    .../fullvirt-cpuid-legacy-nest.json                 |  4 ++--
> > > > >    tests/libxlxml2domconfigdata/fullvirt-cpuid.json    |  4 ++--
> > > > >    .../max-eventchannels-hvm.json                      |  4 ++--
> > > > >    tests/libxlxml2domconfigdata/max-gntframes-hvm.json |  4 ++--
> > > > >    tests/libxlxml2domconfigdata/moredevs-hvm.json      |  4 ++--
> > > > >    .../libxlxml2domconfigdata/variable-clock-hvm.json  |  4 ++--
> > > > >    .../vnuma-hvm-legacy-nest.json                      |  4 ++--
> > > > >    tests/libxlxml2domconfigdata/vnuma-hvm.json         |  4 ++--
> > > > >    12 files changed, 35 insertions(+), 22 deletions(-)
> > > > 
> > > > This looks good to me.
> > > > 
> > > > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> > > > 
> > > > I'll wait a bit with pushing it though in case Jim wants to chime in.
> > > 
> > > This broke the build on Ubuntu 1804 due to tests failing
> > > 
> > > TEST: libxlxml2domconfigtest
> > >        .!!!!.!!!!                               10  FAIL
> > 
> > Oh, Ubuntu 18.04 has libxen-dev-4.9.2 and I'm not sure about FreeBSD, but
> > probably something old too. So we can't use xen 4.10 APIs even though it was
> > released 3 years ago.
> > 
> > Unfortunately, we will have to support Ubuntu 18.04 for quite some time
> > because 20.04 was released only a while ago and we have this two year
> > transition period:
> > 
> > https://libvirt.org/platforms.html
> > 
> > Marek, are you okay with me reverting the patch?
> 
> Technically, the driver code itself should work on both versions (there
> is an #ifdef for that). It's only an issue with tests, where we don't have
> #ifdef inside json files...
> 
> One idea would be to duplicate those affected json files and pick the
> right one based on the libxenlight version, but it doesn't sound nice...
> Any alternative ideas?

I think just duplicating the JSON files is the best solution because it
is simple, low overhead and keeps test coverage in both versions.

We'll remove the duplicates a year or two down the road when we drop
older distros, so the duplication is time limited.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] libxl: use b_info->{acpi,acpi} when available

Posted by Jim Fehlig 5 days ago
On 9/18/20 9:51 AM, Daniel P. Berrangé wrote:
> On Fri, Sep 18, 2020 at 05:41:21PM +0200, Marek Marczykowski-Górecki wrote:
>> On Fri, Sep 18, 2020 at 05:12:52PM +0200, Michal Prívozník wrote:
>>> On 9/18/20 1:31 PM, Daniel P. Berrangé wrote:
>>>> On Wed, Sep 16, 2020 at 11:09:31AM +0200, Michal Privoznik wrote:
>>>>> On 9/10/20 6:18 AM, Marek Marczykowski-Górecki wrote:
>>>>>> b_info->u.hvm.{acpi,apic} are deprecated. But also, on recent libxl
>>>>>> version (4.14) the old one seems to be broken. While libxl part should
>>>>>> be fixed too, update the usage here and at some point drop support for
>>>>>> the old version.
>>>>>> b_info->acpi was added in Xen 4.8
>>>>>> b_info->apic was added in Xen 4.10
>>>>>> Xen 4.10 is the oldest version that still has security support (until
>>>>>> December 2020).
>>>>>>
>>>>>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>>>>>> ---
>>>>>>     src/libxl/libxl_conf.c                              | 13 +++++++++++++
>>>>>>     tests/libxlxml2domconfigdata/basic-hvm.json         |  4 ++--
>>>>>>     tests/libxlxml2domconfigdata/cpu-shares-hvm.json    |  4 ++--
>>>>>>     .../libxlxml2domconfigdata/fullvirt-acpi-slic.json  |  4 ++--
>>>>>>     .../fullvirt-cpuid-legacy-nest.json                 |  4 ++--
>>>>>>     tests/libxlxml2domconfigdata/fullvirt-cpuid.json    |  4 ++--
>>>>>>     .../max-eventchannels-hvm.json                      |  4 ++--
>>>>>>     tests/libxlxml2domconfigdata/max-gntframes-hvm.json |  4 ++--
>>>>>>     tests/libxlxml2domconfigdata/moredevs-hvm.json      |  4 ++--
>>>>>>     .../libxlxml2domconfigdata/variable-clock-hvm.json  |  4 ++--
>>>>>>     .../vnuma-hvm-legacy-nest.json                      |  4 ++--
>>>>>>     tests/libxlxml2domconfigdata/vnuma-hvm.json         |  4 ++--
>>>>>>     12 files changed, 35 insertions(+), 22 deletions(-)
>>>>>
>>>>> This looks good to me.
>>>>>
>>>>> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>>>>>
>>>>> I'll wait a bit with pushing it though in case Jim wants to chime in.
>>>>
>>>> This broke the build on Ubuntu 1804 due to tests failing
>>>>
>>>> TEST: libxlxml2domconfigtest
>>>>         .!!!!.!!!!                               10  FAIL
>>>
>>> Oh, Ubuntu 18.04 has libxen-dev-4.9.2 and I'm not sure about FreeBSD, but
>>> probably something old too. So we can't use xen 4.10 APIs even though it was
>>> released 3 years ago.
>>>
>>> Unfortunately, we will have to support Ubuntu 18.04 for quite some time
>>> because 20.04 was released only a while ago and we have this two year
>>> transition period:
>>>
>>> https://libvirt.org/platforms.html
>>>
>>> Marek, are you okay with me reverting the patch?
>>
>> Technically, the driver code itself should work on both versions (there
>> is an #ifdef for that). It's only an issue with tests, where we don't have
>> #ifdef inside json files...
>>
>> One idea would be to duplicate those affected json files and pick the
>> right one based on the libxenlight version, but it doesn't sound nice...
>> Any alternative ideas?
> 
> I think just duplicating the JSON files is the best solution because it
> is simple, low overhead and keeps test coverage in both versions.

I'm catching up on libvirt mail and could have missed it, but I didn't see a 
follow-up patch to fix the test.

Marek, do you have time for it? If not I'll get to it tomorrow.

Regards,
Jim


Re: [PATCH] libxl: use b_info->{acpi,acpi} when available

Posted by Marek Marczykowski-Górecki 5 days ago
On Tue, Sep 22, 2020 at 09:41:00PM -0600, Jim Fehlig wrote:
> On 9/18/20 9:51 AM, Daniel P. Berrangé wrote:
> > On Fri, Sep 18, 2020 at 05:41:21PM +0200, Marek Marczykowski-Górecki wrote:
> > > On Fri, Sep 18, 2020 at 05:12:52PM +0200, Michal Prívozník wrote:
> > > > On 9/18/20 1:31 PM, Daniel P. Berrangé wrote:
> > > > > On Wed, Sep 16, 2020 at 11:09:31AM +0200, Michal Privoznik wrote:
> > > > > > On 9/10/20 6:18 AM, Marek Marczykowski-Górecki wrote:
> > > > > > > b_info->u.hvm.{acpi,apic} are deprecated. But also, on recent libxl
> > > > > > > version (4.14) the old one seems to be broken. While libxl part should
> > > > > > > be fixed too, update the usage here and at some point drop support for
> > > > > > > the old version.
> > > > > > > b_info->acpi was added in Xen 4.8
> > > > > > > b_info->apic was added in Xen 4.10
> > > > > > > Xen 4.10 is the oldest version that still has security support (until
> > > > > > > December 2020).
> > > > > > > 
> > > > > > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > > > > > ---
> > > > > > >     src/libxl/libxl_conf.c                              | 13 +++++++++++++
> > > > > > >     tests/libxlxml2domconfigdata/basic-hvm.json         |  4 ++--
> > > > > > >     tests/libxlxml2domconfigdata/cpu-shares-hvm.json    |  4 ++--
> > > > > > >     .../libxlxml2domconfigdata/fullvirt-acpi-slic.json  |  4 ++--
> > > > > > >     .../fullvirt-cpuid-legacy-nest.json                 |  4 ++--
> > > > > > >     tests/libxlxml2domconfigdata/fullvirt-cpuid.json    |  4 ++--
> > > > > > >     .../max-eventchannels-hvm.json                      |  4 ++--
> > > > > > >     tests/libxlxml2domconfigdata/max-gntframes-hvm.json |  4 ++--
> > > > > > >     tests/libxlxml2domconfigdata/moredevs-hvm.json      |  4 ++--
> > > > > > >     .../libxlxml2domconfigdata/variable-clock-hvm.json  |  4 ++--
> > > > > > >     .../vnuma-hvm-legacy-nest.json                      |  4 ++--
> > > > > > >     tests/libxlxml2domconfigdata/vnuma-hvm.json         |  4 ++--
> > > > > > >     12 files changed, 35 insertions(+), 22 deletions(-)
> > > > > > 
> > > > > > This looks good to me.
> > > > > > 
> > > > > > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> > > > > > 
> > > > > > I'll wait a bit with pushing it though in case Jim wants to chime in.
> > > > > 
> > > > > This broke the build on Ubuntu 1804 due to tests failing
> > > > > 
> > > > > TEST: libxlxml2domconfigtest
> > > > >         .!!!!.!!!!                               10  FAIL
> > > > 
> > > > Oh, Ubuntu 18.04 has libxen-dev-4.9.2 and I'm not sure about FreeBSD, but
> > > > probably something old too. So we can't use xen 4.10 APIs even though it was
> > > > released 3 years ago.
> > > > 
> > > > Unfortunately, we will have to support Ubuntu 18.04 for quite some time
> > > > because 20.04 was released only a while ago and we have this two year
> > > > transition period:
> > > > 
> > > > https://libvirt.org/platforms.html
> > > > 
> > > > Marek, are you okay with me reverting the patch?
> > > 
> > > Technically, the driver code itself should work on both versions (there
> > > is an #ifdef for that). It's only an issue with tests, where we don't have
> > > #ifdef inside json files...
> > > 
> > > One idea would be to duplicate those affected json files and pick the
> > > right one based on the libxenlight version, but it doesn't sound nice...
> > > Any alternative ideas?
> > 
> > I think just duplicating the JSON files is the best solution because it
> > is simple, low overhead and keeps test coverage in both versions.
> 
> I'm catching up on libvirt mail and could have missed it, but I didn't see a
> follow-up patch to fix the test.
> 
> Marek, do you have time for it? If not I'll get to it tomorrow.

Sadly I'm pretty busy this week, I may find some time tomorrow but can't
promise it.

-- 
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?

Re: [PATCH] libxl: use b_info->{acpi,acpi} when available

Posted by Jim Fehlig 4 days ago
On 9/23/20 1:54 AM, Marek Marczykowski-Górecki wrote:
> On Tue, Sep 22, 2020 at 09:41:00PM -0600, Jim Fehlig wrote:
>> On 9/18/20 9:51 AM, Daniel P. Berrangé wrote:
>>> On Fri, Sep 18, 2020 at 05:41:21PM +0200, Marek Marczykowski-Górecki wrote:
>>>> On Fri, Sep 18, 2020 at 05:12:52PM +0200, Michal Prívozník wrote:
>>>>> On 9/18/20 1:31 PM, Daniel P. Berrangé wrote:
>>>>>> On Wed, Sep 16, 2020 at 11:09:31AM +0200, Michal Privoznik wrote:
>>>>>>> On 9/10/20 6:18 AM, Marek Marczykowski-Górecki wrote:
>>>>>>>> b_info->u.hvm.{acpi,apic} are deprecated. But also, on recent libxl
>>>>>>>> version (4.14) the old one seems to be broken. While libxl part should
>>>>>>>> be fixed too, update the usage here and at some point drop support for
>>>>>>>> the old version.
>>>>>>>> b_info->acpi was added in Xen 4.8
>>>>>>>> b_info->apic was added in Xen 4.10
>>>>>>>> Xen 4.10 is the oldest version that still has security support (until
>>>>>>>> December 2020).
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>>>>>>>> ---
>>>>>>>>      src/libxl/libxl_conf.c                              | 13 +++++++++++++
>>>>>>>>      tests/libxlxml2domconfigdata/basic-hvm.json         |  4 ++--
>>>>>>>>      tests/libxlxml2domconfigdata/cpu-shares-hvm.json    |  4 ++--
>>>>>>>>      .../libxlxml2domconfigdata/fullvirt-acpi-slic.json  |  4 ++--
>>>>>>>>      .../fullvirt-cpuid-legacy-nest.json                 |  4 ++--
>>>>>>>>      tests/libxlxml2domconfigdata/fullvirt-cpuid.json    |  4 ++--
>>>>>>>>      .../max-eventchannels-hvm.json                      |  4 ++--
>>>>>>>>      tests/libxlxml2domconfigdata/max-gntframes-hvm.json |  4 ++--
>>>>>>>>      tests/libxlxml2domconfigdata/moredevs-hvm.json      |  4 ++--
>>>>>>>>      .../libxlxml2domconfigdata/variable-clock-hvm.json  |  4 ++--
>>>>>>>>      .../vnuma-hvm-legacy-nest.json                      |  4 ++--
>>>>>>>>      tests/libxlxml2domconfigdata/vnuma-hvm.json         |  4 ++--
>>>>>>>>      12 files changed, 35 insertions(+), 22 deletions(-)
>>>>>>>
>>>>>>> This looks good to me.
>>>>>>>
>>>>>>> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>>>>>>>
>>>>>>> I'll wait a bit with pushing it though in case Jim wants to chime in.
>>>>>>
>>>>>> This broke the build on Ubuntu 1804 due to tests failing
>>>>>>
>>>>>> TEST: libxlxml2domconfigtest
>>>>>>          .!!!!.!!!!                               10  FAIL
>>>>>
>>>>> Oh, Ubuntu 18.04 has libxen-dev-4.9.2 and I'm not sure about FreeBSD, but
>>>>> probably something old too. So we can't use xen 4.10 APIs even though it was
>>>>> released 3 years ago.
>>>>>
>>>>> Unfortunately, we will have to support Ubuntu 18.04 for quite some time
>>>>> because 20.04 was released only a while ago and we have this two year
>>>>> transition period:
>>>>>
>>>>> https://libvirt.org/platforms.html
>>>>>
>>>>> Marek, are you okay with me reverting the patch?
>>>>
>>>> Technically, the driver code itself should work on both versions (there
>>>> is an #ifdef for that). It's only an issue with tests, where we don't have
>>>> #ifdef inside json files...
>>>>
>>>> One idea would be to duplicate those affected json files and pick the
>>>> right one based on the libxenlight version, but it doesn't sound nice...
>>>> Any alternative ideas?
>>>
>>> I think just duplicating the JSON files is the best solution because it
>>> is simple, low overhead and keeps test coverage in both versions.
>>
>> I'm catching up on libvirt mail and could have missed it, but I didn't see a
>> follow-up patch to fix the test.
>>
>> Marek, do you have time for it? If not I'll get to it tomorrow.
> 
> Sadly I'm pretty busy this week, I may find some time tomorrow but can't
> promise it.

I cooked up a slightly more evil hack to avoid duplicate test data files

https://www.redhat.com/archives/libvir-list/2020-September/msg01311.html

Regards,
Jim


Re: [PATCH] libxl: use b_info->{acpi,acpi} when available

Posted by Jim Fehlig 1 week ago
On 9/16/20 3:09 AM, Michal Privoznik wrote:
> On 9/10/20 6:18 AM, Marek Marczykowski-Górecki wrote:
>> b_info->u.hvm.{acpi,apic} are deprecated. But also, on recent libxl
>> version (4.14) the old one seems to be broken. While libxl part should
>> be fixed too, update the usage here and at some point drop support for
>> the old version.
>> b_info->acpi was added in Xen 4.8
>> b_info->apic was added in Xen 4.10
>> Xen 4.10 is the oldest version that still has security support (until
>> December 2020).
>>
>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>> ---
>>   src/libxl/libxl_conf.c                              | 13 +++++++++++++
>>   tests/libxlxml2domconfigdata/basic-hvm.json         |  4 ++--
>>   tests/libxlxml2domconfigdata/cpu-shares-hvm.json    |  4 ++--
>>   .../libxlxml2domconfigdata/fullvirt-acpi-slic.json  |  4 ++--
>>   .../fullvirt-cpuid-legacy-nest.json                 |  4 ++--
>>   tests/libxlxml2domconfigdata/fullvirt-cpuid.json    |  4 ++--
>>   .../max-eventchannels-hvm.json                      |  4 ++--
>>   tests/libxlxml2domconfigdata/max-gntframes-hvm.json |  4 ++--
>>   tests/libxlxml2domconfigdata/moredevs-hvm.json      |  4 ++--
>>   .../libxlxml2domconfigdata/variable-clock-hvm.json  |  4 ++--
>>   .../vnuma-hvm-legacy-nest.json                      |  4 ++--
>>   tests/libxlxml2domconfigdata/vnuma-hvm.json         |  4 ++--
>>   12 files changed, 35 insertions(+), 22 deletions(-)
> 
> This looks good to me.
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> I'll wait a bit with pushing it though in case Jim wants to chime in.

It looks fine to me, so if you want it

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

On a slightly related note, it would be nice to bump the minimum supported 
LIBXL_API_VERSION in libvirt. Currently it is set to x040500. I'd like to bump 
it to 0x040800 (or perhaps higher). In fact, I have a downstream patch to do 
just that

https://build.opensuse.org/package/view_file/Virtualization/libvirt/suse-bump-xen-version.patch?expand=1

The problem is this API version was never advertised by libxl until Xen 4.13 
with commit c3999835df

http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=c3999835df2d9917cf4b50be80be9a6358b1219d

We would need that commit backported to all downstream Xen packages that libvirt 
is expected to build against, which we've done at SUSE. But I can't expect that 
from all the other distros wired up to the CI. Suggestions welcome :-).

Regards,
Jim


Re: [PATCH] libxl: use b_info->{acpi,acpi} when available

Posted by Michal Prívozník 1 week ago
On 9/17/20 7:57 PM, Jim Fehlig wrote:
> On 9/16/20 3:09 AM, Michal Privoznik wrote:
>> On 9/10/20 6:18 AM, Marek Marczykowski-Górecki wrote:
>>> b_info->u.hvm.{acpi,apic} are deprecated. But also, on recent libxl
>>> version (4.14) the old one seems to be broken. While libxl part should
>>> be fixed too, update the usage here and at some point drop support for
>>> the old version.
>>> b_info->acpi was added in Xen 4.8
>>> b_info->apic was added in Xen 4.10
>>> Xen 4.10 is the oldest version that still has security support (until
>>> December 2020).
>>>
>>> Signed-off-by: Marek Marczykowski-Górecki 
>>> <marmarek@invisiblethingslab.com>
>>> ---
>>>   src/libxl/libxl_conf.c                              | 13 +++++++++++++
>>>   tests/libxlxml2domconfigdata/basic-hvm.json         |  4 ++--
>>>   tests/libxlxml2domconfigdata/cpu-shares-hvm.json    |  4 ++--
>>>   .../libxlxml2domconfigdata/fullvirt-acpi-slic.json  |  4 ++--
>>>   .../fullvirt-cpuid-legacy-nest.json                 |  4 ++--
>>>   tests/libxlxml2domconfigdata/fullvirt-cpuid.json    |  4 ++--
>>>   .../max-eventchannels-hvm.json                      |  4 ++--
>>>   tests/libxlxml2domconfigdata/max-gntframes-hvm.json |  4 ++--
>>>   tests/libxlxml2domconfigdata/moredevs-hvm.json      |  4 ++--
>>>   .../libxlxml2domconfigdata/variable-clock-hvm.json  |  4 ++--
>>>   .../vnuma-hvm-legacy-nest.json                      |  4 ++--
>>>   tests/libxlxml2domconfigdata/vnuma-hvm.json         |  4 ++--
>>>   12 files changed, 35 insertions(+), 22 deletions(-)
>>
>> This looks good to me.
>>
>> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>>
>> I'll wait a bit with pushing it though in case Jim wants to chime in.
> 
> It looks fine to me, so if you want it
> 
> Reviewed-by: Jim Fehlig <jfehlig@suse.com>

Thanks, pushed.

> 
> On a slightly related note, it would be nice to bump the minimum 
> supported LIBXL_API_VERSION in libvirt. Currently it is set to x040500. 
> I'd like to bump it to 0x040800 (or perhaps higher). In fact, I have a 
> downstream patch to do just that
> 
> https://build.opensuse.org/package/view_file/Virtualization/libvirt/suse-bump-xen-version.patch?expand=1 
> 
> 
> The problem is this API version was never advertised by libxl until Xen 
> 4.13 with commit c3999835df
> 
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=c3999835df2d9917cf4b50be80be9a6358b1219d 
> 
> 
> We would need that commit backported to all downstream Xen packages that 
> libvirt is expected to build against, which we've done at SUSE. But I 
> can't expect that from all the other distros wired up to the CI. 
> Suggestions welcome :-).

Yeah, that's very unfortunate. I don't think I have a good answer, 
probably we have to wait until we can bump to 4.13 :-(

Michal