Re: [PATCH v4 2/5] x86: extend mod_l*_entry() to optionally return the old PTE value

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/10/2026 3:34 am, chunjie.zhu@xxxxxxxxxx wrote:
> On Fri, Oct 16, 2026 at 05:48:12 PM, Kevin Lampis <kevin.lampis@xxxxxxxxxx> 
> wrote:
>
>> Calling mod_l*_entry() with a valid ol*e_out pointer will do atomic
>> CMPXCHG to set the new pte value and return the old pte value through
>> the ol*e_out pointer. If the ol*e_out pointer is NULL then old behavior
>> is preserved.
>>
>> Signed-off-by: Kevin Lampis <kevin.lampis@xxxxxxxxxx>
>> ---
>> Changes in v4:
>> - Refactor how UPDATE_ENTRY is used
>> - Update commit message to specify CMPXCHG instead of xchg
>>
>> Changes in v3:
>> - Add swap to mod_l{2,3,4}_entry()
>>
>> Changes in v2:
>> - UPDATE_ENTRY now returns the old pte value instead of using an out
>>   pointer
>> ---
>>  xen/arch/x86/mm.c | 106 ++++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 84 insertions(+), 22 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index e42cd33c0d09..609c8595a487 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -2193,7 +2193,8 @@ static void l3t_unlock(struct page_info *page)
>>  /* Update the L1 entry at pl1e to new value nl1e. */
>>  static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e,
>>                          mfn_t gl1mfn, unsigned int update_flags,
>> -                        struct vcpu *pt_vcpu, struct domain *pg_dom)
>> +                        struct vcpu *pt_vcpu, struct domain *pg_dom,
>> +                        l1_pgentry_t *ol1e_out)
>>  {
>>      l1_pgentry_t ol1e = l1e_read(pl1e);
>>      struct domain *pt_dom = pt_vcpu->domain;
>> @@ -2254,7 +2255,11 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, 
>> l1_pgentry_t nl1e,
>>          /* Fast path for sufficiently-similar mappings. */
>>          if ( !l1e_has_changed(ol1e, nl1e, ~FASTPATH_FLAG_WHITELIST) )
>>          {
>> -            UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, 
>> update_flags);
>> +            intpte_t old = UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, 
>> pt_vcpu,
>> +                                        update_flags);
>> +            if ( ol1e_out )
>> +                *ol1e_out = l1e_from_intpte(old);
>> +
> Same "intpte_t old = UPDATE_ENTRY(); if ( olNe_out ) ..." code block is now
> repeated three times in each of mod_l{1,2,3,4}_entry().
>
> Alternatively, a single local variable per function, assigned at each call
> site and copied out once at the end. Does this avoid the duplication?

This is a bit repetitive, but there's no easy way to merge all 3
UPDATE_ENTRY() in each function.

Given this:

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index a1b7770db0f8..99d4d87e177b 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2408,7 +2408,6 @@ static int mod_l4_entry(l4_pgentry_t *pl4e,
 {
     struct domain *d = vcpu->domain;
     l4_pgentry_t ol4e;
-    int rc = 0;
 
     if ( unlikely(!is_guest_l4_slot(d, pgentry_ptr_to_slot(pl4e))) )
     {
@@ -2421,6 +2420,8 @@ static int mod_l4_entry(l4_pgentry_t *pl4e,
 
     if ( l4e_get_flags(nl4e) & _PAGE_PRESENT )
     {
+        int rc;
+
         if ( unlikely(l4e_get_flags(nl4e) & L4_DISALLOW_MASK) )
         {
             gdprintk(XENLOG_WARNING, "Bad L4 flags %x\n",
@@ -2440,17 +2441,14 @@ static int mod_l4_entry(l4_pgentry_t *pl4e,
         rc = get_page_from_l4e(nl4e, mfn, d, 0);
         if ( unlikely(rc < 0) )
             return rc;
-        rc = 0;
-
-        UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags);
     }
     else if ( pv_l1tf_check_l4e(d, nl4e) )
         return -ERESTART;
-    else
-        UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags);
+
+    UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags);
 
     put_page_from_l4e(ol4e, mfn, PTF_defer);
-    return rc;
+    return 0;
 }
 #endif /* CONFIG_PV */


we can at least turn 3 into 2, which bloat-o-meter says removes an
alarming amount of code:

    add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-165 (-165)
    Function                                     old     new   delta
    mod_l4_entry.cold                             39      31      -8
    mod_l4_entry                                 917     760    -157


That in turn means this patch only doubles up.

I'm pretty certain that the get_page_from_l?e() calls can't return a
positive number on these paths.  The one case I've found returning a
positive number is in iommu_legacy_{un,}map() but we don't pass
IOMMUF_preempt in and only operate on single pages.


Removing the final duplicate would require something like a bool
"dont_put" which is set in the !l4e_has_changed() path, but I think that
makes things more complicated rather than less.

~Andrew