[PATCH 0/3] xen: Trivial MISRA fixes

[PATCH 0/3] xen: Trivial MISRA fixes

[Andrew Cooper]

Trivial fixes for MISRA issues while idly looking through the x86 report.

Andrew Cooper (3):
  x86/p2m.h: Add include guards
  x86/shadow: Don't use signed bitfield in sh_emulate_ctxt
  common/spinlock: Drop inline from _spin_lock_cb()

 xen/arch/x86/mm/p2m.h            | 5 +++++
 xen/arch/x86/mm/shadow/private.h | 2 +-
 xen/common/spinlock.c            | 2 +-
 3 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.11.0

[PATCH 1/3] x86/p2m.h: Add include guards

[Andrew Cooper]

Spotted by Eclair MISRA scanner.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
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>
---
 xen/arch/x86/mm/p2m.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/x86/mm/p2m.h b/xen/arch/x86/mm/p2m.h
index cc0f6766e4df..dc706b8e4799 100644
--- a/xen/arch/x86/mm/p2m.h
+++ b/xen/arch/x86/mm/p2m.h
@@ -15,6 +15,9 @@
  * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#ifndef __ARCH_MM_P2M_H__
+#define __ARCH_MM_P2M_H__
+
 struct p2m_domain *p2m_init_one(struct domain *d);
 void p2m_free_one(struct p2m_domain *p2m);
 
@@ -39,6 +42,8 @@ int ept_p2m_init(struct p2m_domain *p2m);
 void ept_p2m_uninit(struct p2m_domain *p2m);
 void p2m_init_altp2m_ept(struct domain *d, unsigned int i);
 
+#endif /* __ARCH_MM_P2M_H__ */
+
 /*
  * Local variables:
  * mode: C
-- 
2.11.0

[PATCH 2/3] x86/shadow: Don't use signed bitfield in sh_emulate_ctxt

[Andrew Cooper]

'int' bitfields in particular have implementation defined behaviour under gcc
and can change signed-ness with -funsigned-bitfields.

There is no need for low_bit_was_clear to be a bitfield in the first place; it
is only used as a boolean.  Doing so even improves the code generation in
sh_emulate_map_dest() to avoid emitting a merge with structure padding.

Spotted by Eclair MISRA scanner.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
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>
---
 xen/arch/x86/mm/shadow/private.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/shadow/private.h b/xen/arch/x86/mm/shadow/private.h
index 3dc024e30f20..772521b55dd3 100644
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -827,7 +827,7 @@ struct sh_emulate_ctxt {
 #if (SHADOW_OPTIMIZATIONS & SHOPT_SKIP_VERIFY)
     /* Special case for avoiding having to verify writes: remember
      * whether the old value had its low bit (_PAGE_PRESENT) clear. */
-    int low_bit_was_clear:1;
+    bool low_bit_was_clear;
 #endif
 };
 
-- 
2.11.0

[PATCH 3/3] common/spinlock: Drop inline from _spin_lock_cb()

[Andrew Cooper]

This is undefined behaviour, because there is no _spin_lock_cb() in a separate
translation unit (C11 6.7.4.11).

Moreover, MISRA prohibits this construct because, in the case where it is well
defined, the compiler is free to use either implementation and nothing
prevents the two from being different.

This function has external users, so drop the inline.

Spotted by Eclair MISRA scanner.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
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>
---
 xen/common/spinlock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 62c83aaa6a73..8cb3b316c5b1 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -159,7 +159,7 @@ static always_inline u16 observe_head(spinlock_tickets_t *t)
     return read_atomic(&t->head);
 }
 
-void inline _spin_lock_cb(spinlock_t *lock, void (*cb)(void *), void *data)
+void _spin_lock_cb(spinlock_t *lock, void (*cb)(void *), void *data)
 {
     spinlock_tickets_t tickets = SPINLOCK_TICKET_INC;
     LOCK_PROFILE_VAR;
-- 
2.11.0

Re: [PATCH 1/3] x86/p2m.h: Add include guards

[Bertrand Marquis]

Hi Andrew,

> On 9 May 2022, at 13:24, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> Spotted by Eclair MISRA scanner.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

Re: [PATCH 3/3] common/spinlock: Drop inline from _spin_lock_cb()

[Bertrand Marquis]

Hi Andrew,

> On 9 May 2022, at 13:24, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> This is undefined behaviour, because there is no _spin_lock_cb() in a separate
> translation unit (C11 6.7.4.11).
> 
> Moreover, MISRA prohibits this construct because, in the case where it is well
> defined, the compiler is free to use either implementation and nothing
> prevents the two from being different.
> 
> This function has external users, so drop the inline.
> 
> Spotted by Eclair MISRA scanner.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

Re: [PATCH 2/3] x86/shadow: Don't use signed bitfield in sh_emulate_ctxt

[Bertrand Marquis]

Hi Andrew,

> On 9 May 2022, at 13:24, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> 'int' bitfields in particular have implementation defined behaviour under gcc
> and can change signed-ness with -funsigned-bitfields.
> 
> There is no need for low_bit_was_clear to be a bitfield in the first place; it
> is only used as a boolean.  Doing so even improves the code generation in
> sh_emulate_map_dest() to avoid emitting a merge with structure padding.
> 
> Spotted by Eclair MISRA scanner.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Is in fact only used as boolean in hvm.c so does make sense.

Cheers
Bertrand

Re: [PATCH 1/3] x86/p2m.h: Add include guards

[Roger Pau Monné]

On Mon, May 09, 2022 at 01:24:07PM +0100, Andrew Cooper wrote:
> Spotted by Eclair MISRA scanner.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> 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>
> ---
>  xen/arch/x86/mm/p2m.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/x86/mm/p2m.h b/xen/arch/x86/mm/p2m.h
> index cc0f6766e4df..dc706b8e4799 100644
> --- a/xen/arch/x86/mm/p2m.h
> +++ b/xen/arch/x86/mm/p2m.h
> @@ -15,6 +15,9 @@
>   * along with this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#ifndef __ARCH_MM_P2M_H__
> +#define __ARCH_MM_P2M_H__

Do we have any guidelines regarding guard naming? Some files seem to
use __ASM_X86_, others just __ASM and some just _X86.

Thanks, Roger.

Re: [PATCH 2/3] x86/shadow: Don't use signed bitfield in sh_emulate_ctxt

[Roger Pau Monné]

On Mon, May 09, 2022 at 01:24:08PM +0100, Andrew Cooper wrote:
> 'int' bitfields in particular have implementation defined behaviour under gcc
> and can change signed-ness with -funsigned-bitfields.
> 
> There is no need for low_bit_was_clear to be a bitfield in the first place; it
> is only used as a boolean.  Doing so even improves the code generation in
> sh_emulate_map_dest() to avoid emitting a merge with structure padding.
> 
> Spotted by Eclair MISRA scanner.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

Re: [PATCH 1/3] x86/p2m.h: Add include guards

[Andrew Cooper]

On 09/05/2022 14:18, Roger Pau Monné wrote:
> On Mon, May 09, 2022 at 01:24:07PM +0100, Andrew Cooper wrote:
>> Spotted by Eclair MISRA scanner.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> 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>
>> ---
>>  xen/arch/x86/mm/p2m.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/xen/arch/x86/mm/p2m.h b/xen/arch/x86/mm/p2m.h
>> index cc0f6766e4df..dc706b8e4799 100644
>> --- a/xen/arch/x86/mm/p2m.h
>> +++ b/xen/arch/x86/mm/p2m.h
>> @@ -15,6 +15,9 @@
>>   * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>   */
>>  
>> +#ifndef __ARCH_MM_P2M_H__
>> +#define __ARCH_MM_P2M_H__
> Do we have any guidelines regarding guard naming? Some files seem to
> use __ASM_X86_, others just __ASM and some just _X86.

Not really.  This one is especially complicated because x86 has two of them.

$ git ls-files | grep /p2m\.h
arch/arm/include/asm/p2m.h
arch/x86/include/asm/p2m.h
arch/x86/mm/p2m.h

~Andrew

Re: [PATCH 3/3] common/spinlock: Drop inline from _spin_lock_cb()

[Roger Pau Monné]

On Mon, May 09, 2022 at 01:24:09PM +0100, Andrew Cooper wrote:
> This is undefined behaviour, because there is no _spin_lock_cb() in a separate
> translation unit (C11 6.7.4.11).
> 
> Moreover, MISRA prohibits this construct because, in the case where it is well
> defined, the compiler is free to use either implementation and nothing
> prevents the two from being different.

From my reading of the spec, using inline defined function with an
extern declaration could allow the function to be (re)defined in the
scope of a different compilation unit, kind of similar to the usage of
the weak attribute?

> This function has external users, so drop the inline.
> 
> Spotted by Eclair MISRA scanner.

Like wants a:

Fixes: 462090402a ('spinlock: Introduce spin_lock_cb()')

Thanks, Roger.

Re: [PATCH 1/3] x86/p2m.h: Add include guards

[Jan Beulich]

On 09.05.2022 14:24, Andrew Cooper wrote:
> Spotted by Eclair MISRA scanner.

I'm sorry, but what exactly was it that the scanner spotted? It was
actually deliberate to introduce this file without guards. I'm of
the general opinion that (private) headers not to be included by
other headers (but only by .c files) are not in need of guards. If
it is project-wide consensus that _all_ header files should have
guards, then I'll try to keep this in mind (in "x86emul: a few
small steps towards disintegration" for example I introduce
another such instance), but then it should also be put down in
./CODING_STYLE.

Jan

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> 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>
> ---
>  xen/arch/x86/mm/p2m.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/x86/mm/p2m.h b/xen/arch/x86/mm/p2m.h
> index cc0f6766e4df..dc706b8e4799 100644
> --- a/xen/arch/x86/mm/p2m.h
> +++ b/xen/arch/x86/mm/p2m.h
> @@ -15,6 +15,9 @@
>   * along with this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#ifndef __ARCH_MM_P2M_H__
> +#define __ARCH_MM_P2M_H__
> +
>  struct p2m_domain *p2m_init_one(struct domain *d);
>  void p2m_free_one(struct p2m_domain *p2m);
>  
> @@ -39,6 +42,8 @@ int ept_p2m_init(struct p2m_domain *p2m);
>  void ept_p2m_uninit(struct p2m_domain *p2m);
>  void p2m_init_altp2m_ept(struct domain *d, unsigned int i);
>  
> +#endif /* __ARCH_MM_P2M_H__ */
> +
>  /*
>   * Local variables:
>   * mode: C

Re: [PATCH 3/3] common/spinlock: Drop inline from _spin_lock_cb()

[Jan Beulich]

On 09.05.2022 15:53, Roger Pau Monné wrote:
> On Mon, May 09, 2022 at 01:24:09PM +0100, Andrew Cooper wrote:
>> This is undefined behaviour, because there is no _spin_lock_cb() in a separate
>> translation unit (C11 6.7.4.11).
>>
>> Moreover, MISRA prohibits this construct because, in the case where it is well
>> defined, the compiler is free to use either implementation and nothing
>> prevents the two from being different.
> 
> From my reading of the spec, using inline defined function with an
> extern declaration could allow the function to be (re)defined in the
> scope of a different compilation unit, kind of similar to the usage of
> the weak attribute?

Which would then result in a linking error due to duplicate symbols,
wouldn't it?

Jan

Re: [PATCH 1/3] x86/p2m.h: Add include guards

[Roberto Bagnara]

On 17/05/22 17:38, Jan Beulich wrote:
> On 09.05.2022 14:24, Andrew Cooper wrote:
>> Spotted by Eclair MISRA scanner.
> 
> I'm sorry, but what exactly was it that the scanner spotted? It was
> actually deliberate to introduce this file without guards. I'm of
> the general opinion that (private) headers not to be included by
> other headers (but only by .c files) are not in need of guards. If
> it is project-wide consensus that _all_ header files should have
> guards, then I'll try to keep this in mind (in "x86emul: a few
> small steps towards disintegration" for example I introduce
> another such instance), but then it should also be put down in
> ./CODING_STYLE.

The rationale of this rule is as follows:

- With a complex hierarchy of nested header files, it is possible
   for a header file to be included more than once.

- This can bring to circular references of header files, which
   can result in undefined behavior and/or be difficult to debug.

- If multiple inclusion leads to multiple or conflicting definitions,
   then this can result in undefined or erroneous behavior.

- Compilation and analysis time is needlessly increased.

There has been a period (which lasted until the end of the '70s
or the beginning of the '80s, I would have to dig up to be
more precise) when the solution was thought to be "headers
shall not to be included by other headers but only by .c files."
Experience then showed that, in medium to large projects,
each .c file had to begin with a long list of #include
directives;  such lists needed to be ordered to accommodate
the dependencies between header files;  in some cases the
lists were so long that:

a) it was a kind of black magic to find out the right
    inclusion order, one that would work in any of
    possibly many project configurations;
b) the lists of #include directives often contained duplicates,
    possibly because the desperate programmers where trying
    to find the right order.

In the end, the software engineering community converged
on the idea that guards against multiple inclusion are
a much better alternative.

Of course there are valid reasons to deviate the rule:
some header files might be conceived to be included
multiple times.  A one-line configuration for ECLAIR
will do the trick to make sure such header files are
not reported.

Kind regards,

    Roberto

>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> 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>
>> ---
>>   xen/arch/x86/mm/p2m.h | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/xen/arch/x86/mm/p2m.h b/xen/arch/x86/mm/p2m.h
>> index cc0f6766e4df..dc706b8e4799 100644
>> --- a/xen/arch/x86/mm/p2m.h
>> +++ b/xen/arch/x86/mm/p2m.h
>> @@ -15,6 +15,9 @@
>>    * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>    */
>>   
>> +#ifndef __ARCH_MM_P2M_H__
>> +#define __ARCH_MM_P2M_H__
>> +
>>   struct p2m_domain *p2m_init_one(struct domain *d);
>>   void p2m_free_one(struct p2m_domain *p2m);
>>   
>> @@ -39,6 +42,8 @@ int ept_p2m_init(struct p2m_domain *p2m);
>>   void ept_p2m_uninit(struct p2m_domain *p2m);
>>   void p2m_init_altp2m_ept(struct domain *d, unsigned int i);
>>   
>> +#endif /* __ARCH_MM_P2M_H__ */
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
> 
> 

Re: [PATCH 1/3] x86/p2m.h: Add include guards

[Jan Beulich]

On 17.05.2022 20:42, Roberto Bagnara wrote:
> On 17/05/22 17:38, Jan Beulich wrote:
>> On 09.05.2022 14:24, Andrew Cooper wrote:
>>> Spotted by Eclair MISRA scanner.
>>
>> I'm sorry, but what exactly was it that the scanner spotted? It was
>> actually deliberate to introduce this file without guards. I'm of
>> the general opinion that (private) headers not to be included by
>> other headers (but only by .c files) are not in need of guards. If
>> it is project-wide consensus that _all_ header files should have
>> guards, then I'll try to keep this in mind (in "x86emul: a few
>> small steps towards disintegration" for example I introduce
>> another such instance), but then it should also be put down in
>> ./CODING_STYLE.
> 
> The rationale of this rule is as follows:
> 
> - With a complex hierarchy of nested header files, it is possible
>    for a header file to be included more than once.
> 
> - This can bring to circular references of header files, which
>    can result in undefined behavior and/or be difficult to debug.

This, in particular, is known to happen in our and other source
bases despite the use of guards, hence I view this point as at
best partly related. Nevertheless I agree with and understand the
reasons for using guards _where they are needed_. I do not agree
that guards need to be there for no specific reason.

Jan

> - If multiple inclusion leads to multiple or conflicting definitions,
>    then this can result in undefined or erroneous behavior.
> 
> - Compilation and analysis time is needlessly increased.
> 
> There has been a period (which lasted until the end of the '70s
> or the beginning of the '80s, I would have to dig up to be
> more precise) when the solution was thought to be "headers
> shall not to be included by other headers but only by .c files."
> Experience then showed that, in medium to large projects,
> each .c file had to begin with a long list of #include
> directives;  such lists needed to be ordered to accommodate
> the dependencies between header files;  in some cases the
> lists were so long that:
> 
> a) it was a kind of black magic to find out the right
>     inclusion order, one that would work in any of
>     possibly many project configurations;
> b) the lists of #include directives often contained duplicates,
>     possibly because the desperate programmers where trying
>     to find the right order.
> 
> In the end, the software engineering community converged
> on the idea that guards against multiple inclusion are
> a much better alternative.
> 
> Of course there are valid reasons to deviate the rule:
> some header files might be conceived to be included
> multiple times.  A one-line configuration for ECLAIR
> will do the trick to make sure such header files are
> not reported.
> 
> Kind regards,
> 
>     Roberto
> 
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Wei Liu <wl@xen.org>
>>> 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>
>>> ---
>>>   xen/arch/x86/mm/p2m.h | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/mm/p2m.h b/xen/arch/x86/mm/p2m.h
>>> index cc0f6766e4df..dc706b8e4799 100644
>>> --- a/xen/arch/x86/mm/p2m.h
>>> +++ b/xen/arch/x86/mm/p2m.h
>>> @@ -15,6 +15,9 @@
>>>    * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>>    */
>>>   
>>> +#ifndef __ARCH_MM_P2M_H__
>>> +#define __ARCH_MM_P2M_H__
>>> +
>>>   struct p2m_domain *p2m_init_one(struct domain *d);
>>>   void p2m_free_one(struct p2m_domain *p2m);
>>>   
>>> @@ -39,6 +42,8 @@ int ept_p2m_init(struct p2m_domain *p2m);
>>>   void ept_p2m_uninit(struct p2m_domain *p2m);
>>>   void p2m_init_altp2m_ept(struct domain *d, unsigned int i);
>>>   
>>> +#endif /* __ARCH_MM_P2M_H__ */
>>> +
>>>   /*
>>>    * Local variables:
>>>    * mode: C
>>
>>
>