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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> >>>>>> ---
> >>>>>>  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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel