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

[Frediano Ziglio <freddy77@xxxxxxxxx>]

On Thu, 26 Mar 2026 at 09:59, Roger Pau Monné <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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