Re: [PATCH 4/5] x86/shadow: split a nested max() invocation

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2026-05-13 13:46, Jan Beulich wrote:
> Such nesting causes the inner instance to shadow the outer instance's
> macro-local variables, thus violating Misra C:2012 rule 5.3 ("An
> identifier declared in an inner scope shall not hide an identifier
> declared in an outer scope"). Use an intermediate variable for the
> inner invocation. No difference in generated code.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Supposedly this case is deviated (rules.rst). Does that deviation not 
> work
> quite right? Actually, am I mis-reading deviations.ecl or is the
> respective setting only covering the combination of min() and max(), 
> but
> not multiple use of the same macro? Furthermore, why would e.g.
> min(max_t(), ...) need a deviation? Even more generally, aren't those
> expressions too permissive?
Yeah, it does cover only mixing max(_t)?/min(_t)? because that was the 
only pattern that emerged in the safety scope originally. the _t is not 
there for mixing e.g. min(max_t(...), ...) but rather for 
min_t(max_t(...), ...) and viceversa; could be split if you think it's 
worth it. These expressions are a tad broad, because it's way more 
complicated to express the condition: "ignore overlapping only beetween 
identifiers defined in the expansion of max/min when used together". 
Perhaps it could be done, but then if you are already implicitly using 
shadowing in those instances maybe that's not as serious a concern?
In any case, the patch looks ok to me, so

Reviewed-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>

> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -767,11 +767,12 @@ static unsigned int sh_min_allocation(co
>       * megabyte of RAM (for the p2m table, minimally enough for HVM's 
> setting
>       * up of slot zero and an LAPIC page), plus one for HVM's 1-to-1 
> pagetable.
>       */
> +    unsigned int extra = max(domain_tot_pages(d) / 256,
> +                             is_hvm_domain(d) ? CONFIG_PAGING_LEVELS + 
> 2 : 0U) +
> +                         is_hvm_domain(d);
> +
>      return shadow_min_acceptable_pages(d) +
> -           max(max(domain_tot_pages(d) / 256,
> -                   is_hvm_domain(d) ? CONFIG_PAGING_LEVELS + 2 : 0U) +
> -               is_hvm_domain(d),
> -               d->arch.paging.p2m_pages);
> +           max(extra, d->arch.paging.p2m_pages);
>  }
>
>  int shadow_set_allocation(struct domain *d, unsigned int pages, bool 
> *preempted)
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253