Re: [PATCH v2 10/23] xen/arm: compile and run xenbus

Stefano Stabellini
2012-08-08T17:42:33Z

On Wed, 8 Aug 2012, Daniel De Graaf wrote:
> On 08/08/2012 01:19 PM, Stefano Stabellini wrote:
> > On Wed, 8 Aug 2012, Daniel De Graaf wrote:
> >> On 08/08/2012 12:51 PM, Stefano Stabellini wrote:
> >>> On Tue, 7 Aug 2012, Daniel De Graaf wrote:
> >>>> On 08/07/2012 02:21 PM, Konrad Rzeszutek Wilk wrote:
> >>>>> On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote:
> >>>>>> bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not
> >>>>>> an error.
> >>>>>>
> >>>>>> If Linux is running as an HVM domain and is running as Dom0, use
> >>>>>> xenstored_local_init to initialize the xenstore page and event channel.
> >>>>>>
> >>>>>> Changes in v2:
> >>>>>>
> >>>>>> - refactor xenbus_init.
> >>>>>
> >>>>> Thank you. Lets also CC our friend at NSA who has been doing some work
> >>>>> in that area. Daniel are you OK with this change - will it still make
> >>>>> PV initial domain with with the MiniOS XenBus driver?
> >>>>>
> >>>>> Thanks.
> >>>>
> >>>> That case will work, but what this will break is launching the initial domain
> >>>> with a Xenstore stub domain already running (see below).
> >>>>
> >>>>>>
> >>>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>>>>> ---
> >>>>>> drivers/xen/xenbus/xenbus_comms.c | 2 +-
> >>>>>> drivers/xen/xenbus/xenbus_probe.c | 62 +++++++++++++++++++++++++-----------
> >>>>>> drivers/xen/xenbus/xenbus_xs.c | 1 +
> >>>>>> 3 files changed, 45 insertions(+), 20 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c
> >>>>>> index 52fe7ad..c5aa55c 100644
> >>>>>> --- a/drivers/xen/xenbus/xenbus_comms.c
> >>>>>> +++ b/drivers/xen/xenbus/xenbus_comms.c
> >>>>>> @@ -224,7 +224,7 @@ int xb_init_comms(void)
> >>>>>> int err;
> >>>>>> err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting,
> >>>>>> 0, "xenbus", &xb_waitq);
> >>>>>> - if (err <= 0) {
> >>>>>> + if (err < 0) {
> >>>>>> printk(KERN_ERR "XENBUS request irq failed %i\n", err);
> >>>>>> return err;
> >>>>>> }
> >>>>>
> >>>>>> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
> >>>>>> index b793723..a67ccc0 100644
> >>>>>> --- a/drivers/xen/xenbus/xenbus_probe.c
> >>>>>> +++ b/drivers/xen/xenbus/xenbus_probe.c
> >>>>>> @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void)
> >>>>>> return err;
> >>>>>> }
> >>>>>>
> >>>>>> +enum xenstore_init {
> >>>>>> + UNKNOWN,
> >>>>>> + PV,
> >>>>>> + HVM,
> >>>>>> + LOCAL,
> >>>>>> +};
> >>>>>> static int __init xenbus_init(void)
> >>>>>> {
> >>>>>> int err = 0;
> >>>>>> + enum xenstore_init usage = UNKNOWN;
> >>>>>> + uint64_t v = 0;
> >>>>>>
> >>>>>> if (!xen_domain())
> >>>>>> return -ENODEV;
> >>>>>>
> >>>>>> xenbus_ring_ops_init();
> >>>>>>
> >>>>>> - 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 {
> >>>>>> - xen_store_evtchn = xen_start_info->store_evtchn;
> >>>>>> - xen_store_mfn = xen_start_info->store_mfn;
> >>>>>> - if (xen_store_evtchn)
> >>>>>> - xenstored_ready = 1;
> >>>>>> - else {
> >>>>>> + if (xen_pv_domain())
> >>>>>> + usage = PV;
> >>>>>> + if (xen_hvm_domain())
> >>>>>> + usage = HVM;
> >>>>
> >>>> The above is correct for domUs, and is overridden for dom0s:
> >>>>
> >>>>>> + if (xen_hvm_domain() && xen_initial_domain())
> >>>>>> + usage = LOCAL;
> >>>>>> + if (xen_pv_domain() && !xen_start_info->store_evtchn)
> >>>>>> + usage = LOCAL;
> >>>>
> >>>> Instead of these checks, I think it should just be:
> >>>>
> >>>> if (!xen_start_info->store_evtchn)
> >>>> usage = LOCAL;
> >>>>
> >>>> Any domain started after xenstore will have store_evtchn set, so if you don't
> >>>> have this set, you are either going to be running xenstore locally, or will
> >>>> use the ioctl to change it later (and so should still set up everything as if
> >>>> it will be running locally).
> >>>
> >>> That would be wrong for an HVM dom0 domain (at least on ARM), because
> >>> we don't have a start_info page at all.
> >>>
> >>>
> >>>>>> + if (xen_pv_domain() && xen_start_info->store_evtchn)
> >>>>>> + xenstored_ready = 1;
> >>>>
> >>>> This part can now just be moved unconditionally into case PV.
> >>>
> >>> What about:
> >>>
> >>> if (xen_pv_domain())
> >>> usage = PV;
> >>> if (xen_hvm_domain())
> >>> usage = HVM;
> >>> if (!xen_store_evtchn)
> >>> usage = LOCAL;
> >>>
> >>> and moving xenstored_ready in case PV, like you suggested.
> >>>
> >>
> >> That looks correct, but you'd need to split up the switch statement in
> >> order to populate xen_store_evtchn before that last condition, which
> >> ends up pretty much eliminating the usage variable.
> >
> > Going back to what you wrote in the previous email, in what way this
> > patch breaks the case when an initial domain is started after a Xenstore
> > stub domain?
> >
> > Assuming that we are talking about a PV initial domain on x86, the
> > following check
> >
> > if (xen_pv_domain() && !xen_start_info->store_evtchn)
> > usage = LOCAL;
> >
> > will return false (because store_evtchn is set), therefore usage will
> > remain set to PV.
> > And the check:
> >
> > if (xen_pv_domain() && xen_start_info->store_evtchn)
> > xenstored_ready = 1;
> >
> > will return true so xenstored_ready is going to be set to 1.
> >
>
> Right, the original patch didn't break anything with PV domains. The case
> it doesn't handle is an HVM initial domain with an already-running
> Xenstore domain; I think this applies both to ARM and hybrid/PVH on x86.
> In that case, usage would be set to LOCAL instead of HVM.

Right, however if I am not mistaken there is no such thing as an HVM
dom0 right now on x86 and hybrid/PVH is probably going to return true on
xen_pv_domain() and false on xen_hvm_domain().

In the ARM case, given that we don't have a start_info page, we would
need another way to figure out whether a xenstore stub domain is already
running, so I think we can just postpone the solution of that problem
for now.