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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/11/2026 4:13 pm, Andrew Cooper wrote:
> 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.

In the Gitlab run, I put in ASSERT()s to this effect, and nothing
exploded during real testing.

https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/3046070649

Anyway, I think an intermediate patch rearranging each of mod_l?_entry()
in the above way will be a good move.

~Andrew