Re: [PATCH v3 3/6] x86: extend mod_l*_entry() to optionally return the old PTE value

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.10.2026 18:25, Andrew Cooper wrote:
> On 01/10/2026 6:47 pm, Kevin Lampis wrote:
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index 1ddb8b71469a..535a62139cad 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -2154,7 +2154,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;
>> @@ -2215,7 +2216,12 @@ 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);
>> +            if ( ol1e_out )
>> +                *ol1e_out = UPDATE_ENTRY_SWAP(l1, pl1e, ol1e, nl1e, gl1mfn,
>> +                                              pt_vcpu, update_flags);
>> +            else
>> +                UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, 
>> update_flags);
>> +
> 
> This is a reoccurring pattern.   Also, from the subsequent patch, this
> is the caller's reoccuring pattern:
> 
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index
>> 535a62139cad..a2d05d6c7113 100644 --- a/xen/arch/x86/mm.c +++
>> b/xen/arch/x86/mm.c @@ -4111,6 +4111,7 @@ long do_mmu_update( case
>> MMU_NORMAL_PT_UPDATE: case MMU_PT_UPDATE_PRESERVE_AD: case
>> MMU_PT_UPDATE_NO_TRANSLATE: + case MMU_PT_UPDATE_SWAP: { p2m_type_t
>> p2mt; @@ -4172,10 +4173,27 @@ long do_mmu_update( update_flags =
>> PTE_UPDATE_PRESERVE_AD; else if ( cmd == MMU_PT_UPDATE_NO_TRANSLATE )
>> update_flags = PTE_UPDATE_NO_TRANSLATE; + else if ( cmd ==
>> MMU_PT_UPDATE_SWAP ) + update_flags = PTE_UPDATE_SWAP; switch (
>> page->u.inuse.type_info & PGT_type_mask ) { case PGT_l1_page_table: +
>> if ( cmd == MMU_PT_UPDATE_SWAP ) + { + l1_pgentry_t ol1e; + rc =
>> mod_l1_entry(va, l1e_from_intpte(req.val), mfn, + update_flags, v,
>> pg_owner, &ol1e); + + if ( !rc ) + { + req.val = ol1e.l1; + if (
>> unlikely(copy_to_guest(ureqs, &req, 1)) ) + rc = -EFAULT; + } + break;
>> + } + rc = mod_l1_entry(va, l1e_from_intpte(req.val), mfn,
>> update_flags, v, pg_owner, NULL); break;
> 
> 
> Both are rather intractable as far as logic goes.   We've got
> MMU_PT_UPDATE_SWAP setting flags=PTE_UPDATE_SWAP which both gets passed
> in flags and expressed through the NULL-ness of of the ol?e_out pointer,
> which then choses UPDATE_ENTRY() or UPDATE_ENTRY_SWAP().
> 
> The only thing UPDATE_ENTRY_SWAP() is doing is wrapping the return
> intpte_t into a typesafe variant.  It doesn't play with PTE_UPDATE_SWAP,
> so the name is misleading.  It's unfortunate that we can't wrap
> UPDATE_ENTRY(), but I think we'll just have to live with that.
> 
> I think the logic in mod_l?_entry() wants to be along these lines:
> 
>     intpte_t old = UPDATE_ENTRY(...);
>     if ( ol1e_out )
>         *ol1e_out = l1e_from_intpte(old);

This would then require that all callers, ...

> and the logic in do_mmu_update() wants to be along these lines:
> 
>     {
>         l1_pgentry_t ol1e;
> 
>         rc = mod_l1_entry(..., &ol1e);

... e.g. the one here, initialize the local variable to 0.

Jan