| Message ID | 20220108004912.3820176-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dom0less PV drivers | expand |
Hi, On 08/01/2022 00:49, Stefano Stabellini wrote: > From: Luca Miccio <lucmiccio@gmail.com> > > 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? > > Signed-off-by: Luca Miccio <lucmiccio@gmail.com> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: George Dunlap <george.dunlap@citrix.com> > CC: Wei Liu <wl@xen.org> > --- > xen/arch/arm/include/asm/domain.h | 2 ++ > xen/common/kernel.c | 2 ++ > xen/include/public/features.h | 6 ++++++ > 3 files changed, 10 insertions(+) > > diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h > index 9b3647587a..e5ae57cd09 100644 > --- a/xen/arch/arm/include/asm/domain.h > +++ b/xen/arch/arm/include/asm/domain.h > @@ -89,6 +89,8 @@ struct arch_domain > #ifdef CONFIG_TEE > void *tee; > #endif > + /* Is this guest a dom0less domain? */ > + bool is_dom0less; > } __cacheline_aligned; > > struct arch_vcpu > diff --git a/xen/common/kernel.c b/xen/common/kernel.c > index e119e5401f..c00ea67e5f 100644 > --- a/xen/common/kernel.c > +++ b/xen/common/kernel.c > @@ -550,6 +550,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > if ( is_hardware_domain(d) ) > fi.submap |= 1U << XENFEAT_dom0; > #ifdef CONFIG_ARM > + if ( d->arch.is_dom0less ) > + fi.submap |= (1U << XENFEAT_xenstore_late_init); > fi.submap |= (1U << XENFEAT_ARM_SMCCC_supported); > #endif > #ifdef CONFIG_X86 > 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? > + > #define XENFEAT_NR_SUBMAPS 1 > > #endif /* __XEN_PUBLIC_FEATURES_H__ */ Cheers, [1] <alpine.DEB.2.22.394.2112131729100.3376@ubuntu-linux-20-04-desktop>
On 08.01.2022 01:49, Stefano Stabellini wrote: > 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. In addition to what Julien has said, I'd like to point out that Linux'es xenbus driver already has means to deal with xenstored not being around right away (perhaps because of living in a stubdom which starts in parallel). I therefore wonder whether what you want can't be achieved entirely inside that driver, without any new feature flag. Jan
On Sat, 8 Jan 2022, Julien Grall wrote: > On 08/01/2022 00:49, Stefano Stabellini wrote: > > From: Luca Miccio <lucmiccio@gmail.com> > > > > 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.)
On Mon, 10 Jan 2022, Jan Beulich wrote: > On 08.01.2022 01:49, Stefano Stabellini wrote: > > 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. > > In addition to what Julien has said, I'd like to point out that Linux'es > xenbus driver already has means to deal with xenstored not being around > right away (perhaps because of living in a stubdom which starts in > parallel). I therefore wonder whether what you want can't be achieved > entirely inside that driver, without any new feature flag. The fewer changes to Linux the better but we couldn't find a way to make it work with zero changes. In a way, the patch for Linux is re-using the existing infrastructure to delay initialization, e.g. xenbus_probe_thread to call xenbus_probe later. However, today there is nothing stopping Linux/HVM to continue initialization immediately except for xs_hvm_defer_init_for_callback which is not applicable in this case. So we introduced XENFEAT_xenstore_late_init. As I wrote in another reply, I think we could do without XENFEAT_xenstore_late_init if we instead rely on checking for HVM_PARAM_STORE_EVTCHN being valid and HVM_PARAM_STORE_PFN being zero. But either way as far as I can tell we need to add a new check to delay xenstore initialization in Linux/HVM.
On 11.01.2022 00:08, Stefano Stabellini wrote: > On Mon, 10 Jan 2022, Jan Beulich wrote: >> On 08.01.2022 01:49, Stefano Stabellini wrote: >>> 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. >> >> In addition to what Julien has said, I'd like to point out that Linux'es >> xenbus driver already has means to deal with xenstored not being around >> right away (perhaps because of living in a stubdom which starts in >> parallel). I therefore wonder whether what you want can't be achieved >> entirely inside that driver, without any new feature flag. > > The fewer changes to Linux the better but we couldn't find a way to make > it work with zero changes. > > In a way, the patch for Linux is re-using the existing infrastructure to > delay initialization, e.g. xenbus_probe_thread to call xenbus_probe > later. > > However, today there is nothing stopping Linux/HVM to continue > initialization immediately except for xs_hvm_defer_init_for_callback > which is not applicable in this case. So we introduced > XENFEAT_xenstore_late_init. > > As I wrote in another reply, I think we could do without > XENFEAT_xenstore_late_init if we instead rely on checking for > HVM_PARAM_STORE_EVTCHN being valid and HVM_PARAM_STORE_PFN being zero. Just as an aside - as discussed in some other context not so long ago, HVM_PARAM_*_PFN being zero isn't the best way of expressing "not yet initialized", and hence you would also want to check against ~0. > But either way as far as I can tell we need to add a new check to delay > xenstore initialization in Linux/HVM. Yes, I can see that a Linux side change might be needed. But isolating the change to there will be much better than needing to also have a Xen side change in place. Jan
On 10/01/2022 22:55, Stefano Stabellini wrote: > > 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 You can get this behaviour without the new feature. - Xen allocates the event channel at domain creation - Linux boots, sees HVM_PARAM_STORE_PFN is invalid and there's a xenstore event channel and waits 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- David
On Tue, 11 Jan 2022, Jan Beulich wrote: > On 11.01.2022 00:08, Stefano Stabellini wrote: > > On Mon, 10 Jan 2022, Jan Beulich wrote: > >> On 08.01.2022 01:49, Stefano Stabellini wrote: > >>> 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. > >> > >> In addition to what Julien has said, I'd like to point out that Linux'es > >> xenbus driver already has means to deal with xenstored not being around > >> right away (perhaps because of living in a stubdom which starts in > >> parallel). I therefore wonder whether what you want can't be achieved > >> entirely inside that driver, without any new feature flag. > > > > The fewer changes to Linux the better but we couldn't find a way to make > > it work with zero changes. > > > > In a way, the patch for Linux is re-using the existing infrastructure to > > delay initialization, e.g. xenbus_probe_thread to call xenbus_probe > > later. > > > > However, today there is nothing stopping Linux/HVM to continue > > initialization immediately except for xs_hvm_defer_init_for_callback > > which is not applicable in this case. So we introduced > > XENFEAT_xenstore_late_init. > > > > As I wrote in another reply, I think we could do without > > XENFEAT_xenstore_late_init if we instead rely on checking for > > HVM_PARAM_STORE_EVTCHN being valid and HVM_PARAM_STORE_PFN being zero. > > Just as an aside - as discussed in some other context not so long ago, > HVM_PARAM_*_PFN being zero isn't the best way of expressing "not yet > initialized", and hence you would also want to check against ~0. Yes, good point > > But either way as far as I can tell we need to add a new check to delay > > xenstore initialization in Linux/HVM. > > Yes, I can see that a Linux side change might be needed. But isolating > the change to there will be much better than needing to also have a > Xen side change in place. I agree. I managed to remove XENFEAT_xenstore_late_init from the patch series and everything works fine. It will be in the next version.
On Tue, 11 Jan 2022, David Vrabel wrote: > On 10/01/2022 22:55, Stefano Stabellini wrote: > > > > 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 > > You can get this behaviour without the new feature. > > - Xen allocates the event channel at domain creation > - Linux boots, sees HVM_PARAM_STORE_PFN is invalid and there's a xenstore > event channel and waits 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- Hey David! Yes, this is similar to what I had in mind and I managed to make it work successfully.
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h index 9b3647587a..e5ae57cd09 100644 --- a/xen/arch/arm/include/asm/domain.h +++ b/xen/arch/arm/include/asm/domain.h @@ -89,6 +89,8 @@ struct arch_domain #ifdef CONFIG_TEE void *tee; #endif + /* Is this guest a dom0less domain? */ + bool is_dom0less; } __cacheline_aligned; struct arch_vcpu diff --git a/xen/common/kernel.c b/xen/common/kernel.c index e119e5401f..c00ea67e5f 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -550,6 +550,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) if ( is_hardware_domain(d) ) fi.submap |= 1U << XENFEAT_dom0; #ifdef CONFIG_ARM + if ( d->arch.is_dom0less ) + fi.submap |= (1U << XENFEAT_xenstore_late_init); fi.submap |= (1U << XENFEAT_ARM_SMCCC_supported); #endif #ifdef CONFIG_X86 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 + #define XENFEAT_NR_SUBMAPS 1 #endif /* __XEN_PUBLIC_FEATURES_H__ */