lkml.org 
[lkml]   [2012]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 10/23] xen/arm: compile and run xenbus
    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.



    \
     
     \ /
      Last update: 2012-08-08 19:43    [W:0.036 / U:3.792 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site