xen/common/kernel.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
XENFEAT_pae_pgdir_above_4gb is only used by 32bit PV guests, hence limit
its setting in XENVER_get_features to the is_pv_domain() guarded section,
with the rest of the PV specific features.
No functional change intended.
Signed-off-by: Roger Pau Monné <roger@xenproject.org>
---
xen/common/kernel.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index f4bbd8818fa8..95cf56827317 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -677,8 +677,6 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
(1U << XENFEAT_vcpu_time_phys_area) |
#endif
(1U << XENFEAT_runstate_phys_area);
- if ( VM_ASSIST(d, pae_extended_cr3) )
- fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb);
if ( paging_mode_translate(d) )
fi.submap |=
(1U << XENFEAT_writable_page_tables) |
@@ -692,7 +690,9 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
if ( is_pv_domain(d) )
fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) |
(1U << XENFEAT_highmem_assist) |
- (1U << XENFEAT_gnttab_map_avail_bits);
+ (1U << XENFEAT_gnttab_map_avail_bits) |
+ (VM_ASSIST(d, pae_extended_cr3) ?
+ (1U << XENFEAT_pae_pgdir_above_4gb) : 0);
else
fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) |
(1U << XENFEAT_hvm_callback_vector) |
--
2.55.0
On 30.09.2026 21:11, Roger Pau Monne wrote:
> @@ -692,7 +690,9 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> if ( is_pv_domain(d) )
> fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) |
> (1U << XENFEAT_highmem_assist) |
> - (1U << XENFEAT_gnttab_map_avail_bits);
> + (1U << XENFEAT_gnttab_map_avail_bits) |
> + (VM_ASSIST(d, pae_extended_cr3) ?
> + (1U << XENFEAT_pae_pgdir_above_4gb) : 0);
Nit: Unlike binary operators, which we want to have placed at the end of
wrapped lines, we generally prefer the ternary operator to be wrapped
differently. E.g.
(VM_ASSIST(d, pae_extended_cr3)
? (1U << XENFEAT_pae_pgdir_above_4gb) : 0);
or
(VM_ASSIST(d, pae_extended_cr3)
? (1U << XENFEAT_pae_pgdir_above_4gb)
: 0);
This isn't written down anywhere, so I'm not going to insist.
Furthermore, wouldn't it be plausible to also add IS_ENABLED(CONFIG_PV32)
(or, imo less desirably, opt_pv32)?
Jan
On Thu, Oct 01, 2026 at 09:34:34AM +0200, Jan Beulich wrote:
> On 30.09.2026 21:11, Roger Pau Monne wrote:
> > @@ -692,7 +690,9 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> > if ( is_pv_domain(d) )
> > fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) |
> > (1U << XENFEAT_highmem_assist) |
> > - (1U << XENFEAT_gnttab_map_avail_bits);
> > + (1U << XENFEAT_gnttab_map_avail_bits) |
> > + (VM_ASSIST(d, pae_extended_cr3) ?
> > + (1U << XENFEAT_pae_pgdir_above_4gb) : 0);
>
> Nit: Unlike binary operators, which we want to have placed at the end of
> wrapped lines, we generally prefer the ternary operator to be wrapped
> differently. E.g.
>
> (VM_ASSIST(d, pae_extended_cr3)
> ? (1U << XENFEAT_pae_pgdir_above_4gb) : 0);
>
> or
>
> (VM_ASSIST(d, pae_extended_cr3)
> ? (1U << XENFEAT_pae_pgdir_above_4gb)
> : 0);
>
> This isn't written down anywhere, so I'm not going to insist.
I had it written as your first proposal, but then moved the ternary
operator at the end of the line as I've assumed it would be prefer to
match the position of the binary ones.
> Furthermore, wouldn't it be plausible to also add IS_ENABLED(CONFIG_PV32)
> (or, imo less desirably, opt_pv32)?
I could do:
if ( IS_ENABLED(CONFIG_PV32) && VM_ASSIST(d, pae_extended_cr3) )
fi.submap |= 1U << XENFEAT_pae_pgdir_above_4gb
As I think adding the IS_ENABLED(CONFIG_PV32) check to the previous
expression is too much.
Let me know your preference.
Thanks, Roger.
On 01.10.2026 10:29, Roger Pau Monné wrote: > On Thu, Oct 01, 2026 at 09:34:34AM +0200, Jan Beulich wrote: >> On 30.09.2026 21:11, Roger Pau Monne wrote: >>> @@ -692,7 +690,9 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>> if ( is_pv_domain(d) ) >>> fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) | >>> (1U << XENFEAT_highmem_assist) | >>> - (1U << XENFEAT_gnttab_map_avail_bits); >>> + (1U << XENFEAT_gnttab_map_avail_bits) | >>> + (VM_ASSIST(d, pae_extended_cr3) ? >>> + (1U << XENFEAT_pae_pgdir_above_4gb) : 0); >> >> Nit: Unlike binary operators, which we want to have placed at the end of >> wrapped lines, we generally prefer the ternary operator to be wrapped >> differently. E.g. >> >> (VM_ASSIST(d, pae_extended_cr3) >> ? (1U << XENFEAT_pae_pgdir_above_4gb) : 0); >> >> or >> >> (VM_ASSIST(d, pae_extended_cr3) >> ? (1U << XENFEAT_pae_pgdir_above_4gb) >> : 0); >> >> This isn't written down anywhere, so I'm not going to insist. > > I had it written as your first proposal, but then moved the ternary > operator at the end of the line as I've assumed it would be prefer to > match the position of the binary ones. > >> Furthermore, wouldn't it be plausible to also add IS_ENABLED(CONFIG_PV32) >> (or, imo less desirably, opt_pv32)? > > I could do: > > if ( IS_ENABLED(CONFIG_PV32) && VM_ASSIST(d, pae_extended_cr3) ) > fi.submap |= 1U << XENFEAT_pae_pgdir_above_4gb > > As I think adding the IS_ENABLED(CONFIG_PV32) check to the previous > expression is too much. > > Let me know your preference. I'm okay with the if() variant, but as you know I'm also not afraid of long-ish ternary expressions. Jan
On 01/10/2026 9:29 am, Roger Pau Monné wrote:
> On Thu, Oct 01, 2026 at 09:34:34AM +0200, Jan Beulich wrote:
>> On 30.09.2026 21:11, Roger Pau Monne wrote:
>>> @@ -692,7 +690,9 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>> if ( is_pv_domain(d) )
>>> fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) |
>>> (1U << XENFEAT_highmem_assist) |
>>> - (1U << XENFEAT_gnttab_map_avail_bits);
>>> + (1U << XENFEAT_gnttab_map_avail_bits) |
>>> + (VM_ASSIST(d, pae_extended_cr3) ?
>>> + (1U << XENFEAT_pae_pgdir_above_4gb) : 0);
>> Nit: Unlike binary operators, which we want to have placed at the end of
>> wrapped lines, we generally prefer the ternary operator to be wrapped
>> differently. E.g.
>>
>> (VM_ASSIST(d, pae_extended_cr3)
>> ? (1U << XENFEAT_pae_pgdir_above_4gb) : 0);
>>
>> or
>>
>> (VM_ASSIST(d, pae_extended_cr3)
>> ? (1U << XENFEAT_pae_pgdir_above_4gb)
>> : 0);
>>
>> This isn't written down anywhere, so I'm not going to insist.
> I had it written as your first proposal, but then moved the ternary
> operator at the end of the line as I've assumed it would be prefer to
> match the position of the binary ones.
>
>> Furthermore, wouldn't it be plausible to also add IS_ENABLED(CONFIG_PV32)
>> (or, imo less desirably, opt_pv32)?
> I could do:
>
> if ( IS_ENABLED(CONFIG_PV32) && VM_ASSIST(d, pae_extended_cr3) )
> fi.submap |= 1U << XENFEAT_pae_pgdir_above_4gb
>
> As I think adding the IS_ENABLED(CONFIG_PV32) check to the previous
> expression is too much.
>
> Let me know your preference.
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index f4bbd8818fa8..6811599f95fd 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -677,8 +677,6 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
(1U << XENFEAT_vcpu_time_phys_area) |
#endif
(1U << XENFEAT_runstate_phys_area);
- if ( VM_ASSIST(d, pae_extended_cr3) )
- fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb);
if ( paging_mode_translate(d) )
fi.submap |=
(1U << XENFEAT_writable_page_tables) |
@@ -690,9 +688,13 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
#endif
#ifdef CONFIG_X86
if ( is_pv_domain(d) )
+ {
fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) |
(1U << XENFEAT_highmem_assist) |
(1U << XENFEAT_gnttab_map_avail_bits);
+ if ( VM_ASSIST(d, pae_extended_cr3) )
+ fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb);
+ }
else
fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) |
(1U << XENFEAT_hvm_callback_vector) |
Please don't try to be clever trying to fold this logic. All that does
is require someone to revert it later when it inevitably gets too
complicated follow sensibly.
~Andrew
Le 30/09/2026 à 21:14, Roger Pau Monne a écrit : > XENFEAT_pae_pgdir_above_4gb is only used by 32bit PV guests, hence limit > its setting in XENVER_get_features to the is_pv_domain() guarded section, > with the rest of the PV specific features. > > No functional change intended. > > Signed-off-by: Roger Pau Monné <roger@xenproject.org> > --- > xen/common/kernel.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/xen/common/kernel.c b/xen/common/kernel.c > index f4bbd8818fa8..95cf56827317 100644 > --- a/xen/common/kernel.c > +++ b/xen/common/kernel.c > @@ -677,8 +677,6 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > (1U << XENFEAT_vcpu_time_phys_area) | > #endif > (1U << XENFEAT_runstate_phys_area); > - if ( VM_ASSIST(d, pae_extended_cr3) ) > - fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb); > if ( paging_mode_translate(d) ) > fi.submap |= > (1U << XENFEAT_writable_page_tables) | > @@ -692,7 +690,9 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > if ( is_pv_domain(d) ) > fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) | > (1U << XENFEAT_highmem_assist) | > - (1U << XENFEAT_gnttab_map_avail_bits); > + (1U << XENFEAT_gnttab_map_avail_bits) | > + (VM_ASSIST(d, pae_extended_cr3) ? > + (1U << XENFEAT_pae_pgdir_above_4gb) : 0); > else > fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) | > (1U << XENFEAT_hvm_callback_vector) | Reviewed-by: Teddy Astie <teddy.astie@vates.tech> Teddy
© 2016 - 2026 Red Hat, Inc.