xen/x86: Check supported features even for PHV dom0

[PATCH] xen/x86: Check supported features even for PHV dom0

Posted by Frediano Ziglio 1 week, 1 day ago

The supported features ELF note was tested only if the dom0 was
PV. Factor out a function to check ELF notes and reuse it even
for PVH.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 xen/arch/x86/dom0_build.c             | 16 ++++++++++++++++
 xen/arch/x86/hvm/dom0_build.c         |  3 +++
 xen/arch/x86/include/asm/dom0_build.h |  2 ++
 xen/arch/x86/pv/dom0_build.c          | 10 ++--------
 4 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 864dd9e53e..c6bb2f8067 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -321,6 +321,22 @@ unsigned long __init dom0_paging_pages(const
struct domain *d,
 }


+int __init dom0_check_parms(
+    const struct elf_dom_parms *parms, bool is_pv_shim)
+{
+    if ( parms->elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type !=
XEN_ENT_NONE )
+    {
+        if ( !is_pv_shim && !test_bit(XENFEAT_dom0, parms->f_supported) )
+        {
+            printk("Kernel does not support Dom0 operation\n");
+            return -EINVAL;
+        }
+    }
+
+    return 0;
+}
+
+
 /*
  * If allocation isn't specified, reserve 1/16th of available memory for
  * things like DMA buffers. This reservation is clamped to a maximum of 128MB.
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index d69a83b089..ca96f32acd 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -699,6 +699,9 @@ static int __init pvh_load_kernel(
     if ( !check_and_adjust_load_address(d, &elf, &parms) )
         return -ENOSPC;

+    if ( (rc = dom0_check_parms(&parms, false)) != 0 )
+        return rc;
+
     elf_set_vcpu(&elf, v);
     rc = elf_load_binary(&elf);
     if ( rc < 0 )
diff --git a/xen/arch/x86/include/asm/dom0_build.h
b/xen/arch/x86/include/asm/dom0_build.h
index ff021c24af..a322bf455c 100644
--- a/xen/arch/x86/include/asm/dom0_build.h
+++ b/xen/arch/x86/include/asm/dom0_build.h
@@ -8,6 +8,8 @@

 extern unsigned int dom0_memflags;

+int dom0_check_parms(const struct elf_dom_parms *parms,
+                     bool is_pv_shim);
 unsigned long dom0_compute_nr_pages(struct domain *d,
                                     struct elf_dom_parms *parms,
                                     unsigned long initrd_len);
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 075a3646c2..9d0310ad91 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -494,14 +494,8 @@ static int __init dom0_construct(const struct
boot_domain *bd)
         return -EINVAL;
     }

-    if ( parms.elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type != XEN_ENT_NONE )
-    {
-        if ( !pv_shim && !test_bit(XENFEAT_dom0, parms.f_supported) )
-        {
-            printk("Kernel does not support Dom0 operation\n");
-            return -EINVAL;
-        }
-    }
+    if ( (rc = dom0_check_parms(&parms, pv_shim)) != 0 )
+        goto out;

     nr_pages = dom0_compute_nr_pages(d, &parms, initrd_len);

-- 
2.43.0

Re: [PATCH] xen/x86: Check supported features even for PHV dom0

Posted by Jan Beulich 1 week ago

On 25.03.2026 16:55, Frediano Ziglio wrote:
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -321,6 +321,22 @@ unsigned long __init dom0_paging_pages(const
> struct domain *d,
>  }
> 
> 
> +int __init dom0_check_parms(
> +    const struct elf_dom_parms *parms, bool is_pv_shim)
> +{
> +    if ( parms->elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type !=
> XEN_ENT_NONE )
> +    {
> +        if ( !is_pv_shim && !test_bit(XENFEAT_dom0, parms->f_supported) )
> +        {
> +            printk("Kernel does not support Dom0 operation\n");
> +            return -EINVAL;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +

Nit: Rather than adding another bogus pair of blank lines, pleases leverage
the two that there by inserting between them.

> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -494,14 +494,8 @@ static int __init dom0_construct(const struct
> boot_domain *bd)
>          return -EINVAL;
>      }
> 
> -    if ( parms.elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type != XEN_ENT_NONE )
> -    {
> -        if ( !pv_shim && !test_bit(XENFEAT_dom0, parms.f_supported) )
> -        {
> -            printk("Kernel does not support Dom0 operation\n");
> -            return -EINVAL;
> -        }
> -    }
> +    if ( (rc = dom0_check_parms(&parms, pv_shim)) != 0 )
> +        goto out;

Why "goto" when it was "return" before? (This may be warranted, but if so it
needs justifying in the description.)

Jan

Re: [PATCH] xen/x86: Check supported features even for PHV dom0

Posted by Roger Pau Monné 1 week ago

Typo on the subject s/PHV/PVH/.

On Wed, Mar 25, 2026 at 03:55:28PM +0000, Frediano Ziglio wrote:
> The supported features ELF note was tested only if the dom0 was
> PV. Factor out a function to check ELF notes and reuse it even
> for PVH.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> ---
>  xen/arch/x86/dom0_build.c             | 16 ++++++++++++++++
>  xen/arch/x86/hvm/dom0_build.c         |  3 +++
>  xen/arch/x86/include/asm/dom0_build.h |  2 ++
>  xen/arch/x86/pv/dom0_build.c          | 10 ++--------
>  4 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index 864dd9e53e..c6bb2f8067 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -321,6 +321,22 @@ unsigned long __init dom0_paging_pages(const
> struct domain *d,
>  }
> 
> 
> +int __init dom0_check_parms(
> +    const struct elf_dom_parms *parms, bool is_pv_shim)
> +{
> +    if ( parms->elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type !=
> XEN_ENT_NONE )

The patch seems to be mangled here?

And the line is too long otherwise.  You might want to consider
returning early here, to reduce the indentation of the following code
block.

> +    {
> +        if ( !is_pv_shim && !test_bit(XENFEAT_dom0, parms->f_supported) )

I think you want to pass the domain being built to this function, so
you can do a check like:

if ( is_hardware_domain(d) && !test_bit(XENFEAT_dom0, parms->f_supported) )
{
    printk(...

That way you don't need to explicitly check for pv-shim mode, and
would more naturally work with things like
Hyperlaunch/dom0less/late-hwdom.

> +        {
> +            printk("Kernel does not support Dom0 operation\n");
> +            return -EINVAL;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
>  /*
>   * If allocation isn't specified, reserve 1/16th of available memory for
>   * things like DMA buffers. This reservation is clamped to a maximum of 128MB.
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index d69a83b089..ca96f32acd 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -699,6 +699,9 @@ static int __init pvh_load_kernel(
>      if ( !check_and_adjust_load_address(d, &elf, &parms) )
>          return -ENOSPC;
> 
> +    if ( (rc = dom0_check_parms(&parms, false)) != 0 )
> +        return rc;
> +
>      elf_set_vcpu(&elf, v);
>      rc = elf_load_binary(&elf);
>      if ( rc < 0 )
> diff --git a/xen/arch/x86/include/asm/dom0_build.h
> b/xen/arch/x86/include/asm/dom0_build.h
> index ff021c24af..a322bf455c 100644
> --- a/xen/arch/x86/include/asm/dom0_build.h
> +++ b/xen/arch/x86/include/asm/dom0_build.h
> @@ -8,6 +8,8 @@
> 
>  extern unsigned int dom0_memflags;
> 
> +int dom0_check_parms(const struct elf_dom_parms *parms,
> +                     bool is_pv_shim);
>  unsigned long dom0_compute_nr_pages(struct domain *d,
>                                      struct elf_dom_parms *parms,
>                                      unsigned long initrd_len);
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index 075a3646c2..9d0310ad91 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -494,14 +494,8 @@ static int __init dom0_construct(const struct
> boot_domain *bd)
>          return -EINVAL;
>      }
> 
> -    if ( parms.elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type != XEN_ENT_NONE )
> -    {
> -        if ( !pv_shim && !test_bit(XENFEAT_dom0, parms.f_supported) )
> -        {
> -            printk("Kernel does not support Dom0 operation\n");
> -            return -EINVAL;
> -        }
> -    }
> +    if ( (rc = dom0_check_parms(&parms, pv_shim)) != 0 )

pv_shim is a global variable, you don't need to pass it around.

Thanks, Roger.

Re: [PATCH] xen/x86: Check supported features even for PHV dom0

Posted by Frediano Ziglio 1 week ago

On Thu, 26 Mar 2026 at 09:59, Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> Typo on the subject s/PHV/PVH/.
>

Fixed.

> On Wed, Mar 25, 2026 at 03:55:28PM +0000, Frediano Ziglio wrote:
> > The supported features ELF note was tested only if the dom0 was
> > PV. Factor out a function to check ELF notes and reuse it even
> > for PVH.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> > ---
> >  xen/arch/x86/dom0_build.c             | 16 ++++++++++++++++
> >  xen/arch/x86/hvm/dom0_build.c         |  3 +++
> >  xen/arch/x86/include/asm/dom0_build.h |  2 ++
> >  xen/arch/x86/pv/dom0_build.c          | 10 ++--------
> >  4 files changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> > index 864dd9e53e..c6bb2f8067 100644
> > --- a/xen/arch/x86/dom0_build.c
> > +++ b/xen/arch/x86/dom0_build.c
> > @@ -321,6 +321,22 @@ unsigned long __init dom0_paging_pages(const
> > struct domain *d,
> >  }
> >
> >
> > +int __init dom0_check_parms(
> > +    const struct elf_dom_parms *parms, bool is_pv_shim)
> > +{
> > +    if ( parms->elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type !=
> > XEN_ENT_NONE )
>
> The patch seems to be mangled here?
>

email client, sorry about it.

> And the line is too long otherwise.  You might want to consider
> returning early here, to reduce the indentation of the following code
> block.
>

The line is actually exactly 80 characters, so it fits.

What about combining the 2 ifs instead ? In this case I would probably
need to split the line.

> > +    {
> > +        if ( !is_pv_shim && !test_bit(XENFEAT_dom0, parms->f_supported) )
>
> I think you want to pass the domain being built to this function, so
> you can do a check like:
>
> if ( is_hardware_domain(d) && !test_bit(XENFEAT_dom0, parms->f_supported) )
> {
>     printk(...
>
> That way you don't need to explicitly check for pv-shim mode, and
> would more naturally work with things like
> Hyperlaunch/dom0less/late-hwdom.
>

It's not clear why. Are you saying that dom0 could be a no-hardware domain?
Wouldn't that change introduce a regression?

> > +        {
> > +            printk("Kernel does not support Dom0 operation\n");
> > +            return -EINVAL;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +
> >  /*
> >   * If allocation isn't specified, reserve 1/16th of available memory for
> >   * things like DMA buffers. This reservation is clamped to a maximum of 128MB.
> > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> > index d69a83b089..ca96f32acd 100644
> > --- a/xen/arch/x86/hvm/dom0_build.c
> > +++ b/xen/arch/x86/hvm/dom0_build.c
> > @@ -699,6 +699,9 @@ static int __init pvh_load_kernel(
> >      if ( !check_and_adjust_load_address(d, &elf, &parms) )
> >          return -ENOSPC;
> >
> > +    if ( (rc = dom0_check_parms(&parms, false)) != 0 )
> > +        return rc;
> > +
> >      elf_set_vcpu(&elf, v);
> >      rc = elf_load_binary(&elf);
> >      if ( rc < 0 )
> > diff --git a/xen/arch/x86/include/asm/dom0_build.h
> > b/xen/arch/x86/include/asm/dom0_build.h
> > index ff021c24af..a322bf455c 100644
> > --- a/xen/arch/x86/include/asm/dom0_build.h
> > +++ b/xen/arch/x86/include/asm/dom0_build.h
> > @@ -8,6 +8,8 @@
> >
> >  extern unsigned int dom0_memflags;
> >
> > +int dom0_check_parms(const struct elf_dom_parms *parms,
> > +                     bool is_pv_shim);
> >  unsigned long dom0_compute_nr_pages(struct domain *d,
> >                                      struct elf_dom_parms *parms,
> >                                      unsigned long initrd_len);
> > diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> > index 075a3646c2..9d0310ad91 100644
> > --- a/xen/arch/x86/pv/dom0_build.c
> > +++ b/xen/arch/x86/pv/dom0_build.c
> > @@ -494,14 +494,8 @@ static int __init dom0_construct(const struct
> > boot_domain *bd)
> >          return -EINVAL;
> >      }
> >
> > -    if ( parms.elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type != XEN_ENT_NONE )
> > -    {
> > -        if ( !pv_shim && !test_bit(XENFEAT_dom0, parms.f_supported) )
> > -        {
> > -            printk("Kernel does not support Dom0 operation\n");
> > -            return -EINVAL;
> > -        }
> > -    }
> > +    if ( (rc = dom0_check_parms(&parms, pv_shim)) != 0 )
>
> pv_shim is a global variable, you don't need to pass it around.
>

Okay, but in the other call I was always passing false. What do you think?

> Thanks, Roger.

Frediano

Re: [PATCH] xen/x86: Check supported features even for PHV dom0

Posted by Roger Pau Monné 1 week ago

On Thu, Mar 26, 2026 at 07:14:45PM +0000, Frediano Ziglio wrote:
> On Thu, 26 Mar 2026 at 09:59, Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > Typo on the subject s/PHV/PVH/.
> >
> 
> Fixed.
> 
> > On Wed, Mar 25, 2026 at 03:55:28PM +0000, Frediano Ziglio wrote:
> > > The supported features ELF note was tested only if the dom0 was
> > > PV. Factor out a function to check ELF notes and reuse it even
> > > for PVH.
> > >
> > > Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> > > ---
> > >  xen/arch/x86/dom0_build.c             | 16 ++++++++++++++++
> > >  xen/arch/x86/hvm/dom0_build.c         |  3 +++
> > >  xen/arch/x86/include/asm/dom0_build.h |  2 ++
> > >  xen/arch/x86/pv/dom0_build.c          | 10 ++--------
> > >  4 files changed, 23 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> > > index 864dd9e53e..c6bb2f8067 100644
> > > --- a/xen/arch/x86/dom0_build.c
> > > +++ b/xen/arch/x86/dom0_build.c
> > > @@ -321,6 +321,22 @@ unsigned long __init dom0_paging_pages(const
> > > struct domain *d,
> > >  }
> > >
> > >
> > > +int __init dom0_check_parms(
> > > +    const struct elf_dom_parms *parms, bool is_pv_shim)
> > > +{
> > > +    if ( parms->elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type !=
> > > XEN_ENT_NONE )
> >
> > The patch seems to be mangled here?
> >
> 
> email client, sorry about it.
> 
> > And the line is too long otherwise.  You might want to consider
> > returning early here, to reduce the indentation of the following code
> > block.
> >
> 
> The line is actually exactly 80 characters, so it fits.

Sorry, I didn't count correctly it seems then.

> What about combining the 2 ifs instead ? In this case I would probably
> need to split the line.

I wouldn't combine, in case more XENFEAT_* needs to be tested in the
future (I doubt, but you never know).

> 
> > > +    {
> > > +        if ( !is_pv_shim && !test_bit(XENFEAT_dom0, parms->f_supported) )
> >
> > I think you want to pass the domain being built to this function, so
> > you can do a check like:
> >
> > if ( is_hardware_domain(d) && !test_bit(XENFEAT_dom0, parms->f_supported) )
> > {
> >     printk(...
> >
> > That way you don't need to explicitly check for pv-shim mode, and
> > would more naturally work with things like
> > Hyperlaunch/dom0less/late-hwdom.
> >
> 
> It's not clear why. Are you saying that dom0 could be a no-hardware domain?
> Wouldn't that change introduce a regression?

TBH it's unclear to me what capabilities does XENFEAT_dom0 signal.  I
assume it's the ability of a PV kernel to boot as the hardware domain,
which is slightly different from a normal PV domain.

AFAIK dom0 could be a control domain only, and delegate the
hardware management to a hardware domain.  Whether the current code
can really do so I have no idea, as I've never tested it.

I don't think the proposed change introduces a regression.  It
limits the XENFEAT_dom0 checking to it's intended domain type (if my
understanding of XENFEAT_dom0 is correct).

> > > +        {
> > > +            printk("Kernel does not support Dom0 operation\n");
> > > +            return -EINVAL;
> > > +        }
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +
> > >  /*
> > >   * If allocation isn't specified, reserve 1/16th of available memory for
> > >   * things like DMA buffers. This reservation is clamped to a maximum of 128MB.
> > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> > > index d69a83b089..ca96f32acd 100644
> > > --- a/xen/arch/x86/hvm/dom0_build.c
> > > +++ b/xen/arch/x86/hvm/dom0_build.c
> > > @@ -699,6 +699,9 @@ static int __init pvh_load_kernel(
> > >      if ( !check_and_adjust_load_address(d, &elf, &parms) )
> > >          return -ENOSPC;
> > >
> > > +    if ( (rc = dom0_check_parms(&parms, false)) != 0 )
> > > +        return rc;
> > > +
> > >      elf_set_vcpu(&elf, v);
> > >      rc = elf_load_binary(&elf);
> > >      if ( rc < 0 )
> > > diff --git a/xen/arch/x86/include/asm/dom0_build.h
> > > b/xen/arch/x86/include/asm/dom0_build.h
> > > index ff021c24af..a322bf455c 100644
> > > --- a/xen/arch/x86/include/asm/dom0_build.h
> > > +++ b/xen/arch/x86/include/asm/dom0_build.h
> > > @@ -8,6 +8,8 @@
> > >
> > >  extern unsigned int dom0_memflags;
> > >
> > > +int dom0_check_parms(const struct elf_dom_parms *parms,
> > > +                     bool is_pv_shim);
> > >  unsigned long dom0_compute_nr_pages(struct domain *d,
> > >                                      struct elf_dom_parms *parms,
> > >                                      unsigned long initrd_len);
> > > diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> > > index 075a3646c2..9d0310ad91 100644
> > > --- a/xen/arch/x86/pv/dom0_build.c
> > > +++ b/xen/arch/x86/pv/dom0_build.c
> > > @@ -494,14 +494,8 @@ static int __init dom0_construct(const struct
> > > boot_domain *bd)
> > >          return -EINVAL;
> > >      }
> > >
> > > -    if ( parms.elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type != XEN_ENT_NONE )
> > > -    {
> > > -        if ( !pv_shim && !test_bit(XENFEAT_dom0, parms.f_supported) )
> > > -        {
> > > -            printk("Kernel does not support Dom0 operation\n");
> > > -            return -EINVAL;
> > > -        }
> > > -    }
> > > +    if ( (rc = dom0_check_parms(&parms, pv_shim)) != 0 )
> >
> > pv_shim is a global variable, you don't need to pass it around.
> >
> 
> Okay, but in the other call I was always passing false. What do you think?

I think just using pv_shim directly in dom0_check_parms() is easier to
follow rather than passing it around.  But with my suggestion above
to use is_hardware_domain() you might not need to check for pv_shim.

Thanks, Roger.