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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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);

and the logic in do_mmu_update() wants to be along these lines:

    {
        l1_pgentry_t ol1e;

        rc = mod_l1_entry(..., &ol1e);

        if ( !rc && cmd == MMU_PT_UPDATE_SWAP )
            ...

        break;
    }


I think this gets us the same behaviour as before, while keeping the
same (single) calls to mod_l?_entry() and UPDATE_ENTRY().

~Andrew