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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@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.

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.