Re: [PATCH v2 5/7] x86: extend do_mmu_update() to support returning the old PTE value

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 10/09/2026 à 22:31, Kevin Lampis a écrit :
> A new sub-op MMU_PT_UPDATE_SWAP when used will do an atomic swap to set
> the new value and return the old PTE value through the req.val field.
>
> - The new PTE value in req.val must be 0, at the moment this is intended
>    for clearing only.
I'm not sure this is a good idea to restrict the feature for only 0. If 
in the future we want to expand this to non-0 values, we will need to 
advertise this property to the guest as well (so that it knows if non-0 
values are supported). I don't know what supporting !0 would require 
(but it could be useful to get a idea of it).
> - 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 MMU_PT_UPDATE_SWAP is not set then the old behavior is preserved
>    do_mmu_update -> mod_l1_entry -> UPDATE_ENTRY -> paging_write_guest_entry
>
> The new MMU_PT_UPDATE_SWAP call chain looks like this
>    do_mmu_update -> mod_l1_entry -> UPDATE_ENTRY -> paging_cmpxchg_guest_entry
>
> Signed-off-by: Kevin Lampis <kevin.lampis@xxxxxxxxxx>
> ---
> Changes in v2:
> - Add a sub-op to mmu_update instead of new hypercall
> - Rename to "swap" instead of "clear", although only `0` is accepted
> - Remove spurious curly brace from `case PGT_l1_page_table:`
> - Restructure `case PGT_l1_page_table:` to avoid diff churn
> - Add missing `MMU_PT_UPDATE_SWAP` check for the second
>    `PGT_writable_page` block
>
> (Jan) I decided not to remove the `rc = -EINVAL` lines. Even though `rc`
> already has this value I think it's more readable like this. Let me know
> if I misunderstood the issue.
> ---
>   xen/arch/x86/mm.c        | 48 +++++++++++++++++++++++++++++++++++++++-
>   xen/include/public/xen.h |  1 +
>   2 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 59ad5ce16486..b498a8460935 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4055,6 +4055,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;
>   
> @@ -4118,6 +4119,30 @@ long do_mmu_update(
(...)

> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index 2149b8dd3808..559190688a1e 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -349,6 +349,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
>   #define MMU_PT_UPDATE_PRESERVE_AD  2 /* atomically: *ptr = val | (*ptr&(A|D)) 
> */
>   #define MMU_PT_UPDATE_NO_TRANSLATE 3 /* checked '*ptr = val'. ptr is MA.      
> */
>                                        /* val never translated.                 
> */
> +#define MMU_PT_UPDATE_SWAP         4
That wants some documentation : the operation it does, and the 
restrictions that applies (i.e the fact it's L1-only, restrictions, ...).
>   
>   /*
>    * MMU EXTENDED OPERATIONS
Teddy

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature