[PATCH v5 0/9] Guest XenStore page allocation for 11 Dom0less domUs

[PATCH v5 0/9] Guest XenStore page allocation for 11 Dom0less domUs

[Stefano Stabellini]

Hi all,

The last version (v4) of this patch series [1] was fully acked and
committed. However, we later discovered that it caused regressions with
older Linux kernels without a fix [2], so we reverted the patch series
from Xen.

In the meantime, Linux backported the fix [2] to all kernels, so at
present there are no regressions any longer.

This update on the patch series contains the original v4 patches
unmodified. It also updates our automation testing infrastructure to use
a newer kernel. It adds a test to validate the feature introduced by
this patch series (PV drivers together with 11 mapped guests).

Finally, this patch series introduces a new dom0less option to retain
the old behavior in case users want to run older unpatched Linux kernel
versions.

To verify that the legacy option works and retains compatibility with
older unpatched kernels, I added a test at the end of the series using
the older unpatched Linux kernel and the newly introduced "legacy"
dom0less option. I don't suggest we commit this last test but if you
think otherwise, please let me know and I can clean it up and also add
the ImageBuilder part of it (a way to set the legacy option in the
ImageBuilder config file).

Cheers,

Stefano


[1] https://marc.info/?l=xen-devel&m=171659112108921
[2] a3607581cd49 "drivers/xen: Improve the late XenStore init protocol"
[3] https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/1656094397

[PATCH v5 1/9] automation: upgrade Linux kernel for arm64 tests to 6.6.74

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 automation/gitlab-ci/build.yaml                              | 4 ++--
 automation/gitlab-ci/test.yaml                               | 2 +-
 .../{5.19-arm64v8.dockerfile => 6.6.74-arm64v8.dockerfile}   | 5 +++--
 3 files changed, 6 insertions(+), 5 deletions(-)
 rename automation/tests-artifacts/kernel/{5.19-arm64v8.dockerfile => 6.6.74-arm64v8.dockerfile} (90%)

diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index bc4a8a5ad2..411b4902b5 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -269,9 +269,9 @@ alpine-3.18-arm64-rootfs-export:
   tags:
     - arm64
 
-kernel-5.19-arm64-export:
+kernel-6.6.74-arm64-export:
   extends: .test-jobs-artifact-common
-  image: registry.gitlab.com/xen-project/xen/tests-artifacts/kernel:5.19-arm64v8
+  image: registry.gitlab.com/xen-project/xen/tests-artifacts/kernel:6.6.74-arm64v8
   script:
     - mkdir binaries && cp /Image binaries/Image
   artifacts:
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 1822e3ea5f..6ad45269ea 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -4,7 +4,7 @@
 
 .arm64-test-needs: &arm64-test-needs
   - alpine-3.18-arm64-rootfs-export
-  - kernel-5.19-arm64-export
+  - kernel-6.6.74-arm64-export
   - qemu-system-aarch64-6.0.0-arm64-export
 
 .arm32-test-needs: &arm32-test-needs
diff --git a/automation/tests-artifacts/kernel/5.19-arm64v8.dockerfile b/automation/tests-artifacts/kernel/6.6.74-arm64v8.dockerfile
similarity index 90%
rename from automation/tests-artifacts/kernel/5.19-arm64v8.dockerfile
rename to automation/tests-artifacts/kernel/6.6.74-arm64v8.dockerfile
index 8e33995ba3..73e5145425 100644
--- a/automation/tests-artifacts/kernel/5.19-arm64v8.dockerfile
+++ b/automation/tests-artifacts/kernel/6.6.74-arm64v8.dockerfile
@@ -4,7 +4,7 @@ LABEL maintainer.name="The Xen Project" \
       maintainer.email="xen-devel@lists.xenproject.org"
 
 ENV DEBIAN_FRONTEND=noninteractive
-ENV LINUX_VERSION=5.19
+ENV LINUX_VERSION=6.6.74
 ENV USER root
 
 RUN mkdir /build
@@ -18,10 +18,11 @@ RUN apt-get update && \
         curl \
         flex \
         bison \
+        libssl-dev \
         && \
     \
     # Build the kernel
-    curl -fsSLO https://cdn.kernel.org/pub/linux/kernel/v5.x/linux-"$LINUX_VERSION".tar.xz && \
+    curl -fsSLO https://cdn.kernel.org/pub/linux/kernel/v6.x/linux-"$LINUX_VERSION".tar.xz && \
     tar xvJf linux-"$LINUX_VERSION".tar.xz && \
     cd linux-"$LINUX_VERSION" && \
     make defconfig && \
-- 
2.25.1

Re: [PATCH v5 1/9] automation: upgrade Linux kernel for arm64 tests to 6.6.74

[Orzel, Michal]

On 06/02/2025 02:08, Stefano Stabellini wrote:
> 
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Any particular reason behind choosing 6.6.74 and not the latest longterm 6.6.75?

In any case:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v5 1/9] automation: upgrade Linux kernel for arm64 tests to 6.6.74

[Stefano Stabellini]

On Thu, 6 Feb 2025, Orzel, Michal wrote:
> On 06/02/2025 02:08, Stefano Stabellini wrote:
> > 
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Any particular reason behind choosing 6.6.74 and not the latest longterm 6.6.75?

No, it was the latest when I developed this patch last week


> In any case:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thank you!

[PATCH v5 2/9] xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains

[Stefano Stabellini]

From: Henry Wang <xin.wang2@amd.com>

Currently, users are allowed to map static shared memory in a
non-direct-mapped way for direct-mapped domains. This can lead to
clashing of guest memory spaces. Also, the current extended region
finding logic only removes the host physical addresses of the
static shared memory areas for direct-mapped domains, which may be
inconsistent with the guest memory map if users map the static
shared memory in a non-direct-mapped way. This will lead to incorrect
extended region calculation results.

To make things easier, add restriction that static shared memory
should also be direct-mapped for direct-mapped domains. Check the
host physical address to be matched with guest physical address when
parsing the device tree. Document this restriction in the doc.

Signed-off-by: Henry Wang <xin.wang2@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Acked-by: Michal Orzel <michal.orzel@amd.com>
---
 docs/misc/arm/device-tree/booting.txt | 3 +++
 xen/arch/arm/static-shmem.c           | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 9c881baccc..ff70d44462 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -615,6 +615,9 @@ communication.
     Note that if a domain is direct-mapped, i.e. the Dom0 and the Dom0less
     DomUs with `direct-map` device tree property, the static shared memory
     should also be direct-mapped (host physical address == guest address).
+    Note that if a domain is direct-mapped, i.e. the Dom0 and the Dom0less
+    DomUs with `direct-map` device tree property, the static shared memory
+    should also be direct-mapped (host physical address == guest address).
 
     It shall also meet the following criteria:
     1) If the SHM ID matches with an existing region, the address range of the
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 8f87154c35..44eeac2a23 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -325,6 +325,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
             printk("%pd: static shared memory bank not found: '%s'", d, shm_id);
             return -ENOENT;
         }
+        if ( is_domain_direct_mapped(d) && (pbase != gbase) )
+        {
+            printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n",
+                   d, pbase, gbase);
+            return -EINVAL;
+        }
 
         pbase = boot_shm_bank->start;
         psize = boot_shm_bank->size;
-- 
2.25.1

Re: [PATCH v5 2/9] xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains

[Orzel, Michal]

On 06/02/2025 02:08, Stefano Stabellini wrote:
> From: Henry Wang <xin.wang2@amd.com>
> 
> Currently, users are allowed to map static shared memory in a
> non-direct-mapped way for direct-mapped domains. This can lead to
> clashing of guest memory spaces. Also, the current extended region
> finding logic only removes the host physical addresses of the
> static shared memory areas for direct-mapped domains, which may be
> inconsistent with the guest memory map if users map the static
> shared memory in a non-direct-mapped way. This will lead to incorrect
> extended region calculation results.
> 
> To make things easier, add restriction that static shared memory
> should also be direct-mapped for direct-mapped domains. Check the
> host physical address to be matched with guest physical address when
> parsing the device tree. Document this restriction in the doc.
> 
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Acked-by: Michal Orzel <michal.orzel@amd.com>
This patch has already been committed (see 0a0f30c1b55e) and later on fixed (see 988f1c7e1f40).

DO NOT COMMIT.

~Michal

Re: [PATCH v5 2/9] xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains

[Stefano Stabellini]

On Thu, 6 Feb 2025, Orzel, Michal wrote:
> On 06/02/2025 02:08, Stefano Stabellini wrote:
> > From: Henry Wang <xin.wang2@amd.com>
> > 
> > Currently, users are allowed to map static shared memory in a
> > non-direct-mapped way for direct-mapped domains. This can lead to
> > clashing of guest memory spaces. Also, the current extended region
> > finding logic only removes the host physical addresses of the
> > static shared memory areas for direct-mapped domains, which may be
> > inconsistent with the guest memory map if users map the static
> > shared memory in a non-direct-mapped way. This will lead to incorrect
> > extended region calculation results.
> > 
> > To make things easier, add restriction that static shared memory
> > should also be direct-mapped for direct-mapped domains. Check the
> > host physical address to be matched with guest physical address when
> > parsing the device tree. Document this restriction in the doc.
> > 
> > Signed-off-by: Henry Wang <xin.wang2@amd.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > Acked-by: Michal Orzel <michal.orzel@amd.com>
> This patch has already been committed (see 0a0f30c1b55e) and later on fixed (see 988f1c7e1f40).
> 
> DO NOT COMMIT.

Thanks Michal!! I'll take off the series.

[PATCH v5 3/9] xen/arm: Alloc XenStore page for Dom0less DomUs from hypervisor

[Stefano Stabellini]

From: Henry Wang <xin.wang2@amd.com>

There are use cases (for example using the PV driver) in Dom0less
setup that require Dom0less DomUs start immediately with Dom0, but
initialize XenStore later after Dom0's successful boot and call to
the init-dom0less application.

An error message can seen from the init-dom0less application on
1:1 direct-mapped domains:
```
Allocating magic pages
memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
Error on alloc magic pages
```

The "magic page" is a terminology used in the toolstack as reserved
pages for the VM to have access to virtual platform capabilities.
Currently the magic pages for Dom0less DomUs are populated by the
init-dom0less app through populate_physmap(), and populate_physmap()
automatically assumes gfn == mfn for 1:1 direct mapped domains. This
cannot be true for the magic pages that are allocated later from the
init-dom0less application executed in Dom0. For domain using statically
allocated memory but not 1:1 direct-mapped, similar error "failed to
retrieve a reserved page" can be seen as the reserved memory list is
empty at that time.

Since for init-dom0less, the magic page region is only for XenStore.
To solve above issue, this commit allocates the XenStore page for
Dom0less DomUs at the domain construction time. The PFN will be
noted and communicated to the init-dom0less application executed
from Dom0. To keep the XenStore late init protocol, set the connection
status to XENSTORE_RECONNECT.

Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
Suggested-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 xen/arch/arm/dom0less-build.c | 55 ++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 49d1f14d65..046439eb87 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 #include <xen/device_tree.h>
+#include <xen/domain_page.h>
 #include <xen/err.h>
 #include <xen/event.h>
 #include <xen/grant_table.h>
@@ -11,6 +12,8 @@
 #include <xen/sizes.h>
 #include <xen/vmap.h>
 
+#include <public/io/xs_wire.h>
+
 #include <asm/arm64/sve.h>
 #include <asm/dom0less-build.h>
 #include <asm/domain_build.h>
@@ -704,6 +707,53 @@ static int __init alloc_xenstore_evtchn(struct domain *d)
     return 0;
 }
 
+#define XENSTORE_PFN_OFFSET 1
+static int __init alloc_xenstore_page(struct domain *d)
+{
+    struct page_info *xenstore_pg;
+    struct xenstore_domain_interface *interface;
+    mfn_t mfn;
+    gfn_t gfn;
+    int rc;
+
+    if ( (UINT_MAX - d->max_pages) < 1 )
+    {
+        printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages by 1 page.\n",
+               d);
+        return -EINVAL;
+    }
+    d->max_pages += 1;
+    xenstore_pg = alloc_domheap_page(d, MEMF_bits(32));
+    if ( xenstore_pg == NULL && is_64bit_domain(d) )
+        xenstore_pg = alloc_domheap_page(d, 0);
+    if ( xenstore_pg == NULL )
+        return -ENOMEM;
+
+    mfn = page_to_mfn(xenstore_pg);
+    if ( !mfn_x(mfn) )
+        return -ENOMEM;
+
+    if ( !is_domain_direct_mapped(d) )
+        gfn = gaddr_to_gfn(GUEST_MAGIC_BASE +
+                           (XENSTORE_PFN_OFFSET << PAGE_SHIFT));
+    else
+        gfn = gaddr_to_gfn(mfn_to_maddr(mfn));
+
+    rc = guest_physmap_add_page(d, gfn, mfn, 0);
+    if ( rc )
+    {
+        free_domheap_page(xenstore_pg);
+        return rc;
+    }
+
+    d->arch.hvm.params[HVM_PARAM_STORE_PFN] = gfn_x(gfn);
+    interface = map_domain_page(mfn);
+    interface->connection = XENSTORE_RECONNECT;
+    unmap_domain_page(interface);
+
+    return 0;
+}
+
 static int __init construct_domU(struct domain *d,
                                  const struct dt_device_node *node)
 {
@@ -804,7 +854,10 @@ static int __init construct_domU(struct domain *d,
         rc = alloc_xenstore_evtchn(d);
         if ( rc < 0 )
             return rc;
-        d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL;
+
+        rc = alloc_xenstore_page(d);
+        if ( rc < 0 )
+            return rc;
     }
 
     return rc;
-- 
2.25.1

Re: [PATCH v5 3/9] xen/arm: Alloc XenStore page for Dom0less DomUs from hypervisor

[Orzel, Michal]

On 06/02/2025 02:08, Stefano Stabellini wrote:
> From: Henry Wang <xin.wang2@amd.com>
> 
> There are use cases (for example using the PV driver) in Dom0less
> setup that require Dom0less DomUs start immediately with Dom0, but
> initialize XenStore later after Dom0's successful boot and call to
> the init-dom0less application.
> 
> An error message can seen from the init-dom0less application on
> 1:1 direct-mapped domains:
> ```
> Allocating magic pages
> memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
> Error on alloc magic pages
> ```
> 
> The "magic page" is a terminology used in the toolstack as reserved
> pages for the VM to have access to virtual platform capabilities.
> Currently the magic pages for Dom0less DomUs are populated by the
> init-dom0less app through populate_physmap(), and populate_physmap()
> automatically assumes gfn == mfn for 1:1 direct mapped domains. This
> cannot be true for the magic pages that are allocated later from the
> init-dom0less application executed in Dom0. For domain using statically
> allocated memory but not 1:1 direct-mapped, similar error "failed to
> retrieve a reserved page" can be seen as the reserved memory list is
> empty at that time.
> 
> Since for init-dom0less, the magic page region is only for XenStore.
> To solve above issue, this commit allocates the XenStore page for
> Dom0less DomUs at the domain construction time. The PFN will be
> noted and communicated to the init-dom0less application executed
> from Dom0. To keep the XenStore late init protocol, set the connection
> status to XENSTORE_RECONNECT.
> 
> Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
> Suggested-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>  xen/arch/arm/dom0less-build.c | 55 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 49d1f14d65..046439eb87 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -1,5 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  #include <xen/device_tree.h>
> +#include <xen/domain_page.h>
>  #include <xen/err.h>
>  #include <xen/event.h>
>  #include <xen/grant_table.h>
> @@ -11,6 +12,8 @@
>  #include <xen/sizes.h>
>  #include <xen/vmap.h>
>  
> +#include <public/io/xs_wire.h>
> +
>  #include <asm/arm64/sve.h>
>  #include <asm/dom0less-build.h>
>  #include <asm/domain_build.h>
> @@ -704,6 +707,53 @@ static int __init alloc_xenstore_evtchn(struct domain *d)
>      return 0;
>  }
>  
> +#define XENSTORE_PFN_OFFSET 1
> +static int __init alloc_xenstore_page(struct domain *d)
> +{
> +    struct page_info *xenstore_pg;
> +    struct xenstore_domain_interface *interface;
> +    mfn_t mfn;
> +    gfn_t gfn;
> +    int rc;
> +
> +    if ( (UINT_MAX - d->max_pages) < 1 )
> +    {
> +        printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages by 1 page.\n",
> +               d);
> +        return -EINVAL;
> +    }
empty line here

> +    d->max_pages += 1;
If this patch is separate from modifying init-dom0less, max_pages will be bumped twice. Here and in init-dom0less.
Shouldn't we fold it in? The rest is ok.

~Michal

Re: [PATCH v5 3/9] xen/arm: Alloc XenStore page for Dom0less DomUs from hypervisor

[Stefano Stabellini]

On Thu, 6 Feb 2025, Orzel, Michal wrote:
> On 06/02/2025 02:08, Stefano Stabellini wrote:
> > From: Henry Wang <xin.wang2@amd.com>
> > 
> > There are use cases (for example using the PV driver) in Dom0less
> > setup that require Dom0less DomUs start immediately with Dom0, but
> > initialize XenStore later after Dom0's successful boot and call to
> > the init-dom0less application.
> > 
> > An error message can seen from the init-dom0less application on
> > 1:1 direct-mapped domains:
> > ```
> > Allocating magic pages
> > memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
> > Error on alloc magic pages
> > ```
> > 
> > The "magic page" is a terminology used in the toolstack as reserved
> > pages for the VM to have access to virtual platform capabilities.
> > Currently the magic pages for Dom0less DomUs are populated by the
> > init-dom0less app through populate_physmap(), and populate_physmap()
> > automatically assumes gfn == mfn for 1:1 direct mapped domains. This
> > cannot be true for the magic pages that are allocated later from the
> > init-dom0less application executed in Dom0. For domain using statically
> > allocated memory but not 1:1 direct-mapped, similar error "failed to
> > retrieve a reserved page" can be seen as the reserved memory list is
> > empty at that time.
> > 
> > Since for init-dom0less, the magic page region is only for XenStore.
> > To solve above issue, this commit allocates the XenStore page for
> > Dom0less DomUs at the domain construction time. The PFN will be
> > noted and communicated to the init-dom0less application executed
> > from Dom0. To keep the XenStore late init protocol, set the connection
> > status to XENSTORE_RECONNECT.
> > 
> > Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
> > Suggested-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> > Signed-off-by: Henry Wang <xin.wang2@amd.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > ---
> >  xen/arch/arm/dom0less-build.c | 55 ++++++++++++++++++++++++++++++++++-
> >  1 file changed, 54 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> > index 49d1f14d65..046439eb87 100644
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -1,5 +1,6 @@
> >  /* SPDX-License-Identifier: GPL-2.0-only */
> >  #include <xen/device_tree.h>
> > +#include <xen/domain_page.h>
> >  #include <xen/err.h>
> >  #include <xen/event.h>
> >  #include <xen/grant_table.h>
> > @@ -11,6 +12,8 @@
> >  #include <xen/sizes.h>
> >  #include <xen/vmap.h>
> >  
> > +#include <public/io/xs_wire.h>
> > +
> >  #include <asm/arm64/sve.h>
> >  #include <asm/dom0less-build.h>
> >  #include <asm/domain_build.h>
> > @@ -704,6 +707,53 @@ static int __init alloc_xenstore_evtchn(struct domain *d)
> >      return 0;
> >  }
> >  
> > +#define XENSTORE_PFN_OFFSET 1
> > +static int __init alloc_xenstore_page(struct domain *d)
> > +{
> > +    struct page_info *xenstore_pg;
> > +    struct xenstore_domain_interface *interface;
> > +    mfn_t mfn;
> > +    gfn_t gfn;
> > +    int rc;
> > +
> > +    if ( (UINT_MAX - d->max_pages) < 1 )
> > +    {
> > +        printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages by 1 page.\n",
> > +               d);
> > +        return -EINVAL;
> > +    }
> empty line here

Sure


> > +    d->max_pages += 1;
> If this patch is separate from modifying init-dom0less, max_pages will be bumped twice. Here and in init-dom0less.
> Shouldn't we fold it in? The rest is ok.

OK

[PATCH v5 4/9] tools/init-dom0less: Avoid hardcoding GUEST_MAGIC_BASE

[Stefano Stabellini]

From: Henry Wang <xin.wang2@amd.com>

Currently the GUEST_MAGIC_BASE in the init-dom0less application is
hardcoded, which will lead to failures for 1:1 direct-mapped Dom0less
DomUs.

Since the guest magic region allocation from init-dom0less is for
XenStore, and the XenStore page is now allocated from the hypervisor,
instead of hardcoding the guest magic pages region, use
xc_hvm_param_get() to get the XenStore page PFN. Rename alloc_xs_page()
to get_xs_page() to reflect the changes.

With this change, some existing code is not needed anymore, including:
(1) The definition of the XenStore page offset.
(2) Call to xc_domain_setmaxmem() and xc_clear_domain_page() as we
    don't need to set the max mem and clear the page anymore.
(3) Foreign mapping of the XenStore page, setting of XenStore interface
    status and HVM_PARAM_STORE_PFN from init-dom0less, as they are set
    by the hypervisor.

Take the opportunity to do some coding style improvements when possible.

Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
CC: anthony@xenproject.org
---
 tools/helpers/init-dom0less.c | 58 +++++++++--------------------------
 1 file changed, 14 insertions(+), 44 deletions(-)

diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
index fee93459c4..2b51965fa7 100644
--- a/tools/helpers/init-dom0less.c
+++ b/tools/helpers/init-dom0less.c
@@ -16,30 +16,18 @@
 
 #include "init-dom-json.h"
 
-#define XENSTORE_PFN_OFFSET 1
 #define STR_MAX_LENGTH 128
 
-static int alloc_xs_page(struct xc_interface_core *xch,
-                         libxl_dominfo *info,
-                         uint64_t *xenstore_pfn)
+static int get_xs_page(struct xc_interface_core *xch, libxl_dominfo *info,
+                       uint64_t *xenstore_pfn)
 {
     int rc;
-    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
-    xen_pfn_t p2m = (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET;
 
-    rc = xc_domain_setmaxmem(xch, info->domid,
-                             info->max_memkb + (XC_PAGE_SIZE/1024));
-    if (rc < 0)
-        return rc;
-
-    rc = xc_domain_populate_physmap_exact(xch, info->domid, 1, 0, 0, &p2m);
-    if (rc < 0)
-        return rc;
-
-    *xenstore_pfn = base + XENSTORE_PFN_OFFSET;
-    rc = xc_clear_domain_page(xch, info->domid, *xenstore_pfn);
-    if (rc < 0)
-        return rc;
+    rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_PFN, xenstore_pfn);
+    if (rc < 0) {
+        printf("Failed to get HVM_PARAM_STORE_PFN\n");
+        return 1;
+    }
 
     return 0;
 }
@@ -100,6 +88,7 @@ static bool do_xs_write_vm(struct xs_handle *xsh, xs_transaction_t t,
  */
 static int create_xenstore(struct xs_handle *xsh,
                            libxl_dominfo *info, libxl_uuid uuid,
+                           uint64_t xenstore_pfn,
                            evtchn_port_t xenstore_port)
 {
     domid_t domid;
@@ -145,8 +134,7 @@ static int create_xenstore(struct xs_handle *xsh,
     rc = snprintf(target_memkb_str, STR_MAX_LENGTH, "%"PRIu64, info->current_memkb);
     if (rc < 0 || rc >= STR_MAX_LENGTH)
         return rc;
-    rc = snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
-                  (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET);
+    rc = snprintf(ring_ref_str, STR_MAX_LENGTH, "%"PRIu64, xenstore_pfn);
     if (rc < 0 || rc >= STR_MAX_LENGTH)
         return rc;
     rc = snprintf(xenstore_port_str, STR_MAX_LENGTH, "%u", xenstore_port);
@@ -230,7 +218,6 @@ static int init_domain(struct xs_handle *xsh,
     libxl_uuid uuid;
     uint64_t xenstore_evtchn, xenstore_pfn;
     int rc;
-    struct xenstore_domain_interface *intf;
 
     printf("Init dom0less domain: %u\n", info->domid);
 
@@ -245,20 +232,11 @@ static int init_domain(struct xs_handle *xsh,
     if (!xenstore_evtchn)
         return 0;
 
-    /* Alloc xenstore page */
-    if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
-        printf("Error on alloc magic pages\n");
-        return 1;
-    }
-
-    intf = xenforeignmemory_map(xfh, info->domid, PROT_READ | PROT_WRITE, 1,
-                                &xenstore_pfn, NULL);
-    if (!intf) {
-        printf("Error mapping xenstore page\n");
+    /* Get xenstore page */
+    if (get_xs_page(xch, info, &xenstore_pfn) != 0) {
+        printf("Error on getting xenstore page\n");
         return 1;
     }
-    intf->connection = XENSTORE_RECONNECT;
-    xenforeignmemory_unmap(xfh, intf, 1);
 
     rc = xc_dom_gnttab_seed(xch, info->domid, true,
                             (xen_pfn_t)-1, xenstore_pfn, 0, 0);
@@ -272,19 +250,11 @@ static int init_domain(struct xs_handle *xsh,
     if (rc)
         err(1, "gen_stub_json_config");
 
-    /* Now everything is ready: set HVM_PARAM_STORE_PFN */
-    rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
-                          xenstore_pfn);
-    if (rc < 0)
-        return rc;
-
-    rc = create_xenstore(xsh, info, uuid, xenstore_evtchn);
+    rc = create_xenstore(xsh, info, uuid, xenstore_pfn, xenstore_evtchn);
     if (rc)
         err(1, "writing to xenstore");
 
-    rc = xs_introduce_domain(xsh, info->domid,
-            (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
-            xenstore_evtchn);
+    rc = xs_introduce_domain(xsh, info->domid, xenstore_pfn, xenstore_evtchn);
     if (!rc)
         err(1, "xs_introduce_domain");
     return 0;
-- 
2.25.1

[PATCH v5 5/9] docs/features/dom0less: Update the late XenStore init protocol

[Stefano Stabellini]

From: Henry Wang <xin.wang2@amd.com>

With the new allocation strategy of Dom0less DomUs XenStore page,
update the doc of the late XenStore init protocol accordingly.

Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
 docs/features/dom0less.pandoc | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/docs/features/dom0less.pandoc b/docs/features/dom0less.pandoc
index 725afa0558..8b178edee0 100644
--- a/docs/features/dom0less.pandoc
+++ b/docs/features/dom0less.pandoc
@@ -110,9 +110,10 @@ hotplug PV drivers to dom0less guests. E.g. xl network-attach domU.
 The implementation works as follows:
 - Xen allocates the xenstore event channel for each dom0less domU that
   has the "xen,enhanced" property, and sets HVM_PARAM_STORE_EVTCHN
-- Xen does *not* allocate the xenstore page and sets HVM_PARAM_STORE_PFN
-  to ~0ULL (invalid)
-- Dom0less domU kernels check that HVM_PARAM_STORE_PFN is set to invalid
+- Xen allocates the xenstore page and sets HVM_PARAM_STORE_PFN as well
+  as the connection status to XENSTORE_RECONNECT.
+- Dom0less domU kernels check that HVM_PARAM_STORE_PFN is set to
+  ~0ULL (invalid) or the connection status is *not* XENSTORE_CONNECTED.
     - Old kernels will continue without xenstore support (Note: some old
       buggy kernels might crash because they don't check the validity of
       HVM_PARAM_STORE_PFN before using it! Disable "xen,enhanced" in
@@ -121,13 +122,14 @@ The implementation works as follows:
       channel (HVM_PARAM_STORE_EVTCHN) before continuing with the
       initialization
 - Once dom0 is booted, init-dom0less is executed:
-    - it allocates the xenstore shared page and sets HVM_PARAM_STORE_PFN
+    - it gets the xenstore shared page from HVM_PARAM_STORE_PFN
     - it calls xs_introduce_domain
 - Xenstored notices the new domain, initializes interfaces as usual, and
   sends an event channel notification to the domain using the xenstore
   event channel (HVM_PARAM_STORE_EVTCHN)
 - The Linux domU kernel receives the event channel notification, checks
-  HVM_PARAM_STORE_PFN again and continue with the initialization
+  HVM_PARAM_STORE_PFN and the connection status again and continue with
+  the initialization
 
 
 Limitations
-- 
2.25.1

[PATCH v5 6/9] automation: add ping test to static-mem test

[Stefano Stabellini]

With the recent fixes, Dom0less direct mapped domains can use PV
drivers. Extend the existing PV network ping tests to direct mapped
guests.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 automation/scripts/qemu-smoke-dom0less-arm64.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/automation/scripts/qemu-smoke-dom0less-arm64.sh b/automation/scripts/qemu-smoke-dom0less-arm64.sh
index 83e1866ca6..f72d209361 100755
--- a/automation/scripts/qemu-smoke-dom0less-arm64.sh
+++ b/automation/scripts/qemu-smoke-dom0less-arm64.sh
@@ -25,6 +25,9 @@ if [[ "${test_variant}" == "static-mem" ]]; then
     domU_check="
 mem_range=$(printf \"%08x-%08x\" ${domu_base} $(( ${domu_base} + ${domu_size} - 1 )))
 if grep -q -x \"\${mem_range} : System RAM\" /proc/iomem; then
+    until ifconfig eth0 192.168.0.2 &> /dev/null && ping -c 10 192.168.0.1; do
+        sleep 30
+    done
     echo \"${passed}\"
 fi
 "
-- 
2.25.1

[PATCH v5 7/9] init-dom0less: allocate xenstore page is not already allocated

[Stefano Stabellini]

We check if the xenstore page is already allocated. If yes, there is
nothing to do. If no, we proceed allocating it.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 tools/helpers/init-dom0less.c | 54 +++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
index 2b51965fa7..3cee325358 100644
--- a/tools/helpers/init-dom0less.c
+++ b/tools/helpers/init-dom0less.c
@@ -16,8 +16,35 @@
 
 #include "init-dom-json.h"
 
+#define XENSTORE_PFN_OFFSET 1
 #define STR_MAX_LENGTH 128
 
+
+static int alloc_xs_page(struct xc_interface_core *xch,
+                         libxl_dominfo *info,
+                         uint64_t *xenstore_pfn)
+{
+    int rc;
+    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
+    xen_pfn_t p2m = (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET;
+
+    rc = xc_domain_setmaxmem(xch, info->domid,
+                             info->max_memkb + (XC_PAGE_SIZE/1024));
+    if (rc < 0)
+        return rc;
+
+    rc = xc_domain_populate_physmap_exact(xch, info->domid, 1, 0, 0, &p2m);
+    if (rc < 0)
+        return rc;
+
+    *xenstore_pfn = base + XENSTORE_PFN_OFFSET;
+    rc = xc_clear_domain_page(xch, info->domid, *xenstore_pfn);
+    if (rc < 0)
+        return rc;
+
+    return 0;
+}
+
 static int get_xs_page(struct xc_interface_core *xch, libxl_dominfo *info,
                        uint64_t *xenstore_pfn)
 {
@@ -233,9 +260,30 @@ static int init_domain(struct xs_handle *xsh,
         return 0;
 
     /* Get xenstore page */
-    if (get_xs_page(xch, info, &xenstore_pfn) != 0) {
-        printf("Error on getting xenstore page\n");
-        return 1;
+    if (get_xs_page(xch, info, &xenstore_pfn) != 0 || xenstore_pfn == ~0ULL) {
+        struct xenstore_domain_interface *intf;
+
+        rc = alloc_xs_page(xch, info, &xenstore_pfn);
+        if (rc != 0) {
+            printf("Error on getting xenstore page\n");
+            return 1;
+        }
+
+        intf = xenforeignmemory_map(xfh, info->domid, PROT_READ | PROT_WRITE, 1,
+                                    &xenstore_pfn, NULL);
+        if (!intf) {
+            printf("Error mapping xenstore page\n");
+            return 1;
+        }
+
+        intf->connection = XENSTORE_RECONNECT;
+        xenforeignmemory_unmap(xfh, intf, 1);
+
+        /* Now everything is ready: set HVM_PARAM_STORE_PFN */
+        rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
+                xenstore_pfn);
+        if (rc < 0)
+            return rc;
     }
 
     rc = xc_dom_gnttab_seed(xch, info->domid, true,
-- 
2.25.1

Re: [PATCH v5 7/9] init-dom0less: allocate xenstore page is not already allocated

[Jan Beulich]

On 06.02.2025 02:08, Stefano Stabellini wrote:
> --- a/tools/helpers/init-dom0less.c
> +++ b/tools/helpers/init-dom0less.c
> @@ -16,8 +16,35 @@
>  
>  #include "init-dom-json.h"
>  
> +#define XENSTORE_PFN_OFFSET 1
>  #define STR_MAX_LENGTH 128
>  
> +
> +static int alloc_xs_page(struct xc_interface_core *xch,

While this isn't my area of maintainership, may I nevertheless ask that you
please avoid introducing double blank lines?

Jan

Re: [PATCH v5 7/9] init-dom0less: allocate xenstore page is not already allocated

[Stefano Stabellini]

On Thu, 6 Feb 2025, Jan Beulich wrote:
> On 06.02.2025 02:08, Stefano Stabellini wrote:
> > --- a/tools/helpers/init-dom0less.c
> > +++ b/tools/helpers/init-dom0less.c
> > @@ -16,8 +16,35 @@
> >  
> >  #include "init-dom-json.h"
> >  
> > +#define XENSTORE_PFN_OFFSET 1
> >  #define STR_MAX_LENGTH 128
> >  
> > +
> > +static int alloc_xs_page(struct xc_interface_core *xch,
> 
> While this isn't my area of maintainership, may I nevertheless ask that you
> please avoid introducing double blank lines?

Sure, thanks Jan

[PATCH v5 8/9] xen/arm: introduce legacy dom0less option for xenstore allocation

[Stefano Stabellini]

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(-)

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 {
-- 
2.25.1

Re: [PATCH v5 8/9] xen/arm: introduce legacy dom0less option for xenstore allocation

[Orzel, Michal]

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

Re: [PATCH v5 8/9] xen/arm: introduce legacy dom0less option for xenstore allocation

[Stefano Stabellini]

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

[PATCH v5 9/9] [DO NOT COMMIT] automation: add one test using an older unpatched Linux kernel

[Stefano Stabellini]

The original patch series broke compatibility with older Linux kernels.
In the meantime, Linux backported a fix that improves the general
behavior and also resolve the problem.

However, we still want to check Xen against possible regressions, even
against old unpatches kernels. We can use the older Linux kernel version
we had to do that.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 automation/gitlab-ci/build.yaml                 | 11 +++++++++++
 automation/gitlab-ci/test.yaml                  | 10 ++++++++++
 automation/scripts/qemu-smoke-dom0less-arm64.sh |  7 +++++--
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index 411b4902b5..0a867c3ced 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -269,6 +269,17 @@ alpine-3.18-arm64-rootfs-export:
   tags:
     - arm64
 
+kernel-5.19-arm64-export:
+  extends: .test-jobs-artifact-common
+  image: registry.gitlab.com/xen-project/xen/tests-artifacts/kernel:5.19-arm64v8
+  script:
+    - mkdir binaries && cp /Image binaries/Image
+  artifacts:
+    paths:
+      - binaries/Image
+  tags:
+    - arm64
+
 kernel-6.6.74-arm64-export:
   extends: .test-jobs-artifact-common
   image: registry.gitlab.com/xen-project/xen/tests-artifacts/kernel:6.6.74-arm64v8
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 6ad45269ea..06ee2fcc7e 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -335,6 +335,16 @@ qemu-smoke-dom0less-arm64-gcc-debug:
     - *arm64-test-needs
     - alpine-3.18-gcc-debug-arm64
 
+qemu-smoke-dom0less-arm64-gcc-debug-old:
+  extends: .qemu-arm64
+  script:
+    - ./automation/scripts/qemu-smoke-dom0less-arm64.sh old 2>&1 | tee ${LOGFILE}
+  needs:
+    - alpine-3.18-arm64-rootfs-export
+    - qemu-system-aarch64-6.0.0-arm64-export
+    - alpine-3.18-gcc-debug-arm64
+    - kernel-5.19-arm64-export
+
 qemu-smoke-dom0less-arm64-gcc-debug-gicv3:
   extends: .qemu-arm64
   script:
diff --git a/automation/scripts/qemu-smoke-dom0less-arm64.sh b/automation/scripts/qemu-smoke-dom0less-arm64.sh
index f72d209361..ddb158987a 100755
--- a/automation/scripts/qemu-smoke-dom0less-arm64.sh
+++ b/automation/scripts/qemu-smoke-dom0less-arm64.sh
@@ -7,7 +7,7 @@ test_variant=$1
 # Default GIC version
 gic_version="2"
 
-if [ -z "${test_variant}" ]; then
+if [ -z "${test_variant}" -o "${test_variant}" == "old" ]; then
     passed="ping test passed"
     domU_check="
 until ifconfig eth0 192.168.0.2 &> /dev/null && ping -c 10 192.168.0.1; do
@@ -203,7 +203,10 @@ fi
 rm -rf imagebuilder
 git clone --depth 1 https://gitlab.com/xen-project/imagebuilder.git
 bash imagebuilder/scripts/uboot-script-gen -t tftp -d binaries/ -c binaries/config
-
+if [ "${test_variant}" == "old" ]; then
+    sed -i "s/enabled/legacy/g" binaries/boot.source
+    mkimage -A arm64 -T script -C none -a 0x40200000 -e 0x40200000 -d binaries/boot.source binaries/boot.scr
+fi
 
 # Run the test
 rm -f smoke.serial
-- 
2.25.1