| Message ID | 20250206010843.618280-8-stefano.stabellini@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Guest XenStore page allocation for 11 Dom0less domUs | expand |
On 06/02/2025 02:08, Stefano Stabellini wrote: > The new xenstore page allocation scheme might break older unpatches > Linux kernels that do not check for the Xenstore connection status > before proceeding with Xenstore initialization. > > Introduce a dom0less configuration option to retain the older behavior, > which is not compatible with 1:1 mapped guests, but it will work with The issue is for static domains in general - not only for 1:1 guests. Static domains without direct map will simply fail on acquire_reserved_page(). > older legacy kernel versions. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > --- > docs/misc/arm/device-tree/booting.txt | 5 +++++ > xen/arch/arm/dom0less-build.c | 13 ++++++++++++- > xen/arch/arm/include/asm/kernel.h | 14 +++++++++++--- > 3 files changed, 28 insertions(+), 4 deletions(-) > > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt > index ff70d44462..8fa3da95be 100644 > --- a/docs/misc/arm/device-tree/booting.txt > +++ b/docs/misc/arm/device-tree/booting.txt > @@ -222,6 +222,11 @@ with the following properties: > Xen PV interfaces, including grant-table and xenstore, will be > enabled for the VM. > > + - "legacy" > + Same as above, but the way the xenstore page is allocated is not > + compatible with 1:1 mapped guests. On the other hand, it works with Same remark about 1:1 > + older Linux kernels. > + > - "disabled" > Xen PV interfaces are disabled. > > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c > index 046439eb87..9afdbca8b8 100644 > --- a/xen/arch/arm/dom0less-build.c > +++ b/xen/arch/arm/dom0less-build.c > @@ -799,6 +799,13 @@ static int __init construct_domU(struct domain *d, > else > panic("At the moment, Xenstore support requires dom0 to be present\n"); > } > + else if ( rc == 0 && !strcmp(dom0less_enhanced, "legacy") ) > + { > + if ( hardware_domain ) > + kinfo.dom0less_feature = DOM0LESS_ENHANCED_LEGACY; > + else > + panic("At the moment, Xenstore support requires dom0 to be present\n"); > + } > else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") ) > kinfo.dom0less_feature = DOM0LESS_ENHANCED_NO_XS; > > @@ -848,13 +855,17 @@ static int __init construct_domU(struct domain *d, > if ( rc < 0 ) > return rc; > > - if ( kinfo.dom0less_feature & DOM0LESS_XENSTORE ) > + if ( kinfo.dom0less_feature & (DOM0LESS_XENSTORE|DOM0LESS_XS_LEGACY) ) Spaces around | operator. > { > ASSERT(hardware_domain); > rc = alloc_xenstore_evtchn(d); > if ( rc < 0 ) > return rc; > + d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL; > + } > > + if ( kinfo.dom0less_feature & DOM0LESS_XENSTORE ) > + { Can I talk you into moving all of these into separate function e.g. alloc_xenstore_params(struct kernel_info *kinfo)? It would simplify construct_domU() in which we tend to just call functions responsible for a given functionality. > rc = alloc_xenstore_page(d); > if ( rc < 0 ) > return rc; > diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h > index de3f945ae5..4c2ae0b32b 100644 > --- a/xen/arch/arm/include/asm/kernel.h > +++ b/xen/arch/arm/include/asm/kernel.h > @@ -17,16 +17,24 @@ > * default features (excluding Xenstore) will be > * available. Note that an OS *must* not rely on the > * availability of Xen features if this is not set. > - * DOM0LESS_XENSTORE: Xenstore will be enabled for the VM. This feature > - * can't be enabled without the > - * DOM0LESS_ENHANCED_NO_XS. > + * DOM0LESS_XENSTORE: Xenstore will be enabled for the VM. The > + * xenstore page allocation is done by Xen at > + * domain creation. This feature can't be > + * enabled without the DOM0LESS_ENHANCED_NO_XS. > + * DOM0LESS_XS_LEGACY Xenstore will be enabled for the VM, the > + * xenstore page allocation will happen in > + * init-dom0less. This feature can't be enabled > + * without the DOM0LESS_ENHANCED_NO_XS. > * DOM0LESS_ENHANCED: Notify the OS it is running on top of Xen. All the > * default features (including Xenstore) will be > * available. Note that an OS *must* not rely on the > * availability of Xen features if this is not set. > + * DOM0LESS_ENHANCED_LEGACY:Same as before, but using DOM0LESS_XS_LEGACY. NIT: I would just >> all text by one to have a space after : > */ > #define DOM0LESS_ENHANCED_NO_XS BIT(0, U) > #define DOM0LESS_XENSTORE BIT(1, U) > +#define DOM0LESS_XS_LEGACY BIT(2, U) > +#define DOM0LESS_ENHANCED_LEGACY (DOM0LESS_ENHANCED_NO_XS | DOM0LESS_XS_LEGACY) > #define DOM0LESS_ENHANCED (DOM0LESS_ENHANCED_NO_XS | DOM0LESS_XENSTORE) > > struct kernel_info { Otherwise, patch is ok. ~Michal
On Thu, 6 Feb 2025, Orzel, Michal wrote: > On 06/02/2025 02:08, Stefano Stabellini wrote: > > The new xenstore page allocation scheme might break older unpatches > > Linux kernels that do not check for the Xenstore connection status > > before proceeding with Xenstore initialization. > > > > Introduce a dom0less configuration option to retain the older behavior, > > which is not compatible with 1:1 mapped guests, but it will work with > The issue is for static domains in general - not only for 1:1 guests. > Static domains without direct map will simply fail on acquire_reserved_page(). I'll clarify > > older legacy kernel versions. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > --- > > docs/misc/arm/device-tree/booting.txt | 5 +++++ > > xen/arch/arm/dom0less-build.c | 13 ++++++++++++- > > xen/arch/arm/include/asm/kernel.h | 14 +++++++++++--- > > 3 files changed, 28 insertions(+), 4 deletions(-) > > > > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt > > index ff70d44462..8fa3da95be 100644 > > --- a/docs/misc/arm/device-tree/booting.txt > > +++ b/docs/misc/arm/device-tree/booting.txt > > @@ -222,6 +222,11 @@ with the following properties: > > Xen PV interfaces, including grant-table and xenstore, will be > > enabled for the VM. > > > > + - "legacy" > > + Same as above, but the way the xenstore page is allocated is not > > + compatible with 1:1 mapped guests. On the other hand, it works with > Same remark about 1:1 > > > + older Linux kernels. > > + > > - "disabled" > > Xen PV interfaces are disabled. > > > > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c > > index 046439eb87..9afdbca8b8 100644 > > --- a/xen/arch/arm/dom0less-build.c > > +++ b/xen/arch/arm/dom0less-build.c > > @@ -799,6 +799,13 @@ static int __init construct_domU(struct domain *d, > > else > > panic("At the moment, Xenstore support requires dom0 to be present\n"); > > } > > + else if ( rc == 0 && !strcmp(dom0less_enhanced, "legacy") ) > > + { > > + if ( hardware_domain ) > > + kinfo.dom0less_feature = DOM0LESS_ENHANCED_LEGACY; > > + else > > + panic("At the moment, Xenstore support requires dom0 to be present\n"); > > + } > > else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") ) > > kinfo.dom0less_feature = DOM0LESS_ENHANCED_NO_XS; > > > > @@ -848,13 +855,17 @@ static int __init construct_domU(struct domain *d, > > if ( rc < 0 ) > > return rc; > > > > - if ( kinfo.dom0less_feature & DOM0LESS_XENSTORE ) > > + if ( kinfo.dom0less_feature & (DOM0LESS_XENSTORE|DOM0LESS_XS_LEGACY) ) > Spaces around | operator. OK > > > { > > ASSERT(hardware_domain); > > rc = alloc_xenstore_evtchn(d); > > if ( rc < 0 ) > > return rc; > > + d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL; > > + } > > > > + if ( kinfo.dom0less_feature & DOM0LESS_XENSTORE ) > > + { > Can I talk you into moving all of these into separate function e.g. alloc_xenstore_params(struct kernel_info *kinfo)? > It would simplify construct_domU() in which we tend to just call functions responsible for a given functionality. OK > > rc = alloc_xenstore_page(d); > > if ( rc < 0 ) > > return rc; > > diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h > > index de3f945ae5..4c2ae0b32b 100644 > > --- a/xen/arch/arm/include/asm/kernel.h > > +++ b/xen/arch/arm/include/asm/kernel.h > > @@ -17,16 +17,24 @@ > > * default features (excluding Xenstore) will be > > * available. Note that an OS *must* not rely on the > > * availability of Xen features if this is not set. > > - * DOM0LESS_XENSTORE: Xenstore will be enabled for the VM. This feature > > - * can't be enabled without the > > - * DOM0LESS_ENHANCED_NO_XS. > > + * DOM0LESS_XENSTORE: Xenstore will be enabled for the VM. The > > + * xenstore page allocation is done by Xen at > > + * domain creation. This feature can't be > > + * enabled without the DOM0LESS_ENHANCED_NO_XS. > > + * DOM0LESS_XS_LEGACY Xenstore will be enabled for the VM, the > > + * xenstore page allocation will happen in > > + * init-dom0less. This feature can't be enabled > > + * without the DOM0LESS_ENHANCED_NO_XS. > > * DOM0LESS_ENHANCED: Notify the OS it is running on top of Xen. All the > > * default features (including Xenstore) will be > > * available. Note that an OS *must* not rely on the > > * availability of Xen features if this is not set. > > + * DOM0LESS_ENHANCED_LEGACY:Same as before, but using DOM0LESS_XS_LEGACY. > NIT: I would just >> all text by one to have a space after : OK > > #define DOM0LESS_ENHANCED_NO_XS BIT(0, U) > > #define DOM0LESS_XENSTORE BIT(1, U) > > +#define DOM0LESS_XS_LEGACY BIT(2, U) > > +#define DOM0LESS_ENHANCED_LEGACY (DOM0LESS_ENHANCED_NO_XS | DOM0LESS_XS_LEGACY) > > #define DOM0LESS_ENHANCED (DOM0LESS_ENHANCED_NO_XS | DOM0LESS_XENSTORE) > > > > struct kernel_info { > > Otherwise, patch is ok. Thanks for the review
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt index ff70d44462..8fa3da95be 100644 --- a/docs/misc/arm/device-tree/booting.txt +++ b/docs/misc/arm/device-tree/booting.txt @@ -222,6 +222,11 @@ with the following properties: Xen PV interfaces, including grant-table and xenstore, will be enabled for the VM. + - "legacy" + Same as above, but the way the xenstore page is allocated is not + compatible with 1:1 mapped guests. On the other hand, it works with + older Linux kernels. + - "disabled" Xen PV interfaces are disabled. diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index 046439eb87..9afdbca8b8 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -799,6 +799,13 @@ static int __init construct_domU(struct domain *d, else panic("At the moment, Xenstore support requires dom0 to be present\n"); } + else if ( rc == 0 && !strcmp(dom0less_enhanced, "legacy") ) + { + if ( hardware_domain ) + kinfo.dom0less_feature = DOM0LESS_ENHANCED_LEGACY; + else + panic("At the moment, Xenstore support requires dom0 to be present\n"); + } else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") ) kinfo.dom0less_feature = DOM0LESS_ENHANCED_NO_XS; @@ -848,13 +855,17 @@ static int __init construct_domU(struct domain *d, if ( rc < 0 ) return rc; - if ( kinfo.dom0less_feature & DOM0LESS_XENSTORE ) + if ( kinfo.dom0less_feature & (DOM0LESS_XENSTORE|DOM0LESS_XS_LEGACY) ) { ASSERT(hardware_domain); rc = alloc_xenstore_evtchn(d); if ( rc < 0 ) return rc; + d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL; + } + if ( kinfo.dom0less_feature & DOM0LESS_XENSTORE ) + { rc = alloc_xenstore_page(d); if ( rc < 0 ) return rc; diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h index de3f945ae5..4c2ae0b32b 100644 --- a/xen/arch/arm/include/asm/kernel.h +++ b/xen/arch/arm/include/asm/kernel.h @@ -17,16 +17,24 @@ * default features (excluding Xenstore) will be * available. Note that an OS *must* not rely on the * availability of Xen features if this is not set. - * DOM0LESS_XENSTORE: Xenstore will be enabled for the VM. This feature - * can't be enabled without the - * DOM0LESS_ENHANCED_NO_XS. + * DOM0LESS_XENSTORE: Xenstore will be enabled for the VM. The + * xenstore page allocation is done by Xen at + * domain creation. This feature can't be + * enabled without the DOM0LESS_ENHANCED_NO_XS. + * DOM0LESS_XS_LEGACY Xenstore will be enabled for the VM, the + * xenstore page allocation will happen in + * init-dom0less. This feature can't be enabled + * without the DOM0LESS_ENHANCED_NO_XS. * DOM0LESS_ENHANCED: Notify the OS it is running on top of Xen. All the * default features (including Xenstore) will be * available. Note that an OS *must* not rely on the * availability of Xen features if this is not set. + * DOM0LESS_ENHANCED_LEGACY:Same as before, but using DOM0LESS_XS_LEGACY. */ #define DOM0LESS_ENHANCED_NO_XS BIT(0, U) #define DOM0LESS_XENSTORE BIT(1, U) +#define DOM0LESS_XS_LEGACY BIT(2, U) +#define DOM0LESS_ENHANCED_LEGACY (DOM0LESS_ENHANCED_NO_XS | DOM0LESS_XS_LEGACY) #define DOM0LESS_ENHANCED (DOM0LESS_ENHANCED_NO_XS | DOM0LESS_XENSTORE) struct kernel_info {
The new xenstore page allocation scheme might break older unpatches Linux kernels that do not check for the Xenstore connection status before proceeding with Xenstore initialization. Introduce a dom0less configuration option to retain the older behavior, which is not compatible with 1:1 mapped guests, but it will work with older legacy kernel versions. Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> --- docs/misc/arm/device-tree/booting.txt | 5 +++++ xen/arch/arm/dom0less-build.c | 13 ++++++++++++- xen/arch/arm/include/asm/kernel.h | 14 +++++++++++--- 3 files changed, 28 insertions(+), 4 deletions(-)