* [PATCH] xen/x86: Check supported features even for PHV dom0 @ 2026-03-25 15:55 Frediano Ziglio 2026-03-26 9:59 ` Roger Pau Monné 2026-03-26 10:31 ` Jan Beulich 0 siblings, 2 replies; 5+ messages in thread From: Frediano Ziglio @ 2026-03-25 15:55 UTC (permalink / raw) To: xen-devel; +Cc: Jan Beulich, Andrew Cooper, Roger Pau Monné 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/x86: Check supported features even for PHV dom0 2026-03-25 15:55 [PATCH] xen/x86: Check supported features even for PHV dom0 Frediano Ziglio @ 2026-03-26 9:59 ` Roger Pau Monné 2026-03-26 19:14 ` Frediano Ziglio 2026-03-26 10:31 ` Jan Beulich 1 sibling, 1 reply; 5+ messages in thread From: Roger Pau Monné @ 2026-03-26 9:59 UTC (permalink / raw) To: Frediano Ziglio; +Cc: xen-devel, Jan Beulich, Andrew Cooper 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/x86: Check supported features even for PHV dom0 2026-03-26 9:59 ` Roger Pau Monné @ 2026-03-26 19:14 ` Frediano Ziglio 2026-03-26 19:53 ` Roger Pau Monné 0 siblings, 1 reply; 5+ messages in thread From: Frediano Ziglio @ 2026-03-26 19:14 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Jan Beulich, Andrew Cooper 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/x86: Check supported features even for PHV dom0 2026-03-26 19:14 ` Frediano Ziglio @ 2026-03-26 19:53 ` Roger Pau Monné 0 siblings, 0 replies; 5+ messages in thread From: Roger Pau Monné @ 2026-03-26 19:53 UTC (permalink / raw) To: Frediano Ziglio; +Cc: xen-devel, Jan Beulich, Andrew Cooper 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/x86: Check supported features even for PHV dom0 2026-03-25 15:55 [PATCH] xen/x86: Check supported features even for PHV dom0 Frediano Ziglio 2026-03-26 9:59 ` Roger Pau Monné @ 2026-03-26 10:31 ` Jan Beulich 1 sibling, 0 replies; 5+ messages in thread From: Jan Beulich @ 2026-03-26 10:31 UTC (permalink / raw) To: Frediano Ziglio; +Cc: Andrew Cooper, Roger Pau Monné, xen-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-03-26 19:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-03-25 15:55 [PATCH] xen/x86: Check supported features even for PHV dom0 Frediano Ziglio 2026-03-26 9:59 ` Roger Pau Monné 2026-03-26 19:14 ` Frediano Ziglio 2026-03-26 19:53 ` Roger Pau Monné 2026-03-26 10:31 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.