Re: [PATCH] drivers/xen: Improve the late XenStore init protocol

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Stefano,

On 5/17/2024 8:52 AM, Stefano Stabellini wrote:
> On Thu, 16 May 2024, Henry Wang wrote:
> > > >    enum xenstore_init xen_store_domain_type;
> > > >    EXPORT_SYMBOL_GPL(xen_store_domain_type);
> > > >    @@ -751,9 +755,10 @@ static void xenbus_probe(void)
> > > >    {
> > > >         xenstored_ready = 1;
> > > >    -    if (!xen_store_interface) {
> > > > -               xen_store_interface = memremap(xen_store_gfn <<
> > > > XEN_PAGE_SHIFT,
> > > > -                                              XEN_PAGE_SIZE, MEMREMAP_WB);
> > > > +       if (!xen_store_interface || XS_INTERFACE_READY) {
> > > > +               if (!xen_store_interface)
> > > These two nested if's don't make sense to me. If XS_INTERFACE_READY
> > > succeeds, it means that  ((xen_store_interface != NULL) &&
> > > (xen_store_interface->connection == XENSTORE_CONNECTED)).
> > >
> > > So it is not possible that xen_store_interface == NULL immediately
> > > after. Right?
> > I think this is because we want to free the irq for the late init case,
> > otherwise the init-dom0less will fail. For the xenstore PFN allocated case,
> > the connection is already set to CONNECTED when we execute init-dom0less. But
> > I agree with you, would below diff makes more sense to you?
> >
> > diff --git a/drivers/xen/xenbus/xenbus_probe.c
> > b/drivers/xen/xenbus/xenbus_probe.c
> > index 8aec0ed1d047..b8005b651a29 100644
> > --- a/drivers/xen/xenbus/xenbus_probe.c
> > +++ b/drivers/xen/xenbus/xenbus_probe.c
> > @@ -76,6 +76,8 @@ EXPORT_SYMBOL_GPL(xen_store_interface);
> >          ((xen_store_interface != NULL) && \
> >           (xen_store_interface->connection == XENSTORE_CONNECTED))
> >
> > +static bool xs_late_init = false;
> > +
> >   enum xenstore_init xen_store_domain_type;
> >   EXPORT_SYMBOL_GPL(xen_store_domain_type);
> >
> > @@ -755,7 +757,7 @@ static void xenbus_probe(void)
> >   {
> >          xenstored_ready = 1;
> >
> > -       if (!xen_store_interface || XS_INTERFACE_READY) {
> > +       if (xs_late_init) {
> >                  if (!xen_store_interface)
> >                          xen_store_interface = memremap(xen_store_gfn <<
> I would just remove the outer 'if' and do this:
>
>
>         if (!xen_store_interface)
>                 xen_store_interface = memremap(xen_store_gfn << XEN_PAGE_SHIFT,
>                                 XEN_PAGE_SIZE, MEMREMAP_WB);
>         /*
>          * 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.
>          */
>         if (xs_init_irq > 0)
>                 free_irq(xs_init_irq, &xb_waitq);
>
>
> I think this should work fine in all cases.
Thanks. I followed your suggestion in v2.

>   I am unsure if
> xs_init_irq==0 is possible valid value for xs_init_irq. If it is not,
> then we are fine. If 0 is a possible valid irq number, then we should
> initialize xs_init_irq to -1, and here check for xs_init_irq >= 0.
Yeah the xs_init_irq==0 is a valid value. I followed your latter comment 
to init it to -1 and check it >=0.
Kind regards,
Henry