xen: Misc MISRA fixes for *-allcode targets

[PATCH 09/12] x86/shadow: Rework write_atomic() call in shadow_write_entries()

Posted by Andrew Cooper 1 week, 3 days ago

Eclair complains of a side effect in a sizeof() expression (R13.6).

write_atomic() only evaluates each parameter once, but rewrite the expression
to less resemble entries in an obfuscation contest.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Michal Orzel <michal.orzel@amd.com>
CC: consulting@bugseng.com <consulting@bugseng.com>
CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/mm/shadow/set.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/set.c b/xen/arch/x86/mm/shadow/set.c
index 8b670b6bb555..96ba2811077e 100644
--- a/xen/arch/x86/mm/shadow/set.c
+++ b/xen/arch/x86/mm/shadow/set.c
@@ -62,8 +62,8 @@ shadow_write_entries(void *d, const void *s, unsigned int entries, mfn_t mfn)
 
     ASSERT(IS_ALIGNED((unsigned long)dst, sizeof(*dst)));
 
-    for ( ; i < entries; i++ )
-        write_atomic(&dst++->l1, src++->l1);
+    for ( ; i < entries; i++, dst++, src++ )
+        write_atomic(&dst->l1, src->l1);
 
     unmap_domain_page(map);
 }
-- 
2.39.5

Re: [PATCH 09/12] x86/shadow: Rework write_atomic() call in shadow_write_entries()

Posted by Roberto Bagnara 1 week ago

On 20/02/26 22:46, Andrew Cooper wrote:
> Eclair complains of a side effect in a sizeof() expression (R13.6).

I disagree with comments of the form "Eclair complains" used whereas
the right thing to say is, e.g.:

   sizeof() expressions with "potential side effects" violate
   MISRA C:2012 + AMD2 Rule 13.6, for which each function call
   is a potential side effect

or

   Take out potential side effects from sizeof() as mandated by
   MISRA C:2012 + AMD2 Rule 13.6.

Note that in recent versions of MISRA C that rule is no longer
mandatory.  More generally, note also that, IMHO, switching to
a more modern version of MISRA C would simplify compliance.

BTW: the correct spelling is "ECLAIR", all capitals.

> write_atomic() only evaluates each parameter once, but rewrite the expression
> to less resemble entries in an obfuscation contest.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: consulting@bugseng.com <consulting@bugseng.com>
> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>   xen/arch/x86/mm/shadow/set.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/shadow/set.c b/xen/arch/x86/mm/shadow/set.c
> index 8b670b6bb555..96ba2811077e 100644
> --- a/xen/arch/x86/mm/shadow/set.c
> +++ b/xen/arch/x86/mm/shadow/set.c
> @@ -62,8 +62,8 @@ shadow_write_entries(void *d, const void *s, unsigned int entries, mfn_t mfn)
>   
>       ASSERT(IS_ALIGNED((unsigned long)dst, sizeof(*dst)));
>   
> -    for ( ; i < entries; i++ )
> -        write_atomic(&dst++->l1, src++->l1);
> +    for ( ; i < entries; i++, dst++, src++ )
> +        write_atomic(&dst->l1, src->l1);
>   
>       unmap_domain_page(map);
>   }

Re: [PATCH 09/12] x86/shadow: Rework write_atomic() call in shadow_write_entries()

Posted by Andrew Cooper 5 days, 15 hours ago

On 23/02/2026 7:26 am, Roberto Bagnara wrote:
> On 20/02/26 22:46, Andrew Cooper wrote:
>> Eclair complains of a side effect in a sizeof() expression (R13.6).
>
> I disagree with comments of the form "Eclair complains"

We use the same phrasing with other tools too, but I can change it to
"reports" which I suppose is a more neutral term.

>
> Note that in recent versions of MISRA C that rule is no longer
> mandatory.  More generally, note also that, IMHO, switching to
> a more modern version of MISRA C would simplify compliance.

Ok.  Making things simpler for compliance sounds like a good thing. 
What would this entail?

Presumably we've got to adapt to all changes in this newer revision of
MISRA C.

~Andrew

Re: [PATCH 09/12] x86/shadow: Rework write_atomic() call in shadow_write_entries()

Posted by Nicola Vetrini 5 days, 14 hours ago

On 2026-02-25 13:14, Andrew Cooper wrote:
> On 23/02/2026 7:26 am, Roberto Bagnara wrote:
>> On 20/02/26 22:46, Andrew Cooper wrote:
>>> Eclair complains of a side effect in a sizeof() expression (R13.6).
>> 
>> I disagree with comments of the form "Eclair complains"
> 
> We use the same phrasing with other tools too, but I can change it to
> "reports" which I suppose is a more neutral term.
> 
>> 
>> Note that in recent versions of MISRA C that rule is no longer
>> mandatory.  More generally, note also that, IMHO, switching to
>> a more modern version of MISRA C would simplify compliance.
> 
> Ok.  Making things simpler for compliance sounds like a good thing. 
> What would this entail?
> 
> Presumably we've got to adapt to all changes in this newer revision of
> MISRA C.
> 
> ~Andrew

Most likely new violations on new non-clean guidelines, generally rules 
for features that were standardized in C11/C18 and were previously 
widely available extensions (e.g. _Noreturn, _Alignof, threads, ...), 
alongside some minor changes in existing ones, such as the 
classification change mentioned by Roberto. The exact impact depends on 
the target MISRA revision, however. Making an experiment should be only 
a matter of s/MC3A2/MC4/ (or whichever MISRA revision is chosen: MC4 in 
ECLAIR refers to the latest published MISRA revision, that is, MISRA 
C:2025. Perhaps also a few regressions (as in newly introduced 
violations) on clean ones, but I do not expect the results to be 
radically different.

Side note here: are the efforts to make Xen compile with -stc=c11/gnu11 
still ongoing? I say this because any MISRA revision other than the one 
currently used by Xen by default is based on C11, as it introduces 
guidelines for C11/C18 features. Not that this would matter a whole lot 
for Xen, but it is something to consider in the broader picture.

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

Re: [PATCH 09/12] x86/shadow: Rework write_atomic() call in shadow_write_entries()

Posted by Andrew Cooper 5 days, 14 hours ago

On 25/02/2026 12:35 pm, Nicola Vetrini wrote:
> On 2026-02-25 13:14, Andrew Cooper wrote:
>> On 23/02/2026 7:26 am, Roberto Bagnara wrote: 
>>>
>>> Note that in recent versions of MISRA C that rule is no longer
>>> mandatory.  More generally, note also that, IMHO, switching to
>>> a more modern version of MISRA C would simplify compliance.
>>
>> Ok.  Making things simpler for compliance sounds like a good thing. 
>> What would this entail?
>>
>> Presumably we've got to adapt to all changes in this newer revision of
>> MISRA C.
>>
>> ~Andrew
>
> Most likely new violations on new non-clean guidelines, generally
> rules for features that were standardized in C11/C18 and were
> previously widely available extensions (e.g. _Noreturn, _Alignof,
> threads, ...), 

We use noreturn a lot, and alignof() a little.  No threading at all
(well - not as C understands it).

> alongside some minor changes in existing ones, such as the
> classification change mentioned by Roberto. The exact impact depends
> on the target MISRA revision, however. Making an experiment should be
> only a matter of s/MC3A2/MC4/ (or whichever MISRA revision is chosen:
> MC4 in ECLAIR refers to the latest published MISRA revision, that is,
> MISRA C:2025. Perhaps also a few regressions (as in newly introduced
> violations) on clean ones, but I do not expect the results to be
> radically different.
>
> Side note here: are the efforts to make Xen compile with
> -stc=c11/gnu11 still ongoing? I say this because any MISRA revision
> other than the one currently used by Xen by default is based on C11,
> as it introduces guidelines for C11/C18 features. Not that this would
> matter a whole lot for Xen, but it is something to consider in the
> broader picture.
>

Funny you should ask.  I had a paragraph about it in my reply but
dropped it, thinking it was getting off track.

https://gitlab.com/xen-project/xen/-/issues/201

I've just updated it to note that we did start using auto, by way of the
__auto_type language extension.

From the Xen side, switching to gnu11 is a one-line change.  However
"ongoing" is really just me in my copious free time, and I'm not able to
do the ECLAIR/MISRA config side of the work.

It sounds to me like we want a dedicated work item switch to gnu11 and
some newer MISRA revision, but I will have to defer to you on how large
a task this is.

I suppose we should start with an experiment to see what shows up in the
*-amd target builds, and go from there?

~Andrew

Re: [PATCH 09/12] x86/shadow: Rework write_atomic() call in shadow_write_entries()

Posted by Nicola Vetrini 5 days, 14 hours ago

On 2026-02-25 13:53, Andrew Cooper wrote:
> On 25/02/2026 12:35 pm, Nicola Vetrini wrote:
>> On 2026-02-25 13:14, Andrew Cooper wrote:
>>> On 23/02/2026 7:26 am, Roberto Bagnara wrote: 
>>>> 
>>>> Note that in recent versions of MISRA C that rule is no longer
>>>> mandatory.  More generally, note also that, IMHO, switching to
>>>> a more modern version of MISRA C would simplify compliance.
>>> 
>>> Ok.  Making things simpler for compliance sounds like a good thing. 
>>> What would this entail?
>>> 
>>> Presumably we've got to adapt to all changes in this newer revision 
>>> of
>>> MISRA C.
>>> 
>>> ~Andrew
>> 
>> Most likely new violations on new non-clean guidelines, generally
>> rules for features that were standardized in C11/C18 and were
>> previously widely available extensions (e.g. _Noreturn, _Alignof,
>> threads, ...), 
> 
> We use noreturn a lot, and alignof() a little.  No threading at all
> (well - not as C understands it).
> 
>> alongside some minor changes in existing ones, such as the
>> classification change mentioned by Roberto. The exact impact depends
>> on the target MISRA revision, however. Making an experiment should be
>> only a matter of s/MC3A2/MC4/ (or whichever MISRA revision is chosen:
>> MC4 in ECLAIR refers to the latest published MISRA revision, that is,
>> MISRA C:2025. Perhaps also a few regressions (as in newly introduced
>> violations) on clean ones, but I do not expect the results to be
>> radically different.
>> 
>> Side note here: are the efforts to make Xen compile with
>> -stc=c11/gnu11 still ongoing? I say this because any MISRA revision
>> other than the one currently used by Xen by default is based on C11,
>> as it introduces guidelines for C11/C18 features. Not that this would
>> matter a whole lot for Xen, but it is something to consider in the
>> broader picture.
>> 
> 
> Funny you should ask.  I had a paragraph about it in my reply but
> dropped it, thinking it was getting off track.
> 
> https://gitlab.com/xen-project/xen/-/issues/201
> 
> I've just updated it to note that we did start using auto, by way of 
> the
> __auto_type language extension.
> 
> From the Xen side, switching to gnu11 is a one-line change.  However
> "ongoing" is really just me in my copious free time, and I'm not able 
> to
> do the ECLAIR/MISRA config side of the work.
> 
> It sounds to me like we want a dedicated work item switch to gnu11 and
> some newer MISRA revision, but I will have to defer to you on how large
> a task this is.
> 
> I suppose we should start with an experiment to see what shows up in 
> the
> *-amd target builds, and go from there?
> 

Yes, that's sensible. I just wasn't sure if there were other gotchas in 
Xen still to be addressed before using gnu11

> ~Andrew

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

Re: [PATCH 09/12] x86/shadow: Rework write_atomic() call in shadow_write_entries()

Posted by Nicola Vetrini 1 week, 3 days ago

On 2026-02-20 22:46, Andrew Cooper wrote:
> Eclair complains of a side effect in a sizeof() expression (R13.6).
> 
> write_atomic() only evaluates each parameter once, but rewrite the 
> expression
> to less resemble entries in an obfuscation contest.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: consulting@bugseng.com <consulting@bugseng.com>
> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/arch/x86/mm/shadow/set.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/shadow/set.c 
> b/xen/arch/x86/mm/shadow/set.c
> index 8b670b6bb555..96ba2811077e 100644
> --- a/xen/arch/x86/mm/shadow/set.c
> +++ b/xen/arch/x86/mm/shadow/set.c
> @@ -62,8 +62,8 @@ shadow_write_entries(void *d, const void *s, unsigned 
> int entries, mfn_t mfn)
> 
>      ASSERT(IS_ALIGNED((unsigned long)dst, sizeof(*dst)));
> 
> -    for ( ; i < entries; i++ )
> -        write_atomic(&dst++->l1, src++->l1);
> +    for ( ; i < entries; i++, dst++, src++ )
> +        write_atomic(&dst->l1, src->l1);
> 
>      unmap_domain_page(map);
>  }

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