Commit 1787cc167906f3f ("libs/guest: Move the guest ABI check earlier
into xc_dom_parse_image()") broke starting the xenstore stubdom. This
is due to a rather special way the xenstore stubdom domain config is
being initialized: in order to support both, PV and PVH stubdom,
init-xenstore-domain is using xc_dom_parse_image() to find the correct
domain type. Unfortunately above commit requires xc_dom_boot_xen_init()
to have been called before using xc_dom_parse_image(). This requires
the domid, which is known only after xc_domain_create(), which requires
the domain type.

In order to break this circular dependency, call xc_dom_boot_xen_init()
with an arbitrary domid first, and then set dom->guest_domid later.

Fixes: 1787cc167906f3f ("libs/guest: Move the guest ABI check earlier into xc_dom_parse_image()")
Signed-off-by: Juergen Gross <jgross@suse.com>
Release-acked-by: Ian Jackson <iwj@xenproject.org>
---
 tools/helpers/init-xenstore-domain.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index 6836002f0b..a79662bd1b 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -132,6 +132,13 @@ static int build(xc_interface *xch)
         }
     }
 
+    rv = xc_dom_boot_xen_init(dom, xch, domid);
+    if ( rv )
+    {
+        fprintf(stderr, "xc_dom_boot_xen_init failed\n");
+        goto err;
+    }
+
     dom->container_type = XC_DOM_HVM_CONTAINER;
     rv = xc_dom_parse_image(dom);
     if ( rv )
@@ -214,16 +221,11 @@ static int build(xc_interface *xch)
     else
         snprintf(cmdline, 512, "--event %d --internal-db", rv);
 
+    dom->guest_domid = domid;
     dom->cmdline = xc_dom_strdup(dom, cmdline);
     dom->xenstore_domid = domid;
     dom->console_evtchn = console_evtchn;
 
-    rv = xc_dom_boot_xen_init(dom, xch, domid);
-    if ( rv )
-    {
-        fprintf(stderr, "xc_dom_boot_xen_init failed\n");
-        goto err;
-    }
     rv = xc_dom_mem_init(dom, memory);
     if ( rv )
     {
-- 
2.26.2

On 04/11/2021 14:42, Juergen Gross wrote:
> Commit 1787cc167906f3f ("libs/guest: Move the guest ABI check earlier
> into xc_dom_parse_image()") broke starting the xenstore stubdom. This
> is due to a rather special way the xenstore stubdom domain config is
> being initialized: in order to support both, PV and PVH stubdom,
> init-xenstore-domain is using xc_dom_parse_image() to find the correct
> domain type. Unfortunately above commit requires xc_dom_boot_xen_init()
> to have been called before using xc_dom_parse_image(). This requires
> the domid, which is known only after xc_domain_create(), which requires
> the domain type.
>
> In order to break this circular dependency, call xc_dom_boot_xen_init()
> with an arbitrary domid first, and then set dom->guest_domid later.
>
> Fixes: 1787cc167906f3f ("libs/guest: Move the guest ABI check earlier into xc_dom_parse_image()")
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Release-acked-by: Ian Jackson <iwj@xenproject.org>

This is all rather nasty, and really highlights problems with the
libxenguest abi, because you really ought to be able to ask "what ELF
properties do I have on my hand" without an implicit "and try to start
building a VM using it" on the side.

I agree this is probably the best thing to do right now.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

On Thu, Nov 4, 2021 at 11:00 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 04/11/2021 14:42, Juergen Gross wrote:
> > Commit 1787cc167906f3f ("libs/guest: Move the guest ABI check earlier
> > into xc_dom_parse_image()") broke starting the xenstore stubdom. This
> > is due to a rather special way the xenstore stubdom domain config is
> > being initialized: in order to support both, PV and PVH stubdom,
> > init-xenstore-domain is using xc_dom_parse_image() to find the correct
> > domain type. Unfortunately above commit requires xc_dom_boot_xen_init()
> > to have been called before using xc_dom_parse_image(). This requires
> > the domid, which is known only after xc_domain_create(), which requires
> > the domain type.
> >
> > In order to break this circular dependency, call xc_dom_boot_xen_init()
> > with an arbitrary domid first, and then set dom->guest_domid later.
> >
> > Fixes: 1787cc167906f3f ("libs/guest: Move the guest ABI check earlier into xc_dom_parse_image()")
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> > Release-acked-by: Ian Jackson <iwj@xenproject.org>
>
> This is all rather nasty, and really highlights problems with the
> libxenguest abi, because you really ought to be able to ask "what ELF
> properties do I have on my hand" without an implicit "and try to start
> building a VM using it" on the side.
>
> I agree this is probably the best thing to do right now.

Yes, this is probably the best change before release.

If xc_dom_allocate() filled in dom->xen_version & dom->xen_caps - i.e.
move that from xc_dom_boot_xen_init() - then I think this patch
wouldn't be necessary.  But there could be side effects of such a
change.

Regards,
Jason

On 04.11.21 17:33, Jason Andryuk wrote:
> On Thu, Nov 4, 2021 at 11:00 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>
>> On 04/11/2021 14:42, Juergen Gross wrote:
>>> Commit 1787cc167906f3f ("libs/guest: Move the guest ABI check earlier
>>> into xc_dom_parse_image()") broke starting the xenstore stubdom. This
>>> is due to a rather special way the xenstore stubdom domain config is
>>> being initialized: in order to support both, PV and PVH stubdom,
>>> init-xenstore-domain is using xc_dom_parse_image() to find the correct
>>> domain type. Unfortunately above commit requires xc_dom_boot_xen_init()
>>> to have been called before using xc_dom_parse_image(). This requires
>>> the domid, which is known only after xc_domain_create(), which requires
>>> the domain type.
>>>
>>> In order to break this circular dependency, call xc_dom_boot_xen_init()
>>> with an arbitrary domid first, and then set dom->guest_domid later.
>>>
>>> Fixes: 1787cc167906f3f ("libs/guest: Move the guest ABI check earlier into xc_dom_parse_image()")
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> Release-acked-by: Ian Jackson <iwj@xenproject.org>
>>
>> This is all rather nasty, and really highlights problems with the
>> libxenguest abi, because you really ought to be able to ask "what ELF
>> properties do I have on my hand" without an implicit "and try to start
>> building a VM using it" on the side.
>>
>> I agree this is probably the best thing to do right now.
> 
> Yes, this is probably the best change before release.
> 
> If xc_dom_allocate() filled in dom->xen_version & dom->xen_caps - i.e.
> move that from xc_dom_boot_xen_init() - then I think this patch
> wouldn't be necessary.  But there could be side effects of such a
> change.

This is a nice idea for a cleanup patch after 4.16 has been branched.


Juergen