[v1] xen: recognize device_model_override

[PATCH RESEND v1] xen: recognize device_model_override

Posted by Olaf Hering 3 years, 5 months ago

Since Xen 4.2 libxl expects device_model_override="/path" instead of
device_model="/path". Adjust the code to parse this as <emulator>.
While libxl also recognizes device_model_version="", this knob is not
supported by libvirt. A runtime detection exists to select either
"qemu-xen" or "qemu-xen-traditional".

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 src/libxl/xen_common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c
index c82e487d80..88a784ed04 100644
--- a/src/libxl/xen_common.c
+++ b/src/libxl/xen_common.c
@@ -1508,7 +1508,7 @@ xenParseConfigCommon(virConfPtr conf,
     if (xenParseTimeOffset(conf, def) < 0)
         return -1;
 
-    if (xenConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0)
+    if (xenConfigCopyStringOpt(conf, "device_model_override", &def->emulator) < 0)
         return -1;
 
     if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XL)) {
@@ -2242,7 +2242,7 @@ static int
 xenFormatEmulator(virConfPtr conf, virDomainDefPtr def)
 {
     if (def->emulator &&
-        xenConfigSetString(conf, "device_model", def->emulator) < 0)
+        xenConfigSetString(conf, "device_model_override", def->emulator) < 0)
         return -1;
 
     return 0;

Re: [PATCH RESEND v1] xen: recognize device_model_override

Posted by Jim Fehlig 3 years, 5 months ago

On 11/12/20 11:03 AM, Olaf Hering wrote:
> Since Xen 4.2 libxl expects device_model_override="/path" instead of
> device_model="/path".

This has been on my todo list but never got the priority it deserved. Thanks for 
taking it on. Without your patch, using 'xl create' on xl.cfg converted from 
domXML by the libxl driver results in

WARNING: ignoring device_model directive.
WARNING: Use "device_model_override" instead if you really want a non-default 
device_model

> Adjust the code to parse this as <emulator>.

You also have to adjust all the failing {xl,xm}configtests.

> While libxl also recognizes device_model_version="", this knob is not
> supported by libvirt. A runtime detection exists to select either
> "qemu-xen" or "qemu-xen-traditional".

Correct. It is selected based on the specified <emulator> and not modeled in 
domXML. Does it need to be exposed to the user? AFAIK those are the only two 
values we care about, and it is possible to determine which to use based on the 
<emulator>.

Regards,
Jim

> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>   src/libxl/xen_common.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c
> index c82e487d80..88a784ed04 100644
> --- a/src/libxl/xen_common.c
> +++ b/src/libxl/xen_common.c
> @@ -1508,7 +1508,7 @@ xenParseConfigCommon(virConfPtr conf,
>       if (xenParseTimeOffset(conf, def) < 0)
>           return -1;
>   
> -    if (xenConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0)
> +    if (xenConfigCopyStringOpt(conf, "device_model_override", &def->emulator) < 0)
>           return -1;
>   
>       if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XL)) {
> @@ -2242,7 +2242,7 @@ static int
>   xenFormatEmulator(virConfPtr conf, virDomainDefPtr def)
>   {
>       if (def->emulator &&
> -        xenConfigSetString(conf, "device_model", def->emulator) < 0)
> +        xenConfigSetString(conf, "device_model_override", def->emulator) < 0)
>           return -1;
>   
>       return 0;
>

Re: [PATCH RESEND v1] xen: recognize device_model_override

Posted by Olaf Hering 3 years, 5 months ago

Am Mon, 16 Nov 2020 19:48:28 -0700
schrieb Jim Fehlig <jfehlig@suse.com>:

> > Adjust the code to parse this as <emulator>.  
> You also have to adjust all the failing {xl,xm}configtests.

It seems one has to run 'ninja test'.
Can this be done via _multibuild in a libvirt-testsuite pkg in an isolated VM?

> Correct. It is selected based on the specified <emulator> and not modeled in 
> domXML. Does it need to be exposed to the user? AFAIK those are the only two 
> values we care about, and it is possible to determine which to use based on the 
> <emulator>.

I think 'device_model_version' can be ignored.

In current xen.git support for "qemu-xen-traditional" was declared for stubdom only.
It is unrealistic that some HVM domUs with qemu-dm are still running out there.
In case they really exist, they were likely not started with libvirt.


Olaf

Re: [PATCH RESEND v1] xen: recognize device_model_override

Posted by Jim Fehlig 3 years, 5 months ago

On 11/19/20 12:58 PM, Olaf Hering wrote:
> Am Mon, 16 Nov 2020 19:48:28 -0700
> schrieb Jim Fehlig <jfehlig@suse.com>:
> 
>>> Adjust the code to parse this as <emulator>.
>> You also have to adjust all the failing {xl,xm}configtests.
> 
> It seems one has to run 'ninja test'.

Yes. See https://libvirt.org/hacking.html

> Can this be done via _multibuild in a libvirt-testsuite pkg in an isolated VM?

I suppose. Currently it is done in the downstream OBS package in %check

VIR_TEST_DEBUG=1 %meson_test -t 5 --no-suite syntax-check

>> Correct. It is selected based on the specified <emulator> and not modeled in
>> domXML. Does it need to be exposed to the user? AFAIK those are the only two
>> values we care about, and it is possible to determine which to use based on the
>> <emulator>.
> 
> I think 'device_model_version' can be ignored.

I think we need to take a closer look. It is not explicitly set in 
libxl_domain_build_info_init(), hence would be set to 
LIBXL_DEVICE_MODEL_VERSION_UNKNOWN. The only thing interesting from a quick grep 
in $xensrc/tools/libxl/ is

libxl_domain.c: assert(version != LIBXL_DEVICE_MODEL_VERSION_UNKNOWN);

> In current xen.git support for "qemu-xen-traditional" was declared for stubdom only.
> It is unrealistic that some HVM domUs with qemu-dm are still running out there.
> In case they really exist, they were likely not started with libvirt.

Or by "ignored" do you mean removing the logic and unconditionally setting it to 
LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN?

Regards,
Jim

Re: [PATCH RESEND v1] xen: recognize device_model_override

Posted by Olaf Hering 3 years, 4 months ago

Am Thu, 19 Nov 2020 16:11:34 -0700
schrieb Jim Fehlig <jfehlig@suse.com>:

> On 11/19/20 12:58 PM, Olaf Hering wrote:
> > Am Mon, 16 Nov 2020 19:48:28 -0700
> > schrieb Jim Fehlig <jfehlig@suse.com>:
> >> Correct. It is selected based on the specified <emulator> and not modeled in
> >> domXML. Does it need to be exposed to the user? AFAIK those are the only two
> >> values we care about, and it is possible to determine which to use based on the
> >> <emulator>.  
> > I think 'device_model_version' can be ignored.  
> I think we need to take a closer look. It is not explicitly set in 
> libxl_domain_build_info_init(), hence would be set to 
> LIBXL_DEVICE_MODEL_VERSION_UNKNOWN.

libxl__domain_build_info_setdefault sets it, if needed.

libxl_domain_create_new
  do_domain_create
    initiate_domain_create
      libxl__domain_config_setdefault
        libxl__domain_build_info_setdefault

Otherwise 'xl create' would fail as well if device_model_version= is missing domU.cfg.

Olaf

Re: [PATCH RESEND v1] xen: recognize device_model_override

Posted by Jim Fehlig 3 years, 4 months ago

On 12/7/20 10:15 AM, Olaf Hering wrote:
> Am Thu, 19 Nov 2020 16:11:34 -0700
> schrieb Jim Fehlig <jfehlig@suse.com>:
> 
>> On 11/19/20 12:58 PM, Olaf Hering wrote:
>>> Am Mon, 16 Nov 2020 19:48:28 -0700
>>> schrieb Jim Fehlig <jfehlig@suse.com>:
>>>> Correct. It is selected based on the specified <emulator> and not modeled in
>>>> domXML. Does it need to be exposed to the user? AFAIK those are the only two
>>>> values we care about, and it is possible to determine which to use based on the
>>>> <emulator>.
>>> I think 'device_model_version' can be ignored.
>> I think we need to take a closer look. It is not explicitly set in
>> libxl_domain_build_info_init(), hence would be set to
>> LIBXL_DEVICE_MODEL_VERSION_UNKNOWN.
> 
> libxl__domain_build_info_setdefault sets it, if needed.
> 
> libxl_domain_create_new
>    do_domain_create
>      initiate_domain_create
>        libxl__domain_config_setdefault
>          libxl__domain_build_info_setdefault

Which, if needed, calls libxl__default_device_model in the various 
libxl_<os-flavor>.c files.

> Otherwise 'xl create' would fail as well if device_model_version= is missing domU.cfg.

Yes, good point!

Jim

Re: [PATCH RESEND v1] xen: recognize device_model_override

Posted by Olaf Hering 3 years, 5 months ago

Am Thu, 19 Nov 2020 16:11:34 -0700
schrieb Jim Fehlig <jfehlig@suse.com>:

> I suppose. Currently it is done in the downstream OBS package in %check
> VIR_TEST_DEBUG=1 %meson_test -t 5 --no-suite syntax-check

I had to run the build in a chroot in a dom0, not sure why it happens to work elsewhere.
With v2 of the patch all checks pass again.

Will take a closer look at the device_model_version= handling next week.

Olaf