Re: [PATCH v2 1/7] x86: make UPDATE_ENTRY() allow for multiple operation flags

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 10/09/2026 à 22:31, Kevin Lampis a écrit :
> From: Jan Beulich <jbeulich@xxxxxxxx>
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> ---
> Changes in v2:
> - New patch
> ---
>   xen/arch/x86/mm.c    | 48 ++++++++++++++++++++++++--------------------
>   xen/arch/x86/pv/mm.h | 13 +++++++-----
>   2 files changed, 34 insertions(+), 27 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index b158742408f9..5f778557b1e6 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2149,10 +2149,9 @@ 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 cmd,
> +                        mfn_t gl1mfn, unsigned int update_flags,
>                           struct vcpu *pt_vcpu, struct domain *pg_dom)
>   {
> -    bool preserve_ad = (cmd == MMU_PT_UPDATE_PRESERVE_AD);
>       l1_pgentry_t ol1e = l1e_read(pl1e);
>       struct domain *pt_dom = pt_vcpu->domain;
>       int rc = 0;
> @@ -2172,7 +2171,7 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t 
> nl1e,
>           }
>   
>           /* Translate foreign guest address. */
> -        if ( cmd != MMU_PT_UPDATE_NO_TRANSLATE &&
> +        if ( !(update_flags & PTE_UPDATE_NO_TRANSLATE) &&
>                paging_mode_translate(pg_dom) )
>           {
>               p2m_type_t p2mt;
(...)

> @@ -2435,7 +2434,7 @@ static int mod_l4_entry(l4_pgentry_t *pl4e,
>       else if ( pv_l1tf_check_l4e(d, nl4e) )
>           return -ERESTART;
>       else if ( unlikely(!UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu,
> -                                     preserve_ad)) )
> +                                     update_flags)) )
>       {
>           return -EFAULT;
>       }
> @@ -4135,18 +4134,23 @@ long do_mmu_update(
>   
>               if ( page_lock(page) )
>               {
> +                unsigned int update_flags = (cmd == MMU_PT_UPDATE_PRESERVE_AD)
> +                                            ? PTE_UPDATE_PRESERVE_AD
> +                                            : (cmd == 
> MMU_PT_UPDATE_NO_TRANSLATE)
> +                                              ? PTE_UPDATE_NO_TRANSLATE : 0;
> +
I'm not fully convinced by the double-ternary operator here. While it's 
a bit more compact, I don't find it very readable, I would prefer 
something like
unsigned int update_flags = 0;
if ( cmd == MMU_PT_UPDATE_PRESERVE_AD )
    update_flags = PTE_UPDATE_PRESERVE_AD;
if ( cmd == MMU_PT_UPDATE_NO_TRANSLATE )
    update_flags = PTE_UPDATE_NO_TRANSLATE;

Which is one line longer, but looks to me more clear on what it does.

>                   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);
> +                                      update_flags, v, pg_owner);
>                       break;
>   
(...)

The rest looks good to me though.

Teddy

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature