Re: [PATCH v4 2/2] xen: add support for initializing xenstore later as HVM domain

[Juergen Gross <jgross@xxxxxxxx>]

On 05.05.22 14:26, Michal Orzel wrote:
> Hi Luca,
>
> On 05.05.2022 02:23, Stefano Stabellini wrote:
> > From: Luca Miccio <lucmiccio@xxxxxxxxx>
> >
> > When running as dom0less guest (HVM domain on ARM) the xenstore event
> > channel is available at domain creation but the shared xenstore
> > interface page only becomes available later on.
> >
> > In that case, wait for a notification on the xenstore event channel,
> > then complete the xenstore initialization later, when the shared page
> > is actually available.
> >
> > The xenstore page has few extra field. Add them to the shared struct.
> > One of the field is "connection", when the connection is ready, it is
> > zero. If the connection is not-zero, wait for a notification.
> >
> > Signed-off-by: Luca Miccio <lucmiccio@xxxxxxxxx>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > ---
> > Changes in v4:
> > - improve in-code comments
> > - move header sync to separate patch
> > - use XENSTORE_CONNECTED
> >
> > Changes in v3:
> > - check for the connection field, if it is not zero, wait for event
> >
> > Changes in v2:
> > - remove XENFEAT_xenstore_late_init
> > ---
> >   drivers/xen/xenbus/xenbus_probe.c | 91 ++++++++++++++++++++++++-------
> >   1 file changed, 71 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/xen/xenbus/xenbus_probe.c 
> > b/drivers/xen/xenbus/xenbus_probe.c
> > index fe360c33ce71..0a785d5e3e40 100644
> > --- a/drivers/xen/xenbus/xenbus_probe.c
> > +++ b/drivers/xen/xenbus/xenbus_probe.c
> > @@ -65,6 +65,7 @@
> >   #include "xenbus.h"
> >   
> >   
> > +static int xs_init_irq;
> >   int xen_store_evtchn;
> >   EXPORT_SYMBOL_GPL(xen_store_evtchn);
> >   
> > @@ -750,6 +751,20 @@ static void xenbus_probe(void)
> >   {
> >         xenstored_ready = 1;
> >   
> > +	if (!xen_store_interface) {
> > +               xen_store_interface = xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,
> > +                                               XEN_PAGE_SIZE);
> > +               /*
> > +                * Now it is safe to free the IRQ used for xenstore late
> > +                * initialization. No need to unbind: it is about to be
> > +                * bound again from xb_init_comms. Note that calling
> > +                * unbind_from_irqhandler now would result in xen_evtchn_close()
> > +                * being called and the event channel not being enabled again
> > +                * afterwards, resulting in missed event notifications.
> > +                */
> > +               free_irq(xs_init_irq, &xb_waitq);
> > +       }
> > +
> >         /*
> >          * In the HVM case, xenbus_init() deferred its call to
> >          * xs_init() in case callbacks were not operational yet.
> > @@ -798,20 +813,22 @@ static int __init xenbus_probe_initcall(void)
> >   {
> >         /*
> >          * Probe XenBus here in the XS_PV case, and also XS_HVM unless we
> > -        * need to wait for the platform PCI device to come up.
> > +        * need to wait for the platform PCI device to come up or
> > +        * xen_store_interface is not ready.
> >          */
> >         if (xen_store_domain_type == XS_PV ||
> >             (xen_store_domain_type == XS_HVM &&
> > -            !xs_hvm_defer_init_for_callback()))
> > +            !xs_hvm_defer_init_for_callback() &&
> > +            xen_store_interface != NULL))
> >                 xenbus_probe();
> >   
> >   	/*
> > -        * For XS_LOCAL, spawn a thread which will wait for xenstored
> > -        * or a xenstore-stubdom to be started, then probe. It will be
> > -        * triggered when communication starts happening, by waiting
> > -        * on xb_waitq.
> > +        * For XS_LOCAL or when xen_store_interface is not ready, spawn a
> > +        * thread which will wait for xenstored or a xenstore-stubdom to be
> > +        * started, then probe.  It will be triggered when communication
> > +        * starts happening, by waiting on xb_waitq.
> >          */
> > -       if (xen_store_domain_type == XS_LOCAL) {
> > +       if (xen_store_domain_type == XS_LOCAL || xen_store_interface == NULL) {
> >                 struct task_struct *probe_task;
> >   
> >   		probe_task = kthread_run(xenbus_probe_thread, NULL,
> > @@ -907,10 +924,25 @@ static struct notifier_block xenbus_resume_nb = {
> >         .notifier_call = xenbus_resume_cb,
> >   };
> >   
> > +static irqreturn_t xenbus_late_init(int irq, void *unused)
> > +{
> > +       int err = 0;
> No need to set up initial value as it is being reassigned without using the 
> initial value.
>
> > +       uint64_t v = 0;
> > +
> > +       err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
> > +       if (err || !v || !~v)
> > +               return IRQ_HANDLED;
> > +       xen_store_gfn = (unsigned long)v;
> > +
> > +       wake_up(&xb_waitq);
> > +       return IRQ_HANDLED;
> > +}
> > +
> >   static int __init xenbus_init(void)
> >   {
> >         int err;
> >         uint64_t v = 0;
> > +       bool wait = false;
> >         xen_store_domain_type = XS_UNKNOWN;
> >   
> >   	if (!xen_domain())
> > @@ -957,25 +989,44 @@ static int __init xenbus_init(void)
> >                  * been properly initialized. Instead of attempting to map a
> >                  * wrong guest physical address return error.
> >                  *
> > -                * Also recognize all bits set as an invalid value.
> > +                * Also recognize all bits set as an invalid/uninitialized 
> > value.
> >                  */
> > -               if (!v || !~v) {
> > +               if (!v) {
> >                         err = -ENOENT;
> >                         goto out_error;
> >                 }
> > -               /* Avoid truncation on 32-bit. */
> > +               if (v == ~0ULL) {
> No need for brackets for a single instruction.
The coding style says otherwise:

  This does not apply if only one branch of a conditional statement is a single
  statement; in the latter case use braces in both branches:

  .. code-block:: c

        if (condition) {
                do_this();
                do_that();
        } else {
                otherwise();
        }


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature