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

[Michal Orzel <michal.orzel@xxxxxxx>]

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.

> +                     wait = true;
> +             } else {
> +                     /* Avoid truncation on 32-bit. */
>  #if BITS_PER_LONG == 32
> -             if (v > ULONG_MAX) {
> -                     pr_err("%s: cannot handle HVM_PARAM_STORE_PFN=%llx > 
> ULONG_MAX\n",
> -                            __func__, v);
> -                     err = -EINVAL;
> -                     goto out_error;
> -             }
> +                     if (v > ULONG_MAX) {
> +                             pr_err("%s: cannot handle 
> HVM_PARAM_STORE_PFN=%llx > ULONG_MAX\n",
> +                                             __func__, v);
__func shall be aligned with "%s... as it was before.

> +                             err = -EINVAL;
> +                             goto out_error;
> +                     }
>  #endif
> -             xen_store_gfn = (unsigned long)v;
> -             xen_store_interface =
> -                     xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,
> -                               XEN_PAGE_SIZE);
> +                     xen_store_gfn = (unsigned long)v;
> +                     xen_store_interface =
> +                             xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,
> +                                       XEN_PAGE_SIZE);
> +                     if (xen_store_interface->connection != 
> XENSTORE_CONNECTED)
> +                             wait = true;
> +             }
> +             if (wait) {
> +                     err = bind_evtchn_to_irqhandler(xen_store_evtchn,
> +                                                     xenbus_late_init,
> +                                                     0, "xenstore_late_init",
> +                                                     &xb_waitq);
> +                     if (err < 0) {
> +                             pr_err("xenstore_late_init couldn't bind irq 
> err=%d\n",
> +                                    err);
> +                             return err;
> +                     }
> +
> +                     xs_init_irq = err;
> +             }
>               break;
>       default:
>               pr_warn("Xenstore state unknown\n");

Cheers,
Michal