Re: [PATCH] xen/x86: restrict XENFEAT_pae_pgdir_above_4gb setting to PV guests

[Andrew Cooper <andrew.cooper3@citrix.com>]

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