Regression in xen-unstable due to commit 3802ecbaa9eb36

Regression in xen-unstable due to commit 3802ecbaa9eb36

[Juergen Gross]

The patch "libxl: add helper function to set device_model_version"
breaks creating any domain for me.

Creating a PV or HVM guest will trigger the assertion

assert(b_info->device_model_version);

added by that patch. Removing this assert() will then trigger

xl: libxl.c:339: libxl_defbool_val: Assertion
`!libxl_defbool_is_default(db)' failed.

which is called by libxl__domain_build_info_setdefault() line 143,
which has been added by the same patch.

Has this patch ever been tested to work?

I have done a make distclean and configure, so I don't think this
should be a system specific problem.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Juergen Gross]

The patch "libxl: add helper function to set device_model_version"
breaks creating any domain for me.

Creating a PV or HVM guest will trigger the assertion

assert(b_info->device_model_version);

added by that patch. Removing this assert() will then trigger

xl: libxl.c:339: libxl_defbool_val: Assertion
`!libxl_defbool_is_default(db)' failed.

which is called by libxl__domain_build_info_setdefault() line 143,
which has been added by the same patch.

Has this patch ever been tested to work?

I have done a make distclean and configure, so I don't think this
should be a system specific problem.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 169 bytes --]

Am Thu, 16 May 2019 10:09:38 +0200
schrieb Juergen Gross <jgross@suse.com>:

> Has this patch ever been tested to work?

With PV only. I will have a look.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 169 bytes --]

Am Thu, 16 May 2019 10:09:38 +0200
schrieb Juergen Gross <jgross@suse.com>:

> Has this patch ever been tested to work?

With PV only. I will have a look.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Juergen Gross]

On 16/05/2019 10:27, Olaf Hering wrote:
> Am Thu, 16 May 2019 10:09:38 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> 
>> Has this patch ever been tested to work?
> 
> With PV only. I will have a look.

I can't even start a PV domU (maybe that is due to using qdisk?).


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Juergen Gross]

On 16/05/2019 10:27, Olaf Hering wrote:
> Am Thu, 16 May 2019 10:09:38 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> 
>> Has this patch ever been tested to work?
> 
> With PV only. I will have a look.

I can't even start a PV domU (maybe that is due to using qdisk?).


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 526 bytes --]

Am Thu, 16 May 2019 10:27:06 +0200
schrieb Olaf Hering <ohering@suse.com>:

> Am Thu, 16 May 2019 10:09:38 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> > Has this patch ever been tested to work?  
> With PV only. I will have a look.

It happens to work for me:
xen_changeset          : 2019-05-15 16:19:57 +0100 git:2a556b63a2

Domain-0
pv_sle12sp2_c_qcow2
pv_sle12sp2_c_raw
fv_sle12sp2_c_raw
fv_sle12sp2_c_qcow2

Maybe I misunderstand assert? I think the point is that the member is not zero.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 526 bytes --]

Am Thu, 16 May 2019 10:27:06 +0200
schrieb Olaf Hering <ohering@suse.com>:

> Am Thu, 16 May 2019 10:09:38 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> > Has this patch ever been tested to work?  
> With PV only. I will have a look.

It happens to work for me:
xen_changeset          : 2019-05-15 16:19:57 +0100 git:2a556b63a2

Domain-0
pv_sle12sp2_c_qcow2
pv_sle12sp2_c_raw
fv_sle12sp2_c_raw
fv_sle12sp2_c_qcow2

Maybe I misunderstand assert? I think the point is that the member is not zero.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Juergen Gross]

On 16/05/2019 10:30, Olaf Hering wrote:
> Am Thu, 16 May 2019 10:27:06 +0200
> schrieb Olaf Hering <ohering@suse.com>:
> 
>> Am Thu, 16 May 2019 10:09:38 +0200
>> schrieb Juergen Gross <jgross@suse.com>:
>>> Has this patch ever been tested to work?  
>> With PV only. I will have a look.
> 
> It happens to work for me:
> xen_changeset          : 2019-05-15 16:19:57 +0100 git:2a556b63a2
> 
> Domain-0
> pv_sle12sp2_c_qcow2
> pv_sle12sp2_c_raw
> fv_sle12sp2_c_raw
> fv_sle12sp2_c_qcow2
> 
> Maybe I misunderstand assert? I think the point is that the member is not zero.

Hmm, reverting your patches makes it work for me again.

My domain config:

name="sles12sp2"
description="None"
uuid="c59a5cf4-dc41-162b-8ae0-83174f6d87eb"
memory=2048
maxmem=2048
vcpus=1
maxvcpus=4
on_poweroff="destroy"
on_reboot="restart"
on_crash="destroy"
localtime=0
keymap="en-us"
type="pv"
console=tty0 xen-fbfront.video=32,1024,768"
kernel="/usr/lib/grub2/x86_64-xen/grub.xen"
disk=[ '/home/vm/images/sles12sp2,raw,xvda,w,backendtype=qdisk', ]
vif=[ 'mac=00:16:3e:2a:9e:87,bridge=br0', ]
nographic=1


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Juergen Gross]

On 16/05/2019 10:30, Olaf Hering wrote:
> Am Thu, 16 May 2019 10:27:06 +0200
> schrieb Olaf Hering <ohering@suse.com>:
> 
>> Am Thu, 16 May 2019 10:09:38 +0200
>> schrieb Juergen Gross <jgross@suse.com>:
>>> Has this patch ever been tested to work?  
>> With PV only. I will have a look.
> 
> It happens to work for me:
> xen_changeset          : 2019-05-15 16:19:57 +0100 git:2a556b63a2
> 
> Domain-0
> pv_sle12sp2_c_qcow2
> pv_sle12sp2_c_raw
> fv_sle12sp2_c_raw
> fv_sle12sp2_c_qcow2
> 
> Maybe I misunderstand assert? I think the point is that the member is not zero.

Hmm, reverting your patches makes it work for me again.

My domain config:

name="sles12sp2"
description="None"
uuid="c59a5cf4-dc41-162b-8ae0-83174f6d87eb"
memory=2048
maxmem=2048
vcpus=1
maxvcpus=4
on_poweroff="destroy"
on_reboot="restart"
on_crash="destroy"
localtime=0
keymap="en-us"
type="pv"
console=tty0 xen-fbfront.video=32,1024,768"
kernel="/usr/lib/grub2/x86_64-xen/grub.xen"
disk=[ '/home/vm/images/sles12sp2,raw,xvda,w,backendtype=qdisk', ]
vif=[ 'mac=00:16:3e:2a:9e:87,bridge=br0', ]
nographic=1


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 194 bytes --]

Am Thu, 16 May 2019 10:09:38 +0200
schrieb Juergen Gross <jgross@suse.com>:

> assert(b_info->device_model_version);

Is the codepath perhaps coming from libxl_domain_need_memory?

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 194 bytes --]

Am Thu, 16 May 2019 10:09:38 +0200
schrieb Juergen Gross <jgross@suse.com>:

> assert(b_info->device_model_version);

Is the codepath perhaps coming from libxl_domain_need_memory?

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 471 bytes --]

Am Thu, 16 May 2019 10:40:46 +0200
schrieb Olaf Hering <ohering@suse.com>:

> Am Thu, 16 May 2019 10:09:38 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> > assert(b_info->device_model_version);  
> Is the codepath perhaps coming from libxl_domain_need_memory?

If I read create_domain() correctly, freemem will be called with an
incomplete b_info. A workaround is to set device_model_version=.
Not sure why it works for me, I will see what I find.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 471 bytes --]

Am Thu, 16 May 2019 10:40:46 +0200
schrieb Olaf Hering <ohering@suse.com>:

> Am Thu, 16 May 2019 10:09:38 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> > assert(b_info->device_model_version);  
> Is the codepath perhaps coming from libxl_domain_need_memory?

If I read create_domain() correctly, freemem will be called with an
incomplete b_info. A workaround is to set device_model_version=.
Not sure why it works for me, I will see what I find.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Juergen Gross]

On 16/05/2019 10:40, Olaf Hering wrote:
> Am Thu, 16 May 2019 10:09:38 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> 
>> assert(b_info->device_model_version);
> 
> Is the codepath perhaps coming from libxl_domain_need_memory?

Yes:

(gdb) bt
#0  0x00007ffff6f1b0c7 in raise () from /lib64/libc.so.6
#1  0x00007ffff6f1c478 in abort () from /lib64/libc.so.6
#2  0x00007ffff6f14146 in __assert_fail_base () from /lib64/libc.so.6
#3  0x00007ffff6f141f2 in __assert_fail () from /lib64/libc.so.6
#4  0x00007ffff78e30c2 in libxl__domain_build_info_setdefault
(gc=0x7fffffffdae0, b_info=0x7fffffffd780) at libxl_create.c:134
#5  0x00007ffff793d13b in libxl_domain_need_memory (ctx=0x642050,
b_info_in=0x7fffffffdc88, need_memkb=0x7fffffffdb30) at libxl_mem.c:460
#6  0x0000000000427294 in ?? ()
#7  0x00007fffffffdc88 in ?? ()
#8  0xffffffff00000000 in ?? ()
#9  0x0000000000000000 in ?? ()


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Juergen Gross]

On 16/05/2019 10:40, Olaf Hering wrote:
> Am Thu, 16 May 2019 10:09:38 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> 
>> assert(b_info->device_model_version);
> 
> Is the codepath perhaps coming from libxl_domain_need_memory?

Yes:

(gdb) bt
#0  0x00007ffff6f1b0c7 in raise () from /lib64/libc.so.6
#1  0x00007ffff6f1c478 in abort () from /lib64/libc.so.6
#2  0x00007ffff6f14146 in __assert_fail_base () from /lib64/libc.so.6
#3  0x00007ffff6f141f2 in __assert_fail () from /lib64/libc.so.6
#4  0x00007ffff78e30c2 in libxl__domain_build_info_setdefault
(gc=0x7fffffffdae0, b_info=0x7fffffffd780) at libxl_create.c:134
#5  0x00007ffff793d13b in libxl_domain_need_memory (ctx=0x642050,
b_info_in=0x7fffffffdc88, need_memkb=0x7fffffffdb30) at libxl_mem.c:460
#6  0x0000000000427294 in ?? ()
#7  0x00007fffffffdc88 in ?? ()
#8  0xffffffff00000000 in ?? ()
#9  0x0000000000000000 in ?? ()


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 549 bytes --]

Am Thu, 16 May 2019 10:09:38 +0200
schrieb Juergen Gross <jgross@suse.com>:

> The patch "libxl: add helper function to set device_model_version"
> breaks creating any domain for me.

The issue is, create_domain will eventually call freemem.
If autoballoon is set, due to dom0_mem= for example, all is fine.
If memory has to be freed, libxl_domain_need_memory will get an
incomplete b_info. Somehow the new libxl__domain_set_device_model
must be called for the d_config returned by parse_config_data.

How should this be fixed?

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 549 bytes --]

Am Thu, 16 May 2019 10:09:38 +0200
schrieb Juergen Gross <jgross@suse.com>:

> The patch "libxl: add helper function to set device_model_version"
> breaks creating any domain for me.

The issue is, create_domain will eventually call freemem.
If autoballoon is set, due to dom0_mem= for example, all is fine.
If memory has to be freed, libxl_domain_need_memory will get an
incomplete b_info. Somehow the new libxl__domain_set_device_model
must be called for the d_config returned by parse_config_data.

How should this be fixed?

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Roger Pau Monné]

On Thu, May 16, 2019 at 11:07:35AM +0200, Olaf Hering wrote:
> Am Thu, 16 May 2019 10:09:38 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> 
> > The patch "libxl: add helper function to set device_model_version"
> > breaks creating any domain for me.
> 
> The issue is, create_domain will eventually call freemem.
> If autoballoon is set, due to dom0_mem= for example, all is fine.
> If memory has to be freed, libxl_domain_need_memory will get an
> incomplete b_info. Somehow the new libxl__domain_set_device_model
> must be called for the d_config returned by parse_config_data.
> 
> How should this be fixed?

Having a field in build_info with a default value that depends on
fields outside of build_info is problematic, since not all callers of
libxl__domain_build_info_setdefault have access to libxl_domain_config.

An option would be to pass libxl_domain_config to
libxl__domain_build_info_setdefault and fixup the callers. That seems
like the best solution ATM, but it would require reverting the
currently committed patches, since there won't be a reason anymore to
split the device model selection code out of
libxl__domain_build_info_setdefault.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Roger Pau Monné]

On Thu, May 16, 2019 at 11:07:35AM +0200, Olaf Hering wrote:
> Am Thu, 16 May 2019 10:09:38 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> 
> > The patch "libxl: add helper function to set device_model_version"
> > breaks creating any domain for me.
> 
> The issue is, create_domain will eventually call freemem.
> If autoballoon is set, due to dom0_mem= for example, all is fine.
> If memory has to be freed, libxl_domain_need_memory will get an
> incomplete b_info. Somehow the new libxl__domain_set_device_model
> must be called for the d_config returned by parse_config_data.
> 
> How should this be fixed?

Having a field in build_info with a default value that depends on
fields outside of build_info is problematic, since not all callers of
libxl__domain_build_info_setdefault have access to libxl_domain_config.

An option would be to pass libxl_domain_config to
libxl__domain_build_info_setdefault and fixup the callers. That seems
like the best solution ATM, but it would require reverting the
currently committed patches, since there won't be a reason anymore to
split the device model selection code out of
libxl__domain_build_info_setdefault.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 618 bytes --]

Am Thu, 16 May 2019 12:45:40 +0200
schrieb Roger Pau Monné <roger.pau@citrix.com>:

> Having a field in build_info with a default value that depends on
> fields outside of build_info is problematic, since not all callers of
> libxl__domain_build_info_setdefault have access to libxl_domain_config.

One option would be a new API that gets a libxl_domain_config and which
calls libxl__domain_set_device_model, libxl__domain_create_info_setdefault
and libxl__domain_build_info_setdefault. To me it looks like create_domain
can not build a proper d_config all on its own, it just has not enough info.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 618 bytes --]

Am Thu, 16 May 2019 12:45:40 +0200
schrieb Roger Pau Monné <roger.pau@citrix.com>:

> Having a field in build_info with a default value that depends on
> fields outside of build_info is problematic, since not all callers of
> libxl__domain_build_info_setdefault have access to libxl_domain_config.

One option would be a new API that gets a libxl_domain_config and which
calls libxl__domain_set_device_model, libxl__domain_create_info_setdefault
and libxl__domain_build_info_setdefault. To me it looks like create_domain
can not build a proper d_config all on its own, it just has not enough info.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Wei Liu]

On Thu, May 16, 2019 at 12:45:40PM +0200, Roger Pau Monné wrote:
> On Thu, May 16, 2019 at 11:07:35AM +0200, Olaf Hering wrote:
> > Am Thu, 16 May 2019 10:09:38 +0200
> > schrieb Juergen Gross <jgross@suse.com>:
> > 
> > > The patch "libxl: add helper function to set device_model_version"
> > > breaks creating any domain for me.
> > 
> > The issue is, create_domain will eventually call freemem.
> > If autoballoon is set, due to dom0_mem= for example, all is fine.
> > If memory has to be freed, libxl_domain_need_memory will get an
> > incomplete b_info. Somehow the new libxl__domain_set_device_model
> > must be called for the d_config returned by parse_config_data.
> > 
> > How should this be fixed?
> 
> Having a field in build_info with a default value that depends on
> fields outside of build_info is problematic, since not all callers of
> libxl__domain_build_info_setdefault have access to libxl_domain_config.
> 
> An option would be to pass libxl_domain_config to
> libxl__domain_build_info_setdefault and fixup the callers. That seems
> like the best solution ATM, but it would require reverting the

That will 1) make the name wrong 2) you will have to conjure up a
domain_config structure even if it is not needed...

Wei.

> currently committed patches, since there won't be a reason anymore to
> split the device model selection code out of
> libxl__domain_build_info_setdefault.
> 
> Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Wei Liu]

On Thu, May 16, 2019 at 12:45:40PM +0200, Roger Pau Monné wrote:
> On Thu, May 16, 2019 at 11:07:35AM +0200, Olaf Hering wrote:
> > Am Thu, 16 May 2019 10:09:38 +0200
> > schrieb Juergen Gross <jgross@suse.com>:
> > 
> > > The patch "libxl: add helper function to set device_model_version"
> > > breaks creating any domain for me.
> > 
> > The issue is, create_domain will eventually call freemem.
> > If autoballoon is set, due to dom0_mem= for example, all is fine.
> > If memory has to be freed, libxl_domain_need_memory will get an
> > incomplete b_info. Somehow the new libxl__domain_set_device_model
> > must be called for the d_config returned by parse_config_data.
> > 
> > How should this be fixed?
> 
> Having a field in build_info with a default value that depends on
> fields outside of build_info is problematic, since not all callers of
> libxl__domain_build_info_setdefault have access to libxl_domain_config.
> 
> An option would be to pass libxl_domain_config to
> libxl__domain_build_info_setdefault and fixup the callers. That seems
> like the best solution ATM, but it would require reverting the

That will 1) make the name wrong 2) you will have to conjure up a
domain_config structure even if it is not needed...

Wei.

> currently committed patches, since there won't be a reason anymore to
> split the device model selection code out of
> libxl__domain_build_info_setdefault.
> 
> Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Wei Liu]

On Thu, May 16, 2019 at 12:57:35PM +0200, Olaf Hering wrote:
> Am Thu, 16 May 2019 12:45:40 +0200
> schrieb Roger Pau Monné <roger.pau@citrix.com>:
> 
> > Having a field in build_info with a default value that depends on
> > fields outside of build_info is problematic, since not all callers of
> > libxl__domain_build_info_setdefault have access to libxl_domain_config.
> 
> One option would be a new API that gets a libxl_domain_config and which
> calls libxl__domain_set_device_model, libxl__domain_create_info_setdefault
> and libxl__domain_build_info_setdefault. To me it looks like create_domain
> can not build a proper d_config all on its own, it just has not enough info.

If you're talking about adding a new _public_ API:

The problem with this approach is that it doesn't help existing libxl
users. They will need to be fixed by calling this new API.

Will it work if 1) you make libxl__domain_set_device_model idempotent
and 2) call it from within libxl__domain_build_info_setdefault (which
basically restores the original code path before your patch)?

Wei.


> 
> Olaf



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Wei Liu]

On Thu, May 16, 2019 at 12:57:35PM +0200, Olaf Hering wrote:
> Am Thu, 16 May 2019 12:45:40 +0200
> schrieb Roger Pau Monné <roger.pau@citrix.com>:
> 
> > Having a field in build_info with a default value that depends on
> > fields outside of build_info is problematic, since not all callers of
> > libxl__domain_build_info_setdefault have access to libxl_domain_config.
> 
> One option would be a new API that gets a libxl_domain_config and which
> calls libxl__domain_set_device_model, libxl__domain_create_info_setdefault
> and libxl__domain_build_info_setdefault. To me it looks like create_domain
> can not build a proper d_config all on its own, it just has not enough info.

If you're talking about adding a new _public_ API:

The problem with this approach is that it doesn't help existing libxl
users. They will need to be fixed by calling this new API.

Will it work if 1) you make libxl__domain_set_device_model idempotent
and 2) call it from within libxl__domain_build_info_setdefault (which
basically restores the original code path before your patch)?

Wei.


> 
> Olaf



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 928 bytes --]

Am Thu, 16 May 2019 12:24:50 +0100
schrieb Wei Liu <wei.liu2@citrix.com>:

> The problem with this approach is that it doesn't help existing libxl
> users. They will need to be fixed by calling this new API.

If the API needs to be changed, a LIBXL_HAVE_ came with the change.

I'm not sure how to fix this without changing some API.
libxl__domain_build_info_setdefault would need a d_config to make a
usable decision. The callers do not have a d_config. And what their
calles have is an incomplete d_config because libxl lacks a public API
to properly populate missing defaults in d_config.

To me it looks like something like libxl_domain_config_finish(libxl_domain_config*)
is missing now.

Maybe I am just misunderstanding what you trying to say, but to me it
looks like freemem() would need to call an updated libxl public API
anyway. Surely not freemem itself, but xl:create_domain as a whole.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 928 bytes --]

Am Thu, 16 May 2019 12:24:50 +0100
schrieb Wei Liu <wei.liu2@citrix.com>:

> The problem with this approach is that it doesn't help existing libxl
> users. They will need to be fixed by calling this new API.

If the API needs to be changed, a LIBXL_HAVE_ came with the change.

I'm not sure how to fix this without changing some API.
libxl__domain_build_info_setdefault would need a d_config to make a
usable decision. The callers do not have a d_config. And what their
calles have is an incomplete d_config because libxl lacks a public API
to properly populate missing defaults in d_config.

To me it looks like something like libxl_domain_config_finish(libxl_domain_config*)
is missing now.

Maybe I am just misunderstanding what you trying to say, but to me it
looks like freemem() would need to call an updated libxl public API
anyway. Surely not freemem itself, but xl:create_domain as a whole.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Roger Pau Monné]

On Thu, May 16, 2019 at 12:24:50PM +0100, Wei Liu wrote:
> On Thu, May 16, 2019 at 12:57:35PM +0200, Olaf Hering wrote:
> > Am Thu, 16 May 2019 12:45:40 +0200
> > schrieb Roger Pau Monné <roger.pau@citrix.com>:
> > 
> > > Having a field in build_info with a default value that depends on
> > > fields outside of build_info is problematic, since not all callers of
> > > libxl__domain_build_info_setdefault have access to libxl_domain_config.
> > 
> > One option would be a new API that gets a libxl_domain_config and which
> > calls libxl__domain_set_device_model, libxl__domain_create_info_setdefault
> > and libxl__domain_build_info_setdefault. To me it looks like create_domain
> > can not build a proper d_config all on its own, it just has not enough info.
> 
> If you're talking about adding a new _public_ API:
> 
> The problem with this approach is that it doesn't help existing libxl
> users. They will need to be fixed by calling this new API.
> 
> Will it work if 1) you make libxl__domain_set_device_model idempotent
> and 2) call it from within libxl__domain_build_info_setdefault (which
> basically restores the original code path before your patch)?

Calling libxl__domain_set_device_model from
libxl__domain_build_info_setdefault would require passing
domain_config to libxl__domain_build_info_setdefault, which gets back
to my proposal.

In order to know if a PV or PVH domain requires a device model (for
the PV backends) libxl needs to look at the contents of domain_config
because that's where the list of devices is stored.

Another option would be to differentiate between device model and PV
backend. In the PV/PVH case QEMU is not acting as a device model but
rather as a PV backend, hence it could be signaled using a different
field, that could live in domain_config instead of
domain_build_info?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Roger Pau Monné]

On Thu, May 16, 2019 at 12:24:50PM +0100, Wei Liu wrote:
> On Thu, May 16, 2019 at 12:57:35PM +0200, Olaf Hering wrote:
> > Am Thu, 16 May 2019 12:45:40 +0200
> > schrieb Roger Pau Monné <roger.pau@citrix.com>:
> > 
> > > Having a field in build_info with a default value that depends on
> > > fields outside of build_info is problematic, since not all callers of
> > > libxl__domain_build_info_setdefault have access to libxl_domain_config.
> > 
> > One option would be a new API that gets a libxl_domain_config and which
> > calls libxl__domain_set_device_model, libxl__domain_create_info_setdefault
> > and libxl__domain_build_info_setdefault. To me it looks like create_domain
> > can not build a proper d_config all on its own, it just has not enough info.
> 
> If you're talking about adding a new _public_ API:
> 
> The problem with this approach is that it doesn't help existing libxl
> users. They will need to be fixed by calling this new API.
> 
> Will it work if 1) you make libxl__domain_set_device_model idempotent
> and 2) call it from within libxl__domain_build_info_setdefault (which
> basically restores the original code path before your patch)?

Calling libxl__domain_set_device_model from
libxl__domain_build_info_setdefault would require passing
domain_config to libxl__domain_build_info_setdefault, which gets back
to my proposal.

In order to know if a PV or PVH domain requires a device model (for
the PV backends) libxl needs to look at the contents of domain_config
because that's where the list of devices is stored.

Another option would be to differentiate between device model and PV
backend. In the PV/PVH case QEMU is not acting as a device model but
rather as a PV backend, hence it could be signaled using a different
field, that could live in domain_config instead of
domain_build_info?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Wei Liu]

On Thu, May 16, 2019 at 01:38:57PM +0200, Olaf Hering wrote:
> Am Thu, 16 May 2019 12:24:50 +0100
> schrieb Wei Liu <wei.liu2@citrix.com>:
> 
> > The problem with this approach is that it doesn't help existing libxl
> > users. They will need to be fixed by calling this new API.
> 
> If the API needs to be changed, a LIBXL_HAVE_ came with the change.
> 
> I'm not sure how to fix this without changing some API.
> libxl__domain_build_info_setdefault would need a d_config to make a
> usable decision. The callers do not have a d_config. And what their
> calles have is an incomplete d_config because libxl lacks a public API
> to properly populate missing defaults in d_config.
> 
> To me it looks like something like libxl_domain_config_finish(libxl_domain_config*)
> is missing now.
> 
> Maybe I am just misunderstanding what you trying to say, but to me it
> looks like freemem() would need to call an updated libxl public API
> anyway. Surely not freemem itself, but xl:create_domain as a whole.

Yes, that's what I want to avoid if possible. xl is just one of the
users of libxl. You will likely need to change libvirt as well.

Adding new APIs and defining LIBXL_HAVE won't get you out of this hole.
Yes you can add new APIs and new users are free to use them, but You
still need to retain backward compatibility somehow.

Wei.

> 
> Olaf



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Wei Liu]

On Thu, May 16, 2019 at 01:38:57PM +0200, Olaf Hering wrote:
> Am Thu, 16 May 2019 12:24:50 +0100
> schrieb Wei Liu <wei.liu2@citrix.com>:
> 
> > The problem with this approach is that it doesn't help existing libxl
> > users. They will need to be fixed by calling this new API.
> 
> If the API needs to be changed, a LIBXL_HAVE_ came with the change.
> 
> I'm not sure how to fix this without changing some API.
> libxl__domain_build_info_setdefault would need a d_config to make a
> usable decision. The callers do not have a d_config. And what their
> calles have is an incomplete d_config because libxl lacks a public API
> to properly populate missing defaults in d_config.
> 
> To me it looks like something like libxl_domain_config_finish(libxl_domain_config*)
> is missing now.
> 
> Maybe I am just misunderstanding what you trying to say, but to me it
> looks like freemem() would need to call an updated libxl public API
> anyway. Surely not freemem itself, but xl:create_domain as a whole.

Yes, that's what I want to avoid if possible. xl is just one of the
users of libxl. You will likely need to change libvirt as well.

Adding new APIs and defining LIBXL_HAVE won't get you out of this hole.
Yes you can add new APIs and new users are free to use them, but You
still need to retain backward compatibility somehow.

Wei.

> 
> Olaf



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 480 bytes --]

Am Thu, 16 May 2019 13:38:57 +0200
schrieb Olaf Hering <ohering@suse.com>:

> To me it looks like something like libxl_domain_config_finish(libxl_domain_config*)
> is missing now.

If we do not want to add a new API, a hack might be to use container_of()
and assume that the libxl_domain_build_info passed to libxl_domain_need_memory
is always part of a libxl_domain_config. This is, at this point, true
for at least libvirt and xl. But it is certainly fragile.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 480 bytes --]

Am Thu, 16 May 2019 13:38:57 +0200
schrieb Olaf Hering <ohering@suse.com>:

> To me it looks like something like libxl_domain_config_finish(libxl_domain_config*)
> is missing now.

If we do not want to add a new API, a hack might be to use container_of()
and assume that the libxl_domain_build_info passed to libxl_domain_need_memory
is always part of a libxl_domain_config. This is, at this point, true
for at least libvirt and xl. But it is certainly fragile.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 798 bytes --]

Am Thu, 16 May 2019 12:50:43 +0100
schrieb Wei Liu <wei.liu2@citrix.com>:

> Adding new APIs and defining LIBXL_HAVE won't get you out of this hole.

From what I see, src/libxl/libxl_conf.c:libxlBuildDomainConfig would need
to call libxl_domain_config_finish(libxl_domain_config*) at the end of
that function, wrapped in a #ifdef.

But your are right, old libvirt and new libxl need a solution without
introducing a new API.

There are quite a few checks for device_model_version, they would be all
wrong if the assert is removed, or changed back to QEMU_XEN. Perhaps we
can continue to live with that error. device_model_version could become
a local variable. If it is not set, assume the caller just wants the
memory size and enforce QEMU_XEN again within that function.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 798 bytes --]

Am Thu, 16 May 2019 12:50:43 +0100
schrieb Wei Liu <wei.liu2@citrix.com>:

> Adding new APIs and defining LIBXL_HAVE won't get you out of this hole.

From what I see, src/libxl/libxl_conf.c:libxlBuildDomainConfig would need
to call libxl_domain_config_finish(libxl_domain_config*) at the end of
that function, wrapped in a #ifdef.

But your are right, old libvirt and new libxl need a solution without
introducing a new API.

There are quite a few checks for device_model_version, they would be all
wrong if the assert is removed, or changed back to QEMU_XEN. Perhaps we
can continue to live with that error. device_model_version could become
a local variable. If it is not set, assume the caller just wants the
memory size and enforce QEMU_XEN again within that function.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 775 bytes --]

Am Thu, 16 May 2019 14:04:51 +0200
schrieb Olaf Hering <ohering@suse.com>:

> There are quite a few checks for device_model_version, they would be all
> wrong if the assert is removed, or changed back to QEMU_XEN. Perhaps we
> can continue to live with that error. device_model_version could become
> a local variable. If it is not set, assume the caller just wants the
> memory size and enforce QEMU_XEN again within that function.

I think that is what should be done, just for the sake of
libxl_domain_need_memory. If an incomplete b_info is provided, assume
device_model_version=QEMU_XEN, maintain that state in a local variable.

If we want to provide another new public API to fill missing defaults,
that could still be done in a separate patch.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 775 bytes --]

Am Thu, 16 May 2019 14:04:51 +0200
schrieb Olaf Hering <ohering@suse.com>:

> There are quite a few checks for device_model_version, they would be all
> wrong if the assert is removed, or changed back to QEMU_XEN. Perhaps we
> can continue to live with that error. device_model_version could become
> a local variable. If it is not set, assume the caller just wants the
> memory size and enforce QEMU_XEN again within that function.

I think that is what should be done, just for the sake of
libxl_domain_need_memory. If an incomplete b_info is provided, assume
device_model_version=QEMU_XEN, maintain that state in a local variable.

If we want to provide another new public API to fill missing defaults,
that could still be done in a separate patch.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Wei Liu]

On Thu, May 16, 2019 at 01:42:55PM +0200, Roger Pau Monné wrote:
> On Thu, May 16, 2019 at 12:24:50PM +0100, Wei Liu wrote:
> > On Thu, May 16, 2019 at 12:57:35PM +0200, Olaf Hering wrote:
> > > Am Thu, 16 May 2019 12:45:40 +0200
> > > schrieb Roger Pau Monné <roger.pau@citrix.com>:
> > > 
> > > > Having a field in build_info with a default value that depends on
> > > > fields outside of build_info is problematic, since not all callers of
> > > > libxl__domain_build_info_setdefault have access to libxl_domain_config.
> > > 
> > > One option would be a new API that gets a libxl_domain_config and which
> > > calls libxl__domain_set_device_model, libxl__domain_create_info_setdefault
> > > and libxl__domain_build_info_setdefault. To me it looks like create_domain
> > > can not build a proper d_config all on its own, it just has not enough info.
> > 
> > If you're talking about adding a new _public_ API:
> > 
> > The problem with this approach is that it doesn't help existing libxl
> > users. They will need to be fixed by calling this new API.
> > 
> > Will it work if 1) you make libxl__domain_set_device_model idempotent
> > and 2) call it from within libxl__domain_build_info_setdefault (which
> > basically restores the original code path before your patch)?
> 
> Calling libxl__domain_set_device_model from
> libxl__domain_build_info_setdefault would require passing
> domain_config to libxl__domain_build_info_setdefault, which gets back
> to my proposal.
> 

Oh I see what you were getting at.

I have merely been looking at the one patch that introduced this
regression which didn't use any field in d_config. But in the subsequent
patch d_config was used.

> In order to know if a PV or PVH domain requires a device model (for
> the PV backends) libxl needs to look at the contents of domain_config
> because that's where the list of devices is stored.
> 
> Another option would be to differentiate between device model and PV
> backend. In the PV/PVH case QEMU is not acting as a device model but
> rather as a PV backend, hence it could be signaled using a different
> field, that could live in domain_config instead of
> domain_build_info?

I think this is generally a good idea. Not sure how much code churn is
going to be though.

Wei.

> 
> Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Wei Liu]

On Thu, May 16, 2019 at 01:42:55PM +0200, Roger Pau Monné wrote:
> On Thu, May 16, 2019 at 12:24:50PM +0100, Wei Liu wrote:
> > On Thu, May 16, 2019 at 12:57:35PM +0200, Olaf Hering wrote:
> > > Am Thu, 16 May 2019 12:45:40 +0200
> > > schrieb Roger Pau Monné <roger.pau@citrix.com>:
> > > 
> > > > Having a field in build_info with a default value that depends on
> > > > fields outside of build_info is problematic, since not all callers of
> > > > libxl__domain_build_info_setdefault have access to libxl_domain_config.
> > > 
> > > One option would be a new API that gets a libxl_domain_config and which
> > > calls libxl__domain_set_device_model, libxl__domain_create_info_setdefault
> > > and libxl__domain_build_info_setdefault. To me it looks like create_domain
> > > can not build a proper d_config all on its own, it just has not enough info.
> > 
> > If you're talking about adding a new _public_ API:
> > 
> > The problem with this approach is that it doesn't help existing libxl
> > users. They will need to be fixed by calling this new API.
> > 
> > Will it work if 1) you make libxl__domain_set_device_model idempotent
> > and 2) call it from within libxl__domain_build_info_setdefault (which
> > basically restores the original code path before your patch)?
> 
> Calling libxl__domain_set_device_model from
> libxl__domain_build_info_setdefault would require passing
> domain_config to libxl__domain_build_info_setdefault, which gets back
> to my proposal.
> 

Oh I see what you were getting at.

I have merely been looking at the one patch that introduced this
regression which didn't use any field in d_config. But in the subsequent
patch d_config was used.

> In order to know if a PV or PVH domain requires a device model (for
> the PV backends) libxl needs to look at the contents of domain_config
> because that's where the list of devices is stored.
> 
> Another option would be to differentiate between device model and PV
> backend. In the PV/PVH case QEMU is not acting as a device model but
> rather as a PV backend, hence it could be signaled using a different
> field, that could live in domain_config instead of
> domain_build_info?

I think this is generally a good idea. Not sure how much code churn is
going to be though.

Wei.

> 
> Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Regression in xen-unstable due to commit 3802ecbaa9eb36

[Wei Liu]

On Thu, May 16, 2019 at 02:18:05PM +0200, Olaf Hering wrote:
> Am Thu, 16 May 2019 14:04:51 +0200
> schrieb Olaf Hering <ohering@suse.com>:
> 
> > There are quite a few checks for device_model_version, they would be all
> > wrong if the assert is removed, or changed back to QEMU_XEN. Perhaps we
> > can continue to live with that error. device_model_version could become
> > a local variable. If it is not set, assume the caller just wants the
> > memory size and enforce QEMU_XEN again within that function.
> 
> I think that is what should be done, just for the sake of
> libxl_domain_need_memory. If an incomplete b_info is provided, assume
> device_model_version=QEMU_XEN, maintain that state in a local variable.

I checked, libxl_domain_need_memory is actually the only public API
which takes b_info.

So this should work -- we just need to fix that one API to retain its
original behaviour.

> If we want to provide another new public API to fill missing defaults,
> that could still be done in a separate patch.
> 

Yes.

Wei.

> Olaf



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36

[Wei Liu]

On Thu, May 16, 2019 at 02:18:05PM +0200, Olaf Hering wrote:
> Am Thu, 16 May 2019 14:04:51 +0200
> schrieb Olaf Hering <ohering@suse.com>:
> 
> > There are quite a few checks for device_model_version, they would be all
> > wrong if the assert is removed, or changed back to QEMU_XEN. Perhaps we
> > can continue to live with that error. device_model_version could become
> > a local variable. If it is not set, assume the caller just wants the
> > memory size and enforce QEMU_XEN again within that function.
> 
> I think that is what should be done, just for the sake of
> libxl_domain_need_memory. If an incomplete b_info is provided, assume
> device_model_version=QEMU_XEN, maintain that state in a local variable.

I checked, libxl_domain_need_memory is actually the only public API
which takes b_info.

So this should work -- we just need to fix that one API to retain its
original behaviour.

> If we want to provide another new public API to fill missing defaults,
> that could still be done in a separate patch.
> 

Yes.

Wei.

> Olaf



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel