[PATCH 0/5] x86: Misra rule 5.3

[PATCH 0/5] x86: Misra rule 5.3

[Jan Beulich]

1: guest: rename a local variable
2: PV: rename a local variable in pv_emulate_gate_op()
3: shadow: conditionalize / rename local variables
4: shadow: split a nested max() invocation
5: shadow: rename a parameter of shadow_l<N>_index()

For reference (also covering the one rule 5.6 I'm going to send):
https://gitlab.com/xen-project/hardware/xen-staging/-/jobs/14348506098

Jan

[PATCH 1/5] x86/guest: rename a local variable

[Jan Beulich]

... shadowing a file scope one, thuis violating Misra C:2012 rule 5.3
("An identifier declared in an inner scope shall not hide an identifier
declared in an outer scope"). No difference in generated code.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/guest/xen/xen.c
+++ b/xen/arch/x86/guest/xen/xen.c
@@ -203,11 +203,11 @@ static void __init init_memmap(void)
 
 static void cf_check xen_evtchn_upcall(void)
 {
-    struct vcpu_info *vcpu_info = this_cpu(vcpu_info);
+    struct vcpu_info *vi = this_cpu(vcpu_info);
     unsigned long pending;
 
-    vcpu_info->evtchn_upcall_pending = 0;
-    pending = xchg(&vcpu_info->evtchn_pending_sel, 0);
+    vi->evtchn_upcall_pending = 0;
+    pending = xchg(&vi->evtchn_pending_sel, 0);
 
     while ( pending )
     {

[PATCH 2/5] x86/PV: rename a local variable in pv_emulate_gate_op()

[Jan Beulich]

... shadowing a function scope one, 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"). No difference in generated code.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Technically, as the outer scope "rc" isn't used again later, we could
simply drop the inner decl. That seemed more error prone to me, though.

--- a/xen/arch/x86/pv/emul-gate-op.c
+++ b/xen/arch/x86/pv/emul-gate-op.c
@@ -286,16 +286,16 @@ void pv_emulate_gate_op(struct cpu_user_
     if ( !jump )
     {
         unsigned int ss, esp, *stkp;
-        int rc;
+        int left;
 #define push(item) do \
         { \
             --stkp; \
             esp -= 4; \
-            rc = __put_guest(item, stkp); \
-            if ( rc ) \
+            left = __put_guest(item, stkp); \
+            if ( left ) \
             { \
                 pv_inject_page_fault(PFEC_write_access, \
-                                     (unsigned long)(stkp + 1) - rc); \
+                                     (unsigned long)(stkp + 1) - left); \
                 return; \
             } \
         } while ( 0 )
@@ -359,10 +359,11 @@ void pv_emulate_gate_op(struct cpu_user_
                     unsigned int parm;
 
                     --ustkp;
-                    rc = __get_guest(parm, ustkp);
-                    if ( rc )
+                    left = __get_guest(parm, ustkp);
+                    if ( left )
                     {
-                        pv_inject_page_fault(0, (unsigned long)(ustkp + 1) - rc);
+                        pv_inject_page_fault(0,
+                                             (unsigned long)(ustkp + 1) - left);
                         return;
                     }
                     push(parm);

[PATCH 3/5] x86/shadow: conditionalize / rename local variables

[Jan Beulich]

... shadowing a function scope one in one case and the global _end[] in
another, 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").
No difference in generated code.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -857,13 +857,13 @@ do {
 /* 64-bit l2: touch all entries except for PAE compat guests. */
 #define FOREACH_PRESENT_L2E(_sl2mfn, _sl2e, _gl2p, _done, _dom, _code)      \
 do {                                                                        \
-    unsigned int _i, _end = SHADOW_L2_PAGETABLE_ENTRIES;                    \
+    unsigned int _i, _nr = SHADOW_L2_PAGETABLE_ENTRIES;                     \
     shadow_l2e_t *_sp = map_domain_page((_sl2mfn));                         \
     ASSERT_VALID_L2(mfn_to_page(_sl2mfn)->u.sh.type);                       \
     if ( is_pv_32bit_domain(_dom) /* implies !paging_mode_external */ &&    \
          mfn_to_page(_sl2mfn)->u.sh.type != SH_type_l2_64_shadow )          \
-        _end = COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(_dom);                    \
-    for ( _i = 0; _i < _end; ++_i )                                         \
+        _nr = COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(_dom);                     \
+    for ( _i = 0; _i < _nr; ++_i )                                          \
     {                                                                       \
         (_sl2e) = _sp + _i;                                                 \
         if ( shadow_l2e_get_flags(*(_sl2e)) & _PAGE_PRESENT )               \
@@ -3349,7 +3349,9 @@ static pagetable_t cf_check sh_update_cr
 #if SHADOW_PAGING_LEVELS == 3
         {
             mfn_t smfn = pagetable_get_mfn(v->arch.paging.shadow.shadow_table[0]);
+#if GUEST_PAGING_LEVELS != 3
             unsigned int i;
+#endif
 
             for_each_shadow_table(v, i)
             {

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

[Jan Beulich]

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@suse.com>
---
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?

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

[PATCH 5/5] x86/shadow: rename a parameter of shadow_l<N>_index()

[Jan Beulich]

... shadowing a file scope one, 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").

While there,
- replace u32 by uint32_t,
- reduce the number of cf_check by aliasing shadow_l<N>_index() to
  shadow_l1_index() for N > 1 and GUEST_PAGING_LEVELS > 2.

No difference in generated code, except of course the removal of the
duplicate function instances.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -402,46 +402,39 @@ guest_index(const void *ptr)
     return (u32)((unsigned long)ptr & ~PAGE_MASK) / sizeof(guest_l1e_t);
 }
 
-static u32 cf_check shadow_l1_index(mfn_t *smfn, u32 guest_index)
+static uint32_t cf_check shadow_l1_index(mfn_t *smfn, uint32_t gidx)
 {
 #if (GUEST_PAGING_LEVELS == 2)
     ASSERT(mfn_to_page(*smfn)->u.sh.head);
-    if ( guest_index >= SHADOW_L1_PAGETABLE_ENTRIES )
+    if ( gidx >= SHADOW_L1_PAGETABLE_ENTRIES )
         *smfn = sh_next_page(*smfn);
-    return (guest_index % SHADOW_L1_PAGETABLE_ENTRIES);
+    return (gidx % SHADOW_L1_PAGETABLE_ENTRIES);
 #else
-    return guest_index;
+    return gidx;
 #endif
 }
 
-static u32 cf_check shadow_l2_index(mfn_t *smfn, u32 guest_index)
-{
 #if (GUEST_PAGING_LEVELS == 2)
+static uint32_t cf_check shadow_l2_index(mfn_t *smfn, uint32_t gidx)
+{
     int i;
     ASSERT(mfn_to_page(*smfn)->u.sh.head);
     // Because we use 2 shadow l2 entries for each guest entry, the number of
     // guest entries per shadow page is SHADOW_L2_PAGETABLE_ENTRIES/2
-    for ( i = 0; i < guest_index / (SHADOW_L2_PAGETABLE_ENTRIES / 2); i++ )
+    for ( i = 0; i < gidx / (SHADOW_L2_PAGETABLE_ENTRIES / 2); i++ )
         *smfn = sh_next_page(*smfn);
     // We multiply by two to get the index of the first of the two entries
     // used to shadow the specified guest entry.
-    return (guest_index % (SHADOW_L2_PAGETABLE_ENTRIES / 2)) * 2;
+    return (gidx % (SHADOW_L2_PAGETABLE_ENTRIES / 2)) * 2;
+}
 #else
-    return guest_index;
+#define shadow_l2_index shadow_l1_index
 #endif
-}
 
 #if GUEST_PAGING_LEVELS >= 4
 
-static u32 cf_check shadow_l3_index(mfn_t *smfn, u32 guest_index)
-{
-    return guest_index;
-}
-
-static u32 cf_check shadow_l4_index(mfn_t *smfn, u32 guest_index)
-{
-    return guest_index;
-}
+#define shadow_l3_index shadow_l1_index
+#define shadow_l4_index shadow_l1_index
 
 #endif // GUEST_PAGING_LEVELS >= 4
 

Re: [PATCH 1/5] x86/guest: rename a local variable

[Nicola Vetrini]

On 2026-05-13 13:44, Jan Beulich wrote:
> ... shadowing a file scope one, thuis violating Misra C:2012 rule 5.3
> ("An identifier declared in an inner scope shall not hide an identifier
> declared in an outer scope"). No difference in generated code.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 

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

> --- a/xen/arch/x86/guest/xen/xen.c
> +++ b/xen/arch/x86/guest/xen/xen.c
> @@ -203,11 +203,11 @@ static void __init init_memmap(void)
> 
>  static void cf_check xen_evtchn_upcall(void)
>  {
> -    struct vcpu_info *vcpu_info = this_cpu(vcpu_info);
> +    struct vcpu_info *vi = this_cpu(vcpu_info);
>      unsigned long pending;
> 
> -    vcpu_info->evtchn_upcall_pending = 0;
> -    pending = xchg(&vcpu_info->evtchn_pending_sel, 0);
> +    vi->evtchn_upcall_pending = 0;
> +    pending = xchg(&vi->evtchn_pending_sel, 0);
> 
>      while ( pending )
>      {

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

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

[Nicola Vetrini]

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@suse.com>
> ---
> 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@bugseng.com>

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

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

[Jan Beulich]

On 14.05.2026 07:08, Nicola Vetrini wrote:
> 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@suse.com>
>> ---
>> 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.

Yes, I think they would better be split. We shouldn't deviate
max(min_t(), ...) and alike.

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

I understand the "more complicated" aspect, but from an assessor's pov
being too broad (lax) in deviations might easily be a negative point.

> 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@bugseng.com>

Thanks, also for all the other reviews you did.

Jan

Re: [PATCH 1/5] x86/guest: rename a local variable

[Andrew Cooper]

On 13/05/2026 12:44 pm, Jan Beulich wrote:
> ... shadowing a file scope one, thuis violating Misra C:2012 rule 5.3
> ("An identifier declared in an inner scope shall not hide an identifier
> declared in an outer scope"). No difference in generated code.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>