Re: [Xen-devel] [PATCH] xenbus: Fix loopback event channel assuming domain 0

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2011-10-07 at 17:37 +0100, Daniel De Graaf wrote:
> On 10/07/2011 03:52 AM, Ian Campbell wrote:
> > On Thu, 2011-10-06 at 19:32 +0100, Daniel De Graaf wrote:
> >> On 10/06/2011 01:53 PM, Ian Jackson wrote:
> >>> Daniel De Graaf writes ("[Xen-devel] [PATCH] xenbus: Fix loopback event 
> >>> channel assuming domain 0"):
> >>>> The xenbus event channel established in xenbus_init is intended to be a
> >>>> loopback channel, but the remote domain was hardcoded to 0; this will
> >>>> cause the channel to be unusable when xenstore is not being run in
> >>>> domain 0.
> >>>
> >>> I'm not sure I understand this.
> >>>
> >>> ...
> >>>>                  /* Next allocate a local port which xenstored can bind 
> >>>> to */
> >>>>                  alloc_unbound.dom        = DOMID_SELF;
> >>>> -                alloc_unbound.remote_dom = 0;
> >>>> +                alloc_unbound.remote_dom = DOMID_SELF;
> >>>
> >>> The comment doesn't suggest that this is supposedly a loopback channel
> >>> (ie one for use by the xenbus client for signalling to itself,
> >>> somehow).
> >>
> >> The event channel being changed here is a loopback event channel exposed in
> >> /proc/xen/xsd_port, which xenstored binds to. This code is only used for 
> >> the
> >> initial domain; otherwise, the shared info page is used.
> > 
> > How does this change impact the regular dom0? It will be expecting a
> > xenstored to startup locally when in reality it actually needs to wait
> > for another domain and then connect to that.
> 
> This change does not attempt to address the regular dom0, except for not
> breaking existing setups where xenstored resides in dom0.
> 
> > Diego Ongaro did some work several years ago on this issue, it was most
> > recently re-posted by Alex Zeffert, patches against xen-unstable and the
> > linux-2.6.18 tree:
> > http://lists.xensource.com/archives/html/xen-devel/2009-03/msg01484.html
> > http://lists.xensource.com/archives/html/xen-devel/2009-03/msg01488.html
> > 
> > Part of the trick was to fixup the kernel side in a way which was
> > compatible with both existing Xen releases while also supporting new
> > releases which support both stub and non-stub xenstore. To do this Diego
> > setup a lazy xenbus initialisation with a state machine to track which
> > case was active, with transitions triggered either from the local-mmap
> > of /proc/xen/xenbus_xsd (so was backwards compatible) or an ioctl called
> > by the tool which builds the stub domain to tell the dom0 xenbus code
> > which domain/mfn/evtchn contains the xenstored and dom0's connection to
> > it (the patcheset includes a cut-down builder which works without
> > xenstore).
> > 
> > http://lists.xensource.com/archives/html/xen-devel/2009-03/msg01487.html
> > is the key kernel side patch in that regard.
> > 
> > Diego did some initial work with xenstored in a Linux domU, but I think
> > the final patchset was stub-xenstored (i.e. ocaml xenstored on minios,
> > possibly C xenstored on minios too), so I'm not sure about the xenstored
> > in Linux domU use case.
> > 
> > Some of the more trivial bits of this series were committed but the real
> > meat wasn't really pushed through.
> > 
> 
> Thanks for pointing out that series; I hadn't seen it yet. The setup I am
> currently using has a non-Linux dom0, so the state machine in dom0 was not
> required. A separate minios-based xenstored is the eventual goal; this patch
> just avoids creating a broken event channel in an initial domain whose
> domain ID is not 0.

Although I suspect it was envisaged when the API was written I don't
think SIF_INITDOMAIN can actually be used (or redefined) to mean
anything other than dom0 in practice without a whole host of knock-on
effects and breakage.

Setting SIF_INITDOMAIN has effects other than xenstore setup on a Linux
domU, grepping for other uses of xen_initial_domain() shows loads of
them. e.g. the selection of host vs. pseudo-physical e820, various
driver setup stuff, some pagetable features, how the console works etc.

> I do have a more complex version of this patch that replaces the initial
> domain check with a check on the start_info structure so that an initial
> domain can have xenstore information placed in its start_info field like
> any other domain; would this be of interest?

If you already have something then it would be interesting to see.

Ian.

> 
> >>> Rather it's supposed to be a channel to xenstore.  So the remote
> >>> domain should be the xenstore domain, which should come from the
> >>> shared info page.
> >>>
> >>> Have you actually tested this with a separate xenstored domain ?
> >>>
> >>> Ian.
> >>>
> >>
> >> The test setup that exposed this issue is having a non-dom0 Linux domain
> >> running xenstored (in addition to other services); this domain is started
> >> with the SIF_INITDOMAIN flag set. It has been tested and can start other
> >> domains with references back to the xenstored running there; the local
> >> kernel is able to communicate with the locally running xenstore to provide
> >> backend services.
> >>
> >> The test for xen_initial_domain() here might better be replaced with a
> >> check on xen_start_info->store_evtchn which should be a valid event channel
> >> on all domains except the domain running xenstored. This seems like a more
> >> elegant solution than relying on the SIF_INITDOMAIN flag to determine the
> >> location of xenstore.
> >>
> > 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel