Re: [PATCH] x86/shadow: don't use #if in macro invocations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.02.2026 16:29, Andrew Cooper wrote:
> On 18/02/2026 9:03 am, Jan Beulich wrote:
>> As per the standard this is UB, i.e. we're building on a defacto extension
>> in the compilers we use. Misra C:2012 rule 20.6 disallows this altogether,
>> though. Use helper always-inline functions instead.
>>
>> In sh_audit_l1_table(), along with reducing the scope of "gfn", which now
>> isn't used anymore by the if() side of the conditional, also reduce the
>> scope of two other adjacent variables.
>>
>> For audit_magic() note that both which parameters are needed and what
>> their types are is attributed to AUDIT_FAIL() accessing variables which
>> aren't passed as arguments to it.
>>
>> No functional change intended. Of course codegen does change with this,
>> first and foremost in register allocation.
>>
>> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> I included this patch on an interim branch of other MISRA fixes of mine
> to get a run.
> 
> https://gitlab.com/xen-project/hardware/xen-staging/-/jobs/13198988953
> 
> There's one more violation still to fix:
> 
>     if ( unlikely((level == 1)
>                   && sh_mfn_is_a_page_table(target_mfn)
> #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC )
>                   /* Unless the page is out of sync and the guest is
>                      writing to it. */
>                   && !(mfn_oos_may_write(target_mfn)
>                        && (ft == ft_demand_write))
> #endif /* OOS */
>                   ) )
>         sflags &= ~_PAGE_RW;
> 
> 
> 
> I also looked at this one previously.  Making mfn_oos_may_write()
> visible outside of SHOPT_OUT_OF_SYNC is quite invasive.

We could simply add a stub returning constant true for the !OOS case.

> Here, I suggest dropping the unlikely() as the easiest fix.  It's almost
> certainly useless anyway.

Especially when used around an && expression.

I may want to go a little further there, if already we need to touch this,
combining the two adjacent "level == 1" checks:

    if ( level == 1 )
    {
        /* Protect guest page tables. */
        if ( unlikely(sh_mfn_is_a_page_table(target_mfn))
#if SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC
             /*
              * Unless the page is out of sync and the guest is writing to it.
              */
             && (ft != ft_demand_write || !mfn_oos_may_write(target_mfn))
#endif /* OOS */
           )
            sflags &= ~_PAGE_RW;

        /*
         * paging_mode_log_dirty support
         *
         * Only allow the guest write access to a page a) on a demand fault,
         * or b) if the page is already marked as dirty.
         *
         * (We handle log-dirty entirely inside the shadow code, without using
         * the p2m_ram_logdirty p2m type: only HAP uses that.)
         */
        if ( unlikely(paging_mode_log_dirty(d)) && !mmio_mfn )
        {
            if ( ft & FETCH_TYPE_WRITE )
                paging_mark_dirty(d, target_mfn);
            else if ( (sflags & _PAGE_RW) &&
                      !paging_mfn_is_dirty(d, target_mfn) )
                sflags &= ~_PAGE_RW;
        }
    }

Thoughts?

The two different ways of checking for "guest is writing" also look somewhat
unhelpful. But there's yet another "ft & FETCH_TYPE_WRITE" elsewhere, so it
may want to be a separate patch to switch to uniformly comparing against
ft_demand_write.

Jan