Re: [PATCH v5 2/6] x86: rearrange how mod_l*_entry() functions call UPDATE_ENTRY

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/11/2026 10:00 am, Kevin Lampis wrote:
> bloat-o-meter:
> Function                                     old     new   delta
> mod_l1_entry                                1186    1113     -73
> do_mmu_update                               5546    5392    -154

This was only for the sample I pasted inline.  For this patch it's

add/remove: 0/0 grow/shrink: 1/5 up/down: 10/-641 (-631)
Function                                     old     new   delta
mod_l1_entry.cold                             56      66     +10
mod_l1_entry                                1891    1885      -6
mod_l4_entry.cold                             39      31      -8
do_mmu_update.cold                           122     112     -10
mod_l4_entry                                 917     760    -157
do_mmu_update                               5745    5285    -460

although it's not particularly relevant to live in the commit message. 
(I use it as a litmus test that the changes are along the expected lines.)

For the commit message, you probably want "Fold two UPDATE_ENTRY() paths
together, in preparation for an upcoming change." or so.

>
> Signed-off-by: Kevin Lampis <kevin.lampis@xxxxxxxxxx>
>
> ---
> Changes in v5:
> - New patch
> ---
>  xen/arch/x86/mm.c | 45 +++++++++++++++++++--------------------------
>  1 file changed, 19 insertions(+), 26 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index e42cd33c0d09..dc0e0d451c02 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2197,12 +2197,12 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, 
> l1_pgentry_t nl1e,
>  {
>      l1_pgentry_t ol1e = l1e_read(pl1e);
>      struct domain *pt_dom = pt_vcpu->domain;
> -    int rc = 0;
>  
>      ASSERT(!paging_mode_refcounts(pt_dom));
>  
>      if ( l1e_get_flags(nl1e) & _PAGE_PRESENT )
>      {
> +        int rc;
>          struct page_info *page = NULL;
>          uint32_t disallow = l1_disallow_mask(pt_dom);
>  
> @@ -2276,16 +2276,14 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, 
> l1_pgentry_t nl1e,
>          }
>          if ( page )
>              put_page(page);
> -
> -        UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, update_flags);
>      }
>      else if ( pv_l1tf_check_l1e(pt_dom, nl1e) )
>          return -ERESTART;
> -    else
> -        UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, update_flags);
> +
> +    UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, update_flags);
>  
>      put_page_from_l1e(ol1e, pt_dom);
> -    return rc;
> +    return 0;
>  }
>  
>  
> @@ -2300,7 +2298,6 @@ static int mod_l2_entry(l2_pgentry_t *pl2e,
>      struct domain *d = vcpu->domain;
>      struct page_info *l2pg = mfn_to_page(mfn);
>      unsigned long type = l2pg->u.inuse.type_info;
> -    int rc = 0;
>  
>      if ( unlikely(!is_guest_l2_slot(d, type, pgentry_ptr_to_slot(pl2e))) )
>      {
> @@ -2313,6 +2310,8 @@ static int mod_l2_entry(l2_pgentry_t *pl2e,
>  
>      if ( l2e_get_flags(nl2e) & _PAGE_PRESENT )
>      {
> +        int rc;
> +
>          if ( unlikely(l2e_get_flags(nl2e) & L2_DISALLOW_MASK) )
>          {
>              gdprintk(XENLOG_WARNING, "Bad L2 flags %x\n",
> @@ -2331,17 +2330,15 @@ static int mod_l2_entry(l2_pgentry_t *pl2e,
>  
>          if ( unlikely((rc = get_page_from_l2e(nl2e, mfn, d, 0)) < 0) )
>              return rc;

Looking further through the call tree, I'm now confident that
get_page_from_l?e() can't return a positive number.   Continuations are
already expressed as -ERESTART.

So, either we want:

    if ( rc )
        return rc;

or:

    if ( rc < 0 )
        return rc;
    ASSERT(rc == 0);

and (whichever we choose), we want to bring l2 in line with l3 and l4.

Given that there are paths under get_page_from_l?e() which do have
positive return values which are supposed to be unused in this
particular call tree, I'm leaning towards the latter.

~Andrew