| Message ID | 20220505002304.401417-2-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dom0less + PV drivers | expand |
Hi Luca, On 05.05.2022 02:23, Stefano Stabellini wrote: > From: Luca Miccio <lucmiccio@gmail.com> > > 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@gmail.com> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > --- > 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
On 05.05.22 14:26, Michal Orzel wrote: > Hi Luca, > > On 05.05.2022 02:23, Stefano Stabellini wrote: >> From: Luca Miccio <lucmiccio@gmail.com> >> >> 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@gmail.com> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >> --- >> 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
On 05.05.2022 14:29, Juergen Gross wrote: >>> - /* 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(); > } > > Good to know. So Luca, you can omit this comment. > Juergen Cheers, Michal
On Thu, 5 May 2022, Michal Orzel wrote: > On 05.05.2022 14:29, Juergen Gross wrote: > >>> - /* 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(); > > } > > > > > Good to know. So Luca, you can omit this comment. Thanks Michal, I addressed the other two comments.
On 5/4/22 8:23 PM, Stefano Stabellini wrote: > @@ -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. What I really meant (but not what I actually wrote I guess) was that now we are treating -1 differently than 0 and so that comment should go ... > */ > - if (!v || !~v) { > + if (!v) { > err = -ENOENT; > goto out_error; > } > - /* Avoid truncation on 32-bit. */ ... here. But this is ntpicking so for the series Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > + if (v == ~0ULL) { > + wait = true; > + } else { > + /* Avoid truncation on 32-bit. */
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; + 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) { + 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); + 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");