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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home