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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

Is it a real extension, or just something that happens to work?

>  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.

This is grammatically awkward.  IMO it would be clearer to say "For
audit_magic() note that there are more parameters than might seem
necessary, caused by the expectations of AUDIT_FAIL()." 

>
> 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>

Definitely a more surgical fix than my approach.

I was attempting to turn FOREACH_*() into real for() loops behind the
scenes, so we don't have the {code} as a parameter, and get real break's
rather than magic behaviour with the _done variable.  The problem is
that ever helper is different, and there's rather more hidden under the
covers than appear at first glance.

I'd still like to make this approach work in due course, for the ease of
following the logic if nothing else, but its hard enough to not be a
couple of hours work, and not high enough on my priority list right now.

> ---
> Leaving even the fetching of current to the helper in
> sh_rm_write_access_from_l1() looks tidier to me overall, albeit this means
> the fetch will now occur once per present L1E.

This will not make a dent in the performance of the shadow code.

> Converting the #if to if() and #ifdef to if(IS_ENABLED()) wouldn't work
> here, as identifiers are used which aren't available when the respective
> conditions are false.

Personally, I'd have put this in the main commit message, because it's
the justification for why out-of-line static inline's need to be used.

>
> Note that I tested this only on top of
> https://lists.xen.org/archives/html/xen-devel/2023-05/msg01140.html, but I
> have no reason to assume that there is a problematic interaction. Of
> course it would be really nice if the rest of that series finally could go
> in.
>
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -395,7 +395,7 @@ static inline mfn_t cf_check sh_next_pag
>      shadow_set_l2e(d, sl2e, new_sl2e, sl2mfn, SH_type_fl1_shadow, 
> sh_next_page)
>  
>  static inline u32
> -guest_index(void *ptr)
> +guest_index(const void *ptr)
>  {
>      return (u32)((unsigned long)ptr & ~PAGE_MASK) / sizeof(guest_l1e_t);
>  }

While fine per say, this doesn't appear to be related to the patch?

Everything else looks fine.

~Andrew