Re: [XEN PATCH 1/7] xen: introduce XENFEAT_xenstore_late_init

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sat, 8 Jan 2022, Julien Grall wrote:
> On 08/01/2022 00:49, Stefano Stabellini wrote:
> > From: Luca Miccio <lucmiccio@xxxxxxxxx>
> > 
> > Introduce a new feature flag to signal that xenstore will not be
> > immediately available at boot time. Instead, xenstore will become
> > available later, and a notification of xenstore readiness will be
> > signalled to the guest using the xenstore event channel.
> 
> Hmmm... On the thread [1], you semmed to imply that new Linux version (I am
> assuming master) are ready to be used in dom0less with the node xen. So I am
> bit confused why this is necessary?

Today Linux/master can boot on Xen with this patch series applied and
with the hypervisor node in device tree. Linux boots fine but it is not
able to make use of the PV interfaces. During xenstore initialization,
Linux sees that HVM_PARAM_STORE_PFN has an invalid value, so it returns
error and continues without xenstore.

I have a patch for Linux that if XENFEAT_xenstore_late_init is present
makes Linux wait for an event notification before initializing xenstore:
https://marc.info/?l=xen-devel&m=164160299315589

So with v1 of the Xen and Linux patches series:
- Xen allocates the event channel at domain creation
- Linux boots, sees XENFEAT_xenstore_late_init and wait for an event
- init-dom0less later allocates the xenstore page
- init-dom0less triggers the xenstore event channel
- Linux receives the event and finishes the initialization, including
  mapping the xenstore page

With the Xen patches applies but no Linux patches, Linux would:
- try to initialize xenstore
- see an invalid HVM_PARAM_STORE_PFN and return error
- continue without xenstore



> > diff --git a/xen/include/public/features.h b/xen/include/public/features.h
> > index 9ee2f760ef..18f32b1a98 100644
> > --- a/xen/include/public/features.h
> > +++ b/xen/include/public/features.h
> > @@ -128,6 +128,12 @@
> >   #define XENFEAT_not_direct_mapped         16
> >   #define XENFEAT_direct_mapped             17
> >   +/*
> > + * The xenstore interface should be initialized only after receiving a
> > + * xenstore event channel notification.
> > + */
> > +#define XENFEAT_xenstore_late_init 18
> 
> You are assuming that there will be no event until Xenstored has discovered
> the domain. If I am not mistaken, this works because when you allocate an
> unbound port, we will not raise the event.
> 
> But I am not sure this is a guarantee for the event channel ABI. For instance,
> when using bind interdomain an event will be raised on the local port.
> 
> Looking at the Xenstore interface, there are a field connection. Could we use
> it (maybe a flag) to tell when the connection was fully initiated?

If we allocate HVM_PARAM_STORE_PFN directly from Xen, that would work
but the Linux xenbus driver will try to initialize the xenstore
interface immediately and it will get stuck in xenbus_thread. In my
tests wait_event_interruptible is the last thing that is called before
Linux getting stuck. Also note that functions like xb_init_comms looks
like they expect xenstored to be already up and running; xb_init_comms
is called unconditionally if the xenstore page and evtchn are
initialized successfully.

I liked your suggestion of adding a flag to struct
xenstore_domain_interface and I prototyped it. For instance, I added:

+#define XENSTORE_NOTREADY  2 /* xenstored not ready */

intf->connection is set to 2 by Xen at domain creation and later it is
set to 0 by init-dom0less.c to signal that the interface is now ready to
use. I think that would work fine but unfortunately it would break Linux
compatibility, because Linux/master of today doesn't know that it needs
to check for intf->connection to be 0 before continuing. It would get
stuck again because instead of waiting it would proceed with the
initialization.

Thus, I think we need to keep the allocation of HVM_PARAM_STORE_PFN
in init-dom0less.c not to break compatibility.

But we could get rid of XENFEAT_xenstore_late_init. The invalid value of
HVM_PARAM_STORE_PFN could be enough to tell Linux that it needs to
wait before it can continue with the initialization. There is no need
for XENFEAT_xenstore_late_init if we check that HVM_PARAM_STORE_EVTCHN
is valid but HVM_PARAM_STORE_PFN is zero.

If we do that, Linux/master keeps working (without PV drivers) because it
considers HVM_PARAM_STORE_PFN == 0 an error.

Linux with a new TBD patch would wait for an event notification and
check again HVM_PARAM_STORE_PFN when it receives the notification.

It is similar to what you suggested but instead of using a flag on the
Xenstore interface we would use the xen_param HVM_PARAM_STORE_PFN for
the same purpose. (FYI note that I'd be fine with using a flag on the
Xenstore shared interface page as well, but I cannot see how to make it
work without breaking compatibility with Linux/master.)