Re: [LINUX PATCH v3] xen: add support for initializing xenstore later as HVM domain

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 29 Apr 2022, Boris Ostrovsky wrote:
> On 4/29/22 5:10 PM, 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>
> > CC: jgross@xxxxxxxx
> > CC: boris.ostrovsky@xxxxxxxxxx
> > ---
> > 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  | 86 +++++++++++++++++++++++-------
> >   include/xen/interface/io/xs_wire.h |  3 ++
> >   2 files changed, 70 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/xen/xenbus/xenbus_probe.c
> > b/drivers/xen/xenbus/xenbus_probe.c
> > index fe360c33ce71..dc046d25789e 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,17 @@ 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.
> 
> 
> This assumes knowledge of bind/unbind internals. I think we should unbind.

I gave it a try and there is a problem with unbinding the IRQ here. If I
do that, later when xb_init_comms calls bind_evtchn_to_irqhandler, the
event channel doesn't fire anymore. I did some testing and debugging and
as far as I can tell the issue is that if we call unbind_from_irqhandler
here, we end up calling xen_evtchn_close. Then, when xb_init_comms calls
bind_evtchn_to_irqhandler on the same evtchn, it is not enough to
receive event notifications anymore because from Xen POV the evtchn is
"closed".

If I comment out xen_evtchn_close() in __unbind_from_irq, then yes, I
can call unbind_from_irqhandler here instead of free_irq and everything
works.

My suggestion is to keep the call to free_irq here (not
unbind_from_irqhandler) and improve the in-code comment.

 
> > +            */
> > +           free_irq(xs_init_irq, &xb_waitq);
> > +   }
> > +
> 
> 
> 
> > @@ -959,23 +988,42 @@ static int __init xenbus_init(void)
> >              *
> >              * Also recognize all bits set as an invalid value.
> 
> 
> Is this comment still correct?

I can improve the comment

 
> >              */
> > -           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. */