Daniel De Graaf
2011-Oct-05 14:28 UTC
[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. Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> --- drivers/xen/xenbus/xenbus_probe.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c index bd2f90c..82bf38c 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -712,7 +712,7 @@ static int __init xenbus_init(void) /* 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; err = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound, &alloc_unbound); -- 1.7.6.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Oct-06 17:53 UTC
Re: [Xen-devel] [PATCH] xenbus: Fix loopback event channel assuming domain 0
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).
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Daniel De Graaf
2011-Oct-06 18:32 UTC
Re: [Xen-devel] [PATCH] xenbus: Fix loopback event channel assuming domain 0
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.> 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. -- Daniel De Graaf National Security Agency _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Oct-07 07:52 UTC
Re: [Xen-devel] [PATCH] xenbus: Fix loopback event channel assuming domain 0
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. 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.> > > 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@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel De Graaf
2011-Oct-07 16:37 UTC
Re: [Xen-devel] [PATCH] xenbus: Fix loopback event channel assuming domain 0
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. 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?>>> 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@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Oct-10 12:54 UTC
Re: [Xen-devel] [PATCH] xenbus: Fix loopback event channel assuming domain 0
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@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel De Graaf
2011-Oct-11 14:52 UTC
Re: [Xen-devel] [PATCH] xenbus: Fix loopback event channel assuming domain 0
On 10/10/2011 08:54 AM, Ian Campbell wrote:> 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.Yes; splitting up driver domains requires changing a number of users of xen_initial_domain to become more fine-grained. Some disaggregation work[1] requires splitting the SIF_INITDOMAIN flag into a series of finer-grained flags that includes one for xenstore; this becomes unnecessary if xenstore detection code does not check SIF_INITDOMAIN. This patch covers a few cases: 1) Dom0 is Linux, xenstored runs in Dom0 2) Dom0 is domain builder, creating another SIF_INITDOMAIN Linux domain with a nonzero domain ID that runs xenstore and other functions 3) Dom0 is domain builder, creating xenstore and a SIF_INITDOMAIN Linux domain that uses the external xenstore. The second and third case require fairly intrusive hypervisor patches, but the only Linux change required for the second case is the posted fix to the loopback event channel. [1] "Breaking Up is Hard to Do: Security and Functionality in a Commodity Hypervisor" (SOSP 11)> >> 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. >This patch eliminates xen_initial_domain() checks when initializing xenstore, replacing them with checks on the event channel in the start_info page. --- diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c index bd2f90c..cef9b0b 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -684,64 +684,74 @@ static int __init xenbus_probe_initcall(void) device_initcall(xenbus_probe_initcall); -static int __init xenbus_init(void) +/* Set up event channel for xenstored which is run as a local process + * (this is normally used only in dom0) + */ +static int __init xenstored_local_init(void) { int err = 0; unsigned long page = 0; + struct evtchn_alloc_unbound alloc_unbound; - DPRINTK(""); + /* Allocate Xenstore page */ + page = get_zeroed_page(GFP_KERNEL); + if (!page) + goto out_err; - err = -ENODEV; - if (!xen_domain()) - return err; + xen_store_mfn = xen_start_info->store_mfn + pfn_to_mfn(virt_to_phys((void *)page) >> + PAGE_SHIFT); - /* - * Domain0 doesn''t have a store_evtchn or store_mfn yet. - */ - if (xen_initial_domain()) { - struct evtchn_alloc_unbound alloc_unbound; + /* Next allocate a local port which xenstored can bind to */ + alloc_unbound.dom = DOMID_SELF; + alloc_unbound.remote_dom = DOMID_SELF; - /* Allocate Xenstore page */ - page = get_zeroed_page(GFP_KERNEL); - if (!page) - goto out_error; + err = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound, + &alloc_unbound); + if (err == -ENOSYS) + goto out_err; - xen_store_mfn = xen_start_info->store_mfn - pfn_to_mfn(virt_to_phys((void *)page) >> - PAGE_SHIFT); + BUG_ON(err); + xen_store_evtchn = xen_start_info->store_evtchn + alloc_unbound.port; - /* Next allocate a local port which xenstored can bind to */ - alloc_unbound.dom = DOMID_SELF; - alloc_unbound.remote_dom = 0; + return 0; - err = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound, - &alloc_unbound); - if (err == -ENOSYS) - goto out_error; + out_err: + if (page != 0) + free_page(page); + return err; +} - BUG_ON(err); - xen_store_evtchn = xen_start_info->store_evtchn - alloc_unbound.port; +static int __init xenbus_init(void) +{ + int err = 0; - xen_store_interface = mfn_to_virt(xen_store_mfn); + if (!xen_domain()) + return -ENODEV; + + if (xen_hvm_domain()) { + uint64_t v = 0; + err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); + if (err) + goto out_error; + xen_store_evtchn = (int)v; + err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); + if (err) + goto out_error; + xen_store_mfn = (unsigned long)v; + xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); } else { - if (xen_hvm_domain()) { - uint64_t v = 0; - err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); - if (err) - goto out_error; - xen_store_evtchn = (int)v; - err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); + xen_store_evtchn = xen_start_info->store_evtchn; + xen_store_mfn = xen_start_info->store_mfn; + if (xen_store_evtchn) + xenstored_ready = 1; + else { + err = xenstored_local_init(); if (err) goto out_error; - xen_store_mfn = (unsigned long)v; - xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); - } else { - xen_store_evtchn = xen_start_info->store_evtchn; - xen_store_mfn = xen_start_info->store_mfn; - xen_store_interface = mfn_to_virt(xen_store_mfn); - xenstored_ready = 1; } + xen_store_interface = mfn_to_virt(xen_store_mfn); } /* Initialize the interface to xenstore. */ @@ -760,12 +770,7 @@ static int __init xenbus_init(void) proc_mkdir("xen", NULL); #endif - return 0; - - out_error: - if (page != 0) - free_page(page); - + out_error: return err; } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Oct-12 16:32 UTC
Re: [Xen-devel] [PATCH] xenbus: Fix loopback event channel assuming domain 0
On Tue, 2011-10-11 at 15:52 +0100, Daniel De Graaf wrote:> On 10/10/2011 08:54 AM, Ian Campbell wrote: > > 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. > > Yes; splitting up driver domains requires changing a number of users of > xen_initial_domain to become more fine-grained. Some disaggregation work[1] > requires splitting the SIF_INITDOMAIN flag into a series of finer-grained > flags that includes one for xenstore; this becomes unnecessary if xenstore > detection code does not check SIF_INITDOMAIN. > > This patch covers a few cases: > > 1) Dom0 is Linux, xenstored runs in Dom0 > 2) Dom0 is domain builder, creating another SIF_INITDOMAIN Linux domain with > a nonzero domain ID that runs xenstore and other functions > 3) Dom0 is domain builder, creating xenstore and a SIF_INITDOMAIN Linux > domain that uses the external xenstore. > > The second and third case require fairly intrusive hypervisor patches,What sort of hypervisor patches? I''m not convinced that the concept of "another SIF_INITDOMAIN" is possible. Certainly you might have other domains which have some privileges which have previously been associated with the domain whose ID is 0, but that does not imply that you should/can literally pass SIF_INITDOMAIN to those domains.> but > the only Linux change required for the second case is the posted fix to the > loopback event channel.Unless dom0 is also running Linux? In which case it has no way to talk to the xenstored in the second domain? Does the kernel the the "another SIF_INITDOMAIN" not get quite upset wrt it''s ability to see physical hardware and such, which it will be confused about because you''ve given it SIF_INITDOMAIN?> [1] "Breaking Up is Hard to Do: Security and Functionality in a Commodity > Hypervisor" (SOSP 11) > > > > >> 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. > > > > This patch eliminates xen_initial_domain() checks when initializing > xenstore, replacing them with checks on the event channel in the > start_info page.In your scenario 2 and 3 then with this patch the dom0 kernel will see no evtchn in start_info page but there appears to be no mechanism for poking down the evtchn/mfn for dom0 to use to communicate with the xenstored domain. I think that a combination of this patch and Diego''s stuff (reapplied to your xenstored_local_init case) would catch all those combinations and work for both the real dom0 (with or without xenstored in it) and also a domN Linux domain running xenstored too. I think this patch by itself doesn''t make any existing cases wrong nor make adding Diego''s stuff in the future any harder. How does the xenstored running in the second domain get the necessary page/evtchn numbers to allow it to communicate with dom0? I assume it is guaranteed that xen_start_info->store_evtchn == 0 (and presumably xen_start_info->store_mfn == 0) for the real dom0? Ian.> > --- > > diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c > index bd2f90c..cef9b0b 100644 > --- a/drivers/xen/xenbus/xenbus_probe.c > +++ b/drivers/xen/xenbus/xenbus_probe.c > @@ -684,64 +684,74 @@ static int __init xenbus_probe_initcall(void) > > device_initcall(xenbus_probe_initcall); > > -static int __init xenbus_init(void) > +/* Set up event channel for xenstored which is run as a local process > + * (this is normally used only in dom0) > + */ > +static int __init xenstored_local_init(void) > { > int err = 0; > unsigned long page = 0; > + struct evtchn_alloc_unbound alloc_unbound; > > - DPRINTK(""); > + /* Allocate Xenstore page */ > + page = get_zeroed_page(GFP_KERNEL); > + if (!page) > + goto out_err; > > - err = -ENODEV; > - if (!xen_domain()) > - return err; > + xen_store_mfn = xen_start_info->store_mfn > + pfn_to_mfn(virt_to_phys((void *)page) >> > + PAGE_SHIFT); > > - /* > - * Domain0 doesn''t have a store_evtchn or store_mfn yet. > - */ > - if (xen_initial_domain()) { > - struct evtchn_alloc_unbound alloc_unbound; > + /* Next allocate a local port which xenstored can bind to */ > + alloc_unbound.dom = DOMID_SELF; > + alloc_unbound.remote_dom = DOMID_SELF; > > - /* Allocate Xenstore page */ > - page = get_zeroed_page(GFP_KERNEL); > - if (!page) > - goto out_error; > + err = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound, > + &alloc_unbound); > + if (err == -ENOSYS) > + goto out_err; > > - xen_store_mfn = xen_start_info->store_mfn > - pfn_to_mfn(virt_to_phys((void *)page) >> > - PAGE_SHIFT); > + BUG_ON(err); > + xen_store_evtchn = xen_start_info->store_evtchn > + alloc_unbound.port; > > - /* Next allocate a local port which xenstored can bind to */ > - alloc_unbound.dom = DOMID_SELF; > - alloc_unbound.remote_dom = 0; > + return 0; > > - err = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound, > - &alloc_unbound); > - if (err == -ENOSYS) > - goto out_error; > + out_err: > + if (page != 0) > + free_page(page); > + return err; > +} > > - BUG_ON(err); > - xen_store_evtchn = xen_start_info->store_evtchn > - alloc_unbound.port; > +static int __init xenbus_init(void) > +{ > + int err = 0; > > - xen_store_interface = mfn_to_virt(xen_store_mfn); > + if (!xen_domain()) > + return -ENODEV; > + > + if (xen_hvm_domain()) { > + uint64_t v = 0; > + err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); > + if (err) > + goto out_error; > + xen_store_evtchn = (int)v; > + err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); > + if (err) > + goto out_error; > + xen_store_mfn = (unsigned long)v; > + xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); > } else { > - if (xen_hvm_domain()) { > - uint64_t v = 0; > - err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v); > - if (err) > - goto out_error; > - xen_store_evtchn = (int)v; > - err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v); > + xen_store_evtchn = xen_start_info->store_evtchn; > + xen_store_mfn = xen_start_info->store_mfn; > + if (xen_store_evtchn) > + xenstored_ready = 1; > + else { > + err = xenstored_local_init(); > if (err) > goto out_error; > - xen_store_mfn = (unsigned long)v; > - xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE); > - } else { > - xen_store_evtchn = xen_start_info->store_evtchn; > - xen_store_mfn = xen_start_info->store_mfn; > - xen_store_interface = mfn_to_virt(xen_store_mfn); > - xenstored_ready = 1; > } > + xen_store_interface = mfn_to_virt(xen_store_mfn); > } > > /* Initialize the interface to xenstore. */ > @@ -760,12 +770,7 @@ static int __init xenbus_init(void) > proc_mkdir("xen", NULL); > #endif > > - return 0; > - > - out_error: > - if (page != 0) > - free_page(page); > - > + out_error: > return err; > } > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel De Graaf
2011-Oct-12 18:47 UTC
Re: [Xen-devel] [PATCH] xenbus: Fix loopback event channel assuming domain 0
On 10/12/2011 12:32 PM, Ian Campbell wrote:> On Tue, 2011-10-11 at 15:52 +0100, Daniel De Graaf wrote: >> On 10/10/2011 08:54 AM, Ian Campbell wrote: >>> 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. >> >> Yes; splitting up driver domains requires changing a number of users of >> xen_initial_domain to become more fine-grained. Some disaggregation work[1] >> requires splitting the SIF_INITDOMAIN flag into a series of finer-grained >> flags that includes one for xenstore; this becomes unnecessary if xenstore >> detection code does not check SIF_INITDOMAIN. >> >> This patch covers a few cases: >> >> 1) Dom0 is Linux, xenstored runs in Dom0 >> 2) Dom0 is domain builder, creating another SIF_INITDOMAIN Linux domain with >> a nonzero domain ID that runs xenstore and other functions >> 3) Dom0 is domain builder, creating xenstore and a SIF_INITDOMAIN Linux >> domain that uses the external xenstore. >> >> The second and third case require fairly intrusive hypervisor patches, > > What sort of hypervisor patches? > > I''m not convinced that the concept of "another SIF_INITDOMAIN" is > possible. Certainly you might have other domains which have some > privileges which have previously been associated with the domain whose > ID is 0, but that does not imply that you should/can literally pass > SIF_INITDOMAIN to those domains. >The patches add a "hardware domain" parameter which is passed control of the hardware instead of dom0. This domain is in charge of most of the ACPI and other nonspecific hardware devices, while individual PCI devices will be passed to other driver domains (that are not SIF_INITDOMAIN). The domain with ID 0 only acts as a domain builder here: it does not directly access the hardware, only creating and activating other domains. This allows domains (including xenstore and a few others) to start before the Linux domain accessing the hardware has started any part of userspace.>> but >> the only Linux change required for the second case is the posted fix to the >> loopback event channel. > > Unless dom0 is also running Linux? In which case it has no way to talk > to the xenstored in the second domain?Correct; I am not addressing this problem as dom0 is not running Linux here. The other patches you pointed to do address that possibility with the ioctl, which seems a good solution if you want to also run Linux in dom0 - although in that case, dom0 may need to be marked as not SIF_INITDOMAIN to avoid trying to set up hardware twice.> Does the kernel the the "another SIF_INITDOMAIN" not get quite upset wrt > it''s ability to see physical hardware and such, which it will be > confused about because you''ve given it SIF_INITDOMAIN?The hypervisor patches actually give it the access required.>> [1] "Breaking Up is Hard to Do: Security and Functionality in a Commodity >> Hypervisor" (SOSP 11) >> >>> >>>> 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. >>> >> >> This patch eliminates xen_initial_domain() checks when initializing >> xenstore, replacing them with checks on the event channel in the >> start_info page. > > In your scenario 2 and 3 then with this patch the dom0 kernel will see > no evtchn in start_info page but there appears to be no mechanism for > poking down the evtchn/mfn for dom0 to use to communicate with the > xenstored domain. > > I think that a combination of this patch and Diego''s stuff (reapplied to > your xenstored_local_init case) would catch all those combinations and > work for both the real dom0 (with or without xenstored in it) and also a > domN Linux domain running xenstored too. I think this patch by itself > doesn''t make any existing cases wrong nor make adding Diego''s stuff in > the future any harder. > > How does the xenstored running in the second domain get the necessary > page/evtchn numbers to allow it to communicate with dom0?In my setup, it doesn''t ever communicate with dom0 as dom0 dies once it has set up the boot domains. For a more general case, the page/evtchn numbers could be passed in a normal introduce message if they are made available outside dom0 (perhaps by command-line parameters or via another mechanism like v4v).> I assume it is guaranteed that xen_start_info->store_evtchn == 0 (and > presumably xen_start_info->store_mfn == 0) for the real dom0?Yes; the start_info page is zeroed prior to filling it in for dom0, and these fields are not filled in. -- Daniel De Graaf National Security Agency _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Oct-13 09:24 UTC
Re: [Xen-devel] [PATCH] xenbus: Fix loopback event channel assuming domain 0
> >> but > >> the only Linux change required for the second case is the posted fix to the > >> loopback event channel. > > > > Unless dom0 is also running Linux? In which case it has no way to talk > > to the xenstored in the second domain? > > Correct; I am not addressing this problem as dom0 is not running Linux here.Sure, my concern was that we don''t paint ourselves into a corner wrt future moves in this direction. I think I''m now convinced that this isn''t the case and that everything is fine (thanks!).> The other patches you pointed to do address that possibility with the ioctl, > which seems a good solution if you want to also run Linux in dom0Agreed, I think the ioctl path will fit into your "xenstored_local_init()" path just the same as it fits into the current code.> > How does the xenstored running in the second domain get the necessary > > page/evtchn numbers to allow it to communicate with dom0? > > In my setup, it doesn''t ever communicate with dom0 as dom0 dies once it > has set up the boot domains. For a more general case, the page/evtchn > numbers could be passed in a normal introduce message if they are made > available outside dom0 (perhaps by command-line parameters or via another > mechanism like v4v).That rings a bell -- I think Deigo had them on the xenstored domain command line.> > I assume it is guaranteed that xen_start_info->store_evtchn == 0 (and > > presumably xen_start_info->store_mfn == 0) for the real dom0? > > Yes; the start_info page is zeroed prior to filling it in for dom0, and > these fields are not filled in.Great. The representation of your changes which diff chose was not terribly helpful for seeing the trees in the woods but I applied it and reviewed the result and it looks ok to me. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Oct-13 18:24 UTC
Re: [Xen-devel] [PATCH] xenbus: Fix loopback event channel assuming domain 0
On Thu, Oct 13, 2011 at 10:24:34AM +0100, Ian Campbell wrote:> > >> but > > >> the only Linux change required for the second case is the posted fix to the > > >> loopback event channel. > > > > > > Unless dom0 is also running Linux? In which case it has no way to talk > > > to the xenstored in the second domain? > > > > Correct; I am not addressing this problem as dom0 is not running Linux here. > > Sure, my concern was that we don''t paint ourselves into a corner wrt > future moves in this direction. I think I''m now convinced that this > isn''t the case and that everything is fine (thanks!). > > > The other patches you pointed to do address that possibility with the ioctl, > > which seems a good solution if you want to also run Linux in dom0 > > Agreed, I think the ioctl path will fit into your > "xenstored_local_init()" path just the same as it fits into the current > code. > > > > How does the xenstored running in the second domain get the necessary > > > page/evtchn numbers to allow it to communicate with dom0? > > > > In my setup, it doesn''t ever communicate with dom0 as dom0 dies once it > > has set up the boot domains. For a more general case, the page/evtchn > > numbers could be passed in a normal introduce message if they are made > > available outside dom0 (perhaps by command-line parameters or via another > > mechanism like v4v). > > That rings a bell -- I think Deigo had them on the xenstored domain > command line. > > > > I assume it is guaranteed that xen_start_info->store_evtchn == 0 (and > > > presumably xen_start_info->store_mfn == 0) for the real dom0? > > > > Yes; the start_info page is zeroed prior to filling it in for dom0, and > > these fields are not filled in. > > Great. > > The representation of your changes which diff chose was not terribly > helpful for seeing the trees in the woods but I applied it and reviewed > the result and it looks ok to me.Daniel, Could you please repost these two patches with Ian''s Reviewed-by and rebase it on top of 3.1-rc9 please? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel De Graaf
2011-Oct-13 20:07 UTC
[Xen-devel] [PATCH 1/2] 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. Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> Reviewed-by: Ian Campbell <Ian.Campbell@citrix.com> --- drivers/xen/xenbus/xenbus_probe.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c index bd2f90c..82bf38c 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -712,7 +712,7 @@ static int __init xenbus_init(void) /* 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; err = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound, &alloc_unbound); -- 1.7.6.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel De Graaf
2011-Oct-13 20:07 UTC
[Xen-devel] [PATCH 2/2] xenbus: don''t rely on xen_initial_domain to detect local xenstore
The xenstore daemon does not have to run in the xen initial domain;
however, Linux currently uses xen_initial_domain to test if a loopback
event channel should be used instead of the event channel provided in
Xen''s start_info structure. Instead, if the event channel passed in the
start_info structure is not valid, assume that this domain will run
xenstored locally and set up the event channel.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Reviewed-by: Ian Campbell <Ian.Campbell@citrix.com>
---
drivers/xen/xenbus/xenbus_probe.c | 101 +++++++++++++++++++-----------------
1 files changed, 53 insertions(+), 48 deletions(-)
diff --git a/drivers/xen/xenbus/xenbus_probe.c
b/drivers/xen/xenbus/xenbus_probe.c
index 82bf38c..cef9b0b 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -684,64 +684,74 @@ static int __init xenbus_probe_initcall(void)
device_initcall(xenbus_probe_initcall);
-static int __init xenbus_init(void)
+/* Set up event channel for xenstored which is run as a local process
+ * (this is normally used only in dom0)
+ */
+static int __init xenstored_local_init(void)
{
int err = 0;
unsigned long page = 0;
+ struct evtchn_alloc_unbound alloc_unbound;
- DPRINTK("");
+ /* Allocate Xenstore page */
+ page = get_zeroed_page(GFP_KERNEL);
+ if (!page)
+ goto out_err;
- err = -ENODEV;
- if (!xen_domain())
- return err;
+ xen_store_mfn = xen_start_info->store_mfn + pfn_to_mfn(virt_to_phys((void
*)page) >>
+ PAGE_SHIFT);
- /*
- * Domain0 doesn''t have a store_evtchn or store_mfn yet.
- */
- if (xen_initial_domain()) {
- struct evtchn_alloc_unbound alloc_unbound;
+ /* Next allocate a local port which xenstored can bind to */
+ alloc_unbound.dom = DOMID_SELF;
+ alloc_unbound.remote_dom = DOMID_SELF;
- /* Allocate Xenstore page */
- page = get_zeroed_page(GFP_KERNEL);
- if (!page)
- goto out_error;
+ err = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound,
+ &alloc_unbound);
+ if (err == -ENOSYS)
+ goto out_err;
- xen_store_mfn = xen_start_info->store_mfn -
pfn_to_mfn(virt_to_phys((void *)page) >>
- PAGE_SHIFT);
+ BUG_ON(err);
+ xen_store_evtchn = xen_start_info->store_evtchn + alloc_unbound.port;
- /* Next allocate a local port which xenstored can bind to */
- alloc_unbound.dom = DOMID_SELF;
- alloc_unbound.remote_dom = DOMID_SELF;
+ return 0;
- err = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound,
- &alloc_unbound);
- if (err == -ENOSYS)
- goto out_error;
+ out_err:
+ if (page != 0)
+ free_page(page);
+ return err;
+}
- BUG_ON(err);
- xen_store_evtchn = xen_start_info->store_evtchn - alloc_unbound.port;
+static int __init xenbus_init(void)
+{
+ int err = 0;
- xen_store_interface = mfn_to_virt(xen_store_mfn);
+ if (!xen_domain())
+ return -ENODEV;
+
+ if (xen_hvm_domain()) {
+ uint64_t v = 0;
+ err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
+ if (err)
+ goto out_error;
+ xen_store_evtchn = (int)v;
+ err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
+ if (err)
+ goto out_error;
+ xen_store_mfn = (unsigned long)v;
+ xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE);
} else {
- if (xen_hvm_domain()) {
- uint64_t v = 0;
- err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
- if (err)
- goto out_error;
- xen_store_evtchn = (int)v;
- err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
+ xen_store_evtchn = xen_start_info->store_evtchn;
+ xen_store_mfn = xen_start_info->store_mfn;
+ if (xen_store_evtchn)
+ xenstored_ready = 1;
+ else {
+ err = xenstored_local_init();
if (err)
goto out_error;
- xen_store_mfn = (unsigned long)v;
- xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE);
- } else {
- xen_store_evtchn = xen_start_info->store_evtchn;
- xen_store_mfn = xen_start_info->store_mfn;
- xen_store_interface = mfn_to_virt(xen_store_mfn);
- xenstored_ready = 1;
}
+ xen_store_interface = mfn_to_virt(xen_store_mfn);
}
/* Initialize the interface to xenstore. */
@@ -760,12 +770,7 @@ static int __init xenbus_init(void)
proc_mkdir("xen", NULL);
#endif
- return 0;
-
- out_error:
- if (page != 0)
- free_page(page);
-
+ out_error:
return err;
}
--
1.7.6.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel