| Message ID | 20211104144242.14351-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH-for-4.16] tools/helpers: fix broken xenstore stubdom init | expand |
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
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 ) {
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(-)