Re: [PATCH 3/4] x86: extend do_mmu_update() to support returning the old PTE value

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.07.2026 17:06, Kevin Lampis wrote:
> A new parameter write_back_old when set will return the old PTE value.
> 
> - The new PTE value in req.val must be 0, this is for clearing only.

As per Teddy's comment it needs to be determined whether we really want to
limit this to "clear". Even if we do for now, naming of the new sub-op may
want to be such that relaxing later is an option.

> - The PRESERVE_AD flag is rejected with -EINVAL because preserving
>   Accessed/Dirty bits into a zero'ed PTE doesn't make sense.
> 
> - Only l1 PTEs are supported because they are the most frequent and have the
>   biggest performance impact.
> 
> - The old PTE value is passed back to the guest through the req.val field
> 
> If the write_back_old flag is not set then the old behavior is preserved
>   do_mmu_update -> mod_l1_entry -> UPDATE_ENTRY -> paging_write_guest_entry
> 
> The new get_and_clear call chain looks like this
>   do_mmu_update -> mod_l1_entry -> update_intpte -> paging_cmpxchg_guest_entry
> 
> Signed-off-by: Kevin Lampis <kevin.lampis@xxxxxxxxxx>

Apart from the above only a couple of cosmetic comments, as based on other
replies to this series things will likely change quite a bit here.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3988,11 +3988,12 @@ long do_mmuext_op(
>      return rc;
>  }
>  
> -long do_mmu_update(
> +static long __do_mmu_update(

No need for two leading underscores (making the identifier a reserved one),
when one will do. In fact with this becoming a local helper, I question the
need for a prefix altogether: Just mmu_update() would likely do.

> @@ -4144,13 +4145,41 @@ long do_mmu_update(
>                  switch ( page->u.inuse.type_info & PGT_type_mask )
>                  {
>                  case PGT_l1_page_table:
> -                    rc = mod_l1_entry(va, l1e_from_intpte(req.val), mfn,
> -                                      cmd, v, pg_owner, NULL);
> +                {

Why this curly brace, when there are no declarations?

> +                    if ( !write_back_old )
> +                        rc = mod_l1_entry(va, l1e_from_intpte(req.val), mfn,
> +                                          cmd, v, pg_owner, NULL);

Even this little bit of churn could be avoided if you inserted ...

> +                    else

                    if ( write_back_old )

... above the existing code, and ...

> +                    {
> +                        l1_pgentry_t ol1e;
> +                        if ( unlikely(req.val != 0 ||
> +                                      cmd == MMU_PT_UPDATE_PRESERVE_AD) )
> +                        {
> +                            rc = -EINVAL;
> +                            break;
> +                        }
> +
> +                        rc = mod_l1_entry(va, l1e_from_intpte(req.val), mfn,
> +                                          cmd, v, pg_owner, &ol1e);
> +
> +                        if ( !rc )
> +                        {
> +                            req.val = ol1e.l1;
> +                            if ( unlikely(copy_to_guest(ureqs, &req, 1)) )
> +                                rc = -EFAULT;
> +                        }

... a separate "break" here.

> +                    }
>                      break;
> +                }
>  
>                  case PGT_l2_page_table:
>                      if ( unlikely(pg_owner != pt_owner) )
>                          break;
> +                    if ( unlikely(write_back_old) )
> +                    {
> +                        rc = -EINVAL;

rc already is -EINVAL when make it here, isn't it? That's also leveraged by
the owner check visible in context. (Same for the further cases below,
obviously.)

> @@ -4198,6 +4237,11 @@ long do_mmu_update(
>                      break;
>  
>                  case PGT_writable_page:
> +                    if ( unlikely(write_back_old) )
> +                    {
> +                        rc = -EINVAL;
> +                        break;
> +                    }
>                      perfc_incr(writable_mmu_updates);
>                      paging_write_guest_entry(v, va, req.val, mfn);
>                      rc = 0;

Below here there's a copy of this PGT_writable_page handling, which looks
as if it also wants to reject write_back_old being true.

Jan