* Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 10:45 ` Roger Pau Monné
0 siblings, 0 replies; 42+ messages in thread
From: Roger Pau Monné @ 2019-05-16 10:45 UTC (permalink / raw)
To: Olaf Hering; +Cc: Juergen Gross, Wei Liu, Ian Jackson, xen-devel
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 10:57 ` Olaf Hering
0 siblings, 0 replies; 42+ messages in thread
From: Olaf Hering @ 2019-05-16 10:57 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: Juergen Gross, Wei Liu, Ian Jackson, xen-devel
[-- 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 10:57 ` Olaf Hering
0 siblings, 0 replies; 42+ messages in thread
From: Olaf Hering @ 2019-05-16 10:57 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: Juergen Gross, Wei Liu, Ian Jackson, xen-devel
[-- 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 11:24 ` Wei Liu
0 siblings, 0 replies; 42+ messages in thread
From: Wei Liu @ 2019-05-16 11:24 UTC (permalink / raw)
To: Olaf Hering
Cc: Juergen Gross, Wei Liu, Ian Jackson, xen-devel,
Roger Pau Monné
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 11:24 ` Wei Liu
0 siblings, 0 replies; 42+ messages in thread
From: Wei Liu @ 2019-05-16 11:24 UTC (permalink / raw)
To: Olaf Hering
Cc: Juergen Gross, Wei Liu, Ian Jackson, xen-devel,
Roger Pau Monné
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 11:38 ` Olaf Hering
0 siblings, 0 replies; 42+ messages in thread
From: Olaf Hering @ 2019-05-16 11:38 UTC (permalink / raw)
To: Wei Liu; +Cc: Juergen Gross, xen-devel, Ian Jackson, Roger Pau Monné
[-- 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 11:38 ` Olaf Hering
0 siblings, 0 replies; 42+ messages in thread
From: Olaf Hering @ 2019-05-16 11:38 UTC (permalink / raw)
To: Wei Liu; +Cc: Juergen Gross, xen-devel, Ian Jackson, Roger Pau Monné
[-- 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 11:50 ` Wei Liu
0 siblings, 0 replies; 42+ messages in thread
From: Wei Liu @ 2019-05-16 11:50 UTC (permalink / raw)
To: Olaf Hering
Cc: Juergen Gross, Ian Jackson, Wei Liu, xen-devel,
Roger Pau Monné
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 11:50 ` Wei Liu
0 siblings, 0 replies; 42+ messages in thread
From: Wei Liu @ 2019-05-16 11:50 UTC (permalink / raw)
To: Olaf Hering
Cc: Juergen Gross, Ian Jackson, Wei Liu, xen-devel,
Roger Pau Monné
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 12:04 ` Olaf Hering
0 siblings, 0 replies; 42+ messages in thread
From: Olaf Hering @ 2019-05-16 12:04 UTC (permalink / raw)
To: Wei Liu; +Cc: Juergen Gross, xen-devel, Ian Jackson, Roger Pau Monné
[-- 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 12:04 ` Olaf Hering
0 siblings, 0 replies; 42+ messages in thread
From: Olaf Hering @ 2019-05-16 12:04 UTC (permalink / raw)
To: Wei Liu; +Cc: Juergen Gross, xen-devel, Ian Jackson, Roger Pau Monné
[-- 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 12:18 ` Olaf Hering
0 siblings, 0 replies; 42+ messages in thread
From: Olaf Hering @ 2019-05-16 12:18 UTC (permalink / raw)
To: Wei Liu; +Cc: Juergen Gross, xen-devel, Ian Jackson, Roger Pau Monné
[-- 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 12:18 ` Olaf Hering
0 siblings, 0 replies; 42+ messages in thread
From: Olaf Hering @ 2019-05-16 12:18 UTC (permalink / raw)
To: Wei Liu; +Cc: Juergen Gross, xen-devel, Ian Jackson, Roger Pau Monné
[-- 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 12:50 ` Wei Liu
0 siblings, 0 replies; 42+ messages in thread
From: Wei Liu @ 2019-05-16 12:50 UTC (permalink / raw)
To: Olaf Hering
Cc: Juergen Gross, Ian Jackson, Wei Liu, xen-devel,
Roger Pau Monné
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 12:50 ` Wei Liu
0 siblings, 0 replies; 42+ messages in thread
From: Wei Liu @ 2019-05-16 12:50 UTC (permalink / raw)
To: Olaf Hering
Cc: Juergen Gross, Ian Jackson, Wei Liu, xen-devel,
Roger Pau Monné
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 11:52 ` Olaf Hering
0 siblings, 0 replies; 42+ messages in thread
From: Olaf Hering @ 2019-05-16 11:52 UTC (permalink / raw)
To: Wei Liu; +Cc: Juergen Gross, xen-devel, Ian Jackson, Roger Pau Monné
[-- 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 11:52 ` Olaf Hering
0 siblings, 0 replies; 42+ messages in thread
From: Olaf Hering @ 2019-05-16 11:52 UTC (permalink / raw)
To: Wei Liu; +Cc: Juergen Gross, xen-devel, Ian Jackson, Roger Pau Monné
[-- 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 11:42 ` Roger Pau Monné
0 siblings, 0 replies; 42+ messages in thread
From: Roger Pau Monné @ 2019-05-16 11:42 UTC (permalink / raw)
To: Wei Liu; +Cc: Juergen Gross, xen-devel, Ian Jackson, Olaf Hering
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 11:42 ` Roger Pau Monné
0 siblings, 0 replies; 42+ messages in thread
From: Roger Pau Monné @ 2019-05-16 11:42 UTC (permalink / raw)
To: Wei Liu; +Cc: Juergen Gross, xen-devel, Ian Jackson, Olaf Hering
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 12:42 ` Wei Liu
0 siblings, 0 replies; 42+ messages in thread
From: Wei Liu @ 2019-05-16 12:42 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Juergen Gross, Ian Jackson, Wei Liu, Olaf Hering, xen-devel
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 12:42 ` Wei Liu
0 siblings, 0 replies; 42+ messages in thread
From: Wei Liu @ 2019-05-16 12:42 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Juergen Gross, Ian Jackson, Wei Liu, Olaf Hering, xen-devel
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 11:24 ` Wei Liu
0 siblings, 0 replies; 42+ messages in thread
From: Wei Liu @ 2019-05-16 11:24 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Juergen Gross, Wei Liu, Ian Jackson, Olaf Hering, xen-devel
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] Regression in xen-unstable due to commit 3802ecbaa9eb36
@ 2019-05-16 11:24 ` Wei Liu
0 siblings, 0 replies; 42+ messages in thread
From: Wei Liu @ 2019-05-16 11:24 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Juergen Gross, Wei Liu, Ian Jackson, Olaf Hering, xen-devel
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
^ permalink raw reply [flat|nested] 42+ messages in thread