[XEN PATCH 0/5] address violations of MISRA C rules

[XEN PATCH 0/5] address violations of MISRA C rules

[Nicola Vetrini]

Patches 1 to 4 address violations of MISRA C Rule 20.12 by deviating certain
uses of some macros, while the last patch addresses some regressions introduced
by the latest bitops series

Nicola Vetrini (5):
  xen/domain: deviate violation of MISRA C Rule 20.12
  x86/domain: deviate violation of MISRA C Rule 20.12
  x86: deviate violation of MISRA C Rule 20.12
  automation/eclair_analysis: address remaining violations of MISRA C
    Rule 20.12
  xen: fix MISRA regressions on rule 20.9 and 20.12

 automation/eclair_analysis/ECLAIR/deviations.ecl | 8 +++++++-
 docs/misra/safe.json                             | 8 ++++++++
 xen/arch/x86/domain.c                            | 1 +
 xen/arch/x86/domctl.c                            | 1 +
 xen/arch/x86/include/asm/shared.h                | 1 +
 xen/common/domain.c                              | 1 +
 xen/include/xen/self-tests.h                     | 2 +-
 7 files changed, 20 insertions(+), 2 deletions(-)

-- 
2.34.1

[XEN PATCH 1/5] xen/domain: deviate violation of MISRA C Rule 20.12

[Nicola Vetrini]

MISRA C Rule 20.12 states: "A macro parameter used as an operand to
the # or ## operators, which is itself subject to further macro replacement,
shall only be used as an operand to these operators".

In this case, builds where CONFIG_DEBUG_LOCK_PROFILE=y the domain_lock
macro is used both as a regular macro argument and as an operand for
stringification in the expansion of macro spin_lock_init_prof.
A SAF-x-safe deviation is introduced to justify this.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 docs/misra/safe.json | 8 ++++++++
 xen/common/domain.c  | 1 +
 2 files changed, 9 insertions(+)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 9b13bcf71706..c213e0a0be3b 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -52,6 +52,14 @@
         },
         {
             "id": "SAF-6-safe",
+            "analyser": {
+                "eclair": "MC3R1.R20.12"
+            },
+            "name": "MC3R1.R20.12: use of a macro argument that deliberately violates the Rule",
+            "text": "A macro parameter that is itself a macro is intentionally used within the macro both as a regular parameter and for text replacement."
+        },
+        {
+            "id": "SAF-7-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 67cadb7c3f4f..2c7168093734 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -632,6 +632,7 @@ struct domain *domain_create(domid_t domid,
 
     atomic_set(&d->refcnt, 1);
     RCU_READ_LOCK_INIT(&d->rcu_lock);
+    /* SAF-6-safe Rule 20.12 expansion of macro domain_lock in debug builds */
     rspin_lock_init_prof(d, domain_lock);
     rspin_lock_init_prof(d, page_alloc_lock);
     spin_lock_init(&d->hypercall_deadlock_mutex);
-- 
2.34.1

Re: [XEN PATCH 1/5] xen/domain: deviate violation of MISRA C Rule 20.12

[Jan Beulich]

On 01.06.2024 12:16, Nicola Vetrini wrote:
> MISRA C Rule 20.12 states: "A macro parameter used as an operand to
> the # or ## operators, which is itself subject to further macro replacement,
> shall only be used as an operand to these operators".
> 
> In this case, builds where CONFIG_DEBUG_LOCK_PROFILE=y the domain_lock
> macro is used both as a regular macro argument and as an operand for
> stringification in the expansion of macro spin_lock_init_prof.

The shouldn't the marker be on the definition of spin_lock_init_prof(),
rather than ...

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -632,6 +632,7 @@ struct domain *domain_create(domid_t domid,
>  
>      atomic_set(&d->refcnt, 1);
>      RCU_READ_LOCK_INIT(&d->rcu_lock);
> +    /* SAF-6-safe Rule 20.12 expansion of macro domain_lock in debug builds */
>      rspin_lock_init_prof(d, domain_lock);
>      rspin_lock_init_prof(d, page_alloc_lock);
>      spin_lock_init(&d->hypercall_deadlock_mutex);

... actually just one of the two uses here (and presumably several more
elsewhere)?

Jan

Re: [XEN PATCH 1/5] xen/domain: deviate violation of MISRA C Rule 20.12

[Nicola Vetrini]

On 2024-06-03 08:39, Jan Beulich wrote:
> On 01.06.2024 12:16, Nicola Vetrini wrote:
>> MISRA C Rule 20.12 states: "A macro parameter used as an operand to
>> the # or ## operators, which is itself subject to further macro 
>> replacement,
>> shall only be used as an operand to these operators".
>> 
>> In this case, builds where CONFIG_DEBUG_LOCK_PROFILE=y the domain_lock
>> macro is used both as a regular macro argument and as an operand for
>> stringification in the expansion of macro spin_lock_init_prof.
> 
> The shouldn't the marker be on the definition of spin_lock_init_prof(),
> rather than ...
> 
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -632,6 +632,7 @@ struct domain *domain_create(domid_t domid,
>> 
>>      atomic_set(&d->refcnt, 1);
>>      RCU_READ_LOCK_INIT(&d->rcu_lock);
>> +    /* SAF-6-safe Rule 20.12 expansion of macro domain_lock in debug 
>> builds */
>>      rspin_lock_init_prof(d, domain_lock);
>>      rspin_lock_init_prof(d, page_alloc_lock);
>>      spin_lock_init(&d->hypercall_deadlock_mutex);
> 
> ... actually just one of the two uses here (and presumably several more
> elsewhere)?
> 
> Jan

Actually it seems that this violation went away with some refactorings, 
so this patch is no longer needed other than for the addition to 
safe.json, so it can be folded into the next one.
I'll make the adjustment.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

[XEN PATCH 2/5] x86/domain: deviate violation of MISRA C Rule 20.12

[Nicola Vetrini]

MISRA C Rule 20.12 states: "A macro parameter used as an operand to
the # or ## operators, which is itself subject to further macro replacement,
shall only be used as an operand to these operators".

In this case, builds where CONFIG_COMPAT=y the fpu_ctxt
macro is used both as a regular macro argument and as an operand for
stringification in the expansion of CHECK_FIELD_.
This is deviated using a SAF-x-safe comment.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/domain.c | 1 +
 xen/arch/x86/domctl.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 536542841ef5..ccadfe0c9e70 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1084,6 +1084,7 @@ void arch_domain_creation_finished(struct domain *d)
 #ifdef CONFIG_COMPAT
 #define xen_vcpu_guest_context vcpu_guest_context
 #define fpu_ctxt fpu_ctxt.x
+/* SAF-6-safe Rule 20.12 expansion of macro fpu_ctxt with CONFIG_COMPAT */
 CHECK_FIELD_(struct, vcpu_guest_context, fpu_ctxt);
 #undef fpu_ctxt
 #undef xen_vcpu_guest_context
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9a72d57333e9..335aedf46d03 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1326,6 +1326,7 @@ long arch_do_domctl(
 #ifdef CONFIG_COMPAT
 #define xen_vcpu_guest_context vcpu_guest_context
 #define fpu_ctxt fpu_ctxt.x
+/* SAF-6-safe Rule 20.12 expansion of macro fpu_ctxt with CONFIG_COMPAT */
 CHECK_FIELD_(struct, vcpu_guest_context, fpu_ctxt);
 #undef fpu_ctxt
 #undef xen_vcpu_guest_context
-- 
2.34.1

Re: [XEN PATCH 2/5] x86/domain: deviate violation of MISRA C Rule 20.12

[Jan Beulich]

On 01.06.2024 12:16, Nicola Vetrini wrote:
> MISRA C Rule 20.12 states: "A macro parameter used as an operand to
> the # or ## operators, which is itself subject to further macro replacement,
> shall only be used as an operand to these operators".
> 
> In this case, builds where CONFIG_COMPAT=y the fpu_ctxt
> macro is used both as a regular macro argument and as an operand for
> stringification in the expansion of CHECK_FIELD_.
> This is deviated using a SAF-x-safe comment.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

At least for the time being; plans that Andrew vaguely described my
render this unnecessary down the road.

Jan

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1084,6 +1084,7 @@ void arch_domain_creation_finished(struct domain *d)
>  #ifdef CONFIG_COMPAT
>  #define xen_vcpu_guest_context vcpu_guest_context
>  #define fpu_ctxt fpu_ctxt.x
> +/* SAF-6-safe Rule 20.12 expansion of macro fpu_ctxt with CONFIG_COMPAT */
>  CHECK_FIELD_(struct, vcpu_guest_context, fpu_ctxt);
>  #undef fpu_ctxt
>  #undef xen_vcpu_guest_context
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 9a72d57333e9..335aedf46d03 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1326,6 +1326,7 @@ long arch_do_domctl(
>  #ifdef CONFIG_COMPAT
>  #define xen_vcpu_guest_context vcpu_guest_context
>  #define fpu_ctxt fpu_ctxt.x
> +/* SAF-6-safe Rule 20.12 expansion of macro fpu_ctxt with CONFIG_COMPAT */
>  CHECK_FIELD_(struct, vcpu_guest_context, fpu_ctxt);
>  #undef fpu_ctxt
>  #undef xen_vcpu_guest_context

[XEN PATCH 3/5] x86: deviate violation of MISRA C Rule 20.12

[Nicola Vetrini]

MISRA C Rule 20.12 states: "A macro parameter used as an operand to
the # or ## operators, which is itself subject to further macro replacement,
shall only be used as an operand to these operators".

When the second parameter of GET_SET_SHARED is a macro and is used as both
a regular parameter and for token pasting the rule deliberately violated.
A SAF-x-safe comment is used to deviate the usage.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/include/asm/shared.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/include/asm/shared.h b/xen/arch/x86/include/asm/shared.h
index 60b67fa4b427..c26d4b2b3f0f 100644
--- a/xen/arch/x86/include/asm/shared.h
+++ b/xen/arch/x86/include/asm/shared.h
@@ -76,6 +76,7 @@ static inline void arch_set_##field(struct vcpu *v,         \
 
 GET_SET_SHARED(unsigned long, max_pfn)
 GET_SET_SHARED(xen_pfn_t, pfn_to_mfn_frame_list_list)
+/* SAF-6-safe Rule 20.12: expansion of macro nmi_reason */
 GET_SET_SHARED(unsigned long, nmi_reason)
 
 GET_SET_VCPU(unsigned long, cr2)
-- 
2.34.1

Re: [XEN PATCH 3/5] x86: deviate violation of MISRA C Rule 20.12

[Jan Beulich]

On 01.06.2024 12:16, Nicola Vetrini wrote:
> --- a/xen/arch/x86/include/asm/shared.h
> +++ b/xen/arch/x86/include/asm/shared.h
> @@ -76,6 +76,7 @@ static inline void arch_set_##field(struct vcpu *v,         \
>  
>  GET_SET_SHARED(unsigned long, max_pfn)
>  GET_SET_SHARED(xen_pfn_t, pfn_to_mfn_frame_list_list)
> +/* SAF-6-safe Rule 20.12: expansion of macro nmi_reason */
>  GET_SET_SHARED(unsigned long, nmi_reason)

Before we go this route, were alternatives at least considered? Plus
didn't we special-case function-like macros already, when used in
situations where only object-like macros would be expanded anyway?

As to alternatives: nmi_reason() is used in exactly one place.
Dropping the #define and expanding the one use instead would be an
option. I further wonder whether moving the #define-s past the
piece of code you actually modify would also be an option (i.e. the
tool then no longer complaining).

Jan

Re: [XEN PATCH 3/5] x86: deviate violation of MISRA C Rule 20.12

[Nicola Vetrini]

On 2024-06-04 08:08, Jan Beulich wrote:
> On 01.06.2024 12:16, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/include/asm/shared.h
>> +++ b/xen/arch/x86/include/asm/shared.h
>> @@ -76,6 +76,7 @@ static inline void arch_set_##field(struct vcpu *v,  
>>        \
>> 
>>  GET_SET_SHARED(unsigned long, max_pfn)
>>  GET_SET_SHARED(xen_pfn_t, pfn_to_mfn_frame_list_list)
>> +/* SAF-6-safe Rule 20.12: expansion of macro nmi_reason */
>>  GET_SET_SHARED(unsigned long, nmi_reason)
> 
> Before we go this route, were alternatives at least considered? Plus
> didn't we special-case function-like macros already, when used in
> situations where only object-like macros would be expanded anyway?
> 

It may be the case that this is already deviated, hence meaning that the 
patch can be dropped: I'll recheck.

In that case, thanks for pointing this out.

> As to alternatives: nmi_reason() is used in exactly one place.
> Dropping the #define and expanding the one use instead would be an
> option. I further wonder whether moving the #define-s past the
> piece of code you actually modify would also be an option (i.e. the
> tool then no longer complaining).
> 
> Jan

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

[XEN PATCH 4/5] automation/eclair_analysis: address remaining violations of MISRA C Rule 20.12

[Nicola Vetrini]

The DEFINE macro in asm-offsets.c (for all architectures) still generates
violations despite the file(s) being excluded from compliance, due to the
fact that in its expansion it sometimes refers entities in non-excluded files.
These corner cases are deviated by the configuration.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index cf62a874d928..f29db9e08248 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -483,6 +483,12 @@ leads to a violation of the Rule are deviated."
 -config=MC3R1.R20.12,macros+={deliberate, "name(GENERATE_CASE)&&loc(file(deliberate_generate_case))"}
 -doc_end
 
+-doc_begin="The macro DEFINE is defined and used in excluded files asm-offsets.c.
+This may still cause violations if entities outside these files are referred to
+in the expansion."
+-config=MC3R1.R20.12,macros+={deliberate, "name(DEFINE)&&loc(file(asm_offsets))"}
+-doc_end
+
 #
 # Series 21.
 #
-- 
2.34.1

Re: [XEN PATCH 4/5] automation/eclair_analysis: address remaining violations of MISRA C Rule 20.12

[Jan Beulich]

On 01.06.2024 12:16, Nicola Vetrini wrote:
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -483,6 +483,12 @@ leads to a violation of the Rule are deviated."
>  -config=MC3R1.R20.12,macros+={deliberate, "name(GENERATE_CASE)&&loc(file(deliberate_generate_case))"}
>  -doc_end
>  
> +-doc_begin="The macro DEFINE is defined and used in excluded files asm-offsets.c.
> +This may still cause violations if entities outside these files are referred to
> +in the expansion."
> +-config=MC3R1.R20.12,macros+={deliberate, "name(DEFINE)&&loc(file(asm_offsets))"}
> +-doc_end

Can you give an example of such a reference? Nothing _in_ asm-offsets.c
should be referenced, I'd think. Only stuff in asm-offsets.h as _generated
from_ asm-offsets.c will, of course, be.

Jan

Re: [XEN PATCH 4/5] automation/eclair_analysis: address remaining violations of MISRA C Rule 20.12

[Nicola Vetrini]

On 2024-06-03 07:58, Jan Beulich wrote:
> On 01.06.2024 12:16, Nicola Vetrini wrote:
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -483,6 +483,12 @@ leads to a violation of the Rule are deviated."
>>  -config=MC3R1.R20.12,macros+={deliberate, 
>> "name(GENERATE_CASE)&&loc(file(deliberate_generate_case))"}
>>  -doc_end
>> 
>> +-doc_begin="The macro DEFINE is defined and used in excluded files 
>> asm-offsets.c.
>> +This may still cause violations if entities outside these files are 
>> referred to
>> +in the expansion."
>> +-config=MC3R1.R20.12,macros+={deliberate, 
>> "name(DEFINE)&&loc(file(asm_offsets))"}
>> +-doc_end
> 
> Can you give an example of such a reference? Nothing _in_ asm-offsets.c
> should be referenced, I'd think. Only stuff in asm-offsets.h as 
> _generated
> from_ asm-offsets.c will, of course, be.
> 

Perhaps I could have expressed that more clearly. What I meant is that 
there are some arguments to DEFINE that are not part of asm-offsets.c, 
therefore they end up in the violation report, but are not actually 
relevant, because the macro DEFINE is actually what we want to exclude.

See for instance at the link below VCPU_TRAP_{NMI,MCE}, which are 
defined in asm/domain.h and used as arguments to DEFINE inside 
asm-offsets.c.

https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/staging/X86_64-BUGSENG/676/PROJECT.ecd;/by_service/MC3R1.R20.12.html

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 4/5] automation/eclair_analysis: address remaining violations of MISRA C Rule 20.12

[Jan Beulich]

On 03.06.2024 09:13, Nicola Vetrini wrote:
> On 2024-06-03 07:58, Jan Beulich wrote:
>> On 01.06.2024 12:16, Nicola Vetrini wrote:
>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> @@ -483,6 +483,12 @@ leads to a violation of the Rule are deviated."
>>>  -config=MC3R1.R20.12,macros+={deliberate, 
>>> "name(GENERATE_CASE)&&loc(file(deliberate_generate_case))"}
>>>  -doc_end
>>>
>>> +-doc_begin="The macro DEFINE is defined and used in excluded files 
>>> asm-offsets.c.
>>> +This may still cause violations if entities outside these files are 
>>> referred to
>>> +in the expansion."
>>> +-config=MC3R1.R20.12,macros+={deliberate, 
>>> "name(DEFINE)&&loc(file(asm_offsets))"}
>>> +-doc_end
>>
>> Can you give an example of such a reference? Nothing _in_ asm-offsets.c
>> should be referenced, I'd think. Only stuff in asm-offsets.h as 
>> _generated
>> from_ asm-offsets.c will, of course, be.
>>
> 
> Perhaps I could have expressed that more clearly. What I meant is that 
> there are some arguments to DEFINE that are not part of asm-offsets.c, 
> therefore they end up in the violation report, but are not actually 
> relevant, because the macro DEFINE is actually what we want to exclude.
> 
> See for instance at the link below VCPU_TRAP_{NMI,MCE}, which are 
> defined in asm/domain.h and used as arguments to DEFINE inside 
> asm-offsets.c.
> 
> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/staging/X86_64-BUGSENG/676/PROJECT.ecd;/by_service/MC3R1.R20.12.html

I'm afraid I still don't understand: The file being supposed to be
excluded from scanning, why does it even show up in that report?

Jan

Re: [XEN PATCH 4/5] automation/eclair_analysis: address remaining violations of MISRA C Rule 20.12

[Nicola Vetrini]

On 2024-06-03 20:52, Jan Beulich wrote:
> On 03.06.2024 09:13, Nicola Vetrini wrote:
>> On 2024-06-03 07:58, Jan Beulich wrote:
>>> On 01.06.2024 12:16, Nicola Vetrini wrote:
>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>> @@ -483,6 +483,12 @@ leads to a violation of the Rule are deviated."
>>>>  -config=MC3R1.R20.12,macros+={deliberate,
>>>> "name(GENERATE_CASE)&&loc(file(deliberate_generate_case))"}
>>>>  -doc_end
>>>> 
>>>> +-doc_begin="The macro DEFINE is defined and used in excluded files
>>>> asm-offsets.c.
>>>> +This may still cause violations if entities outside these files are
>>>> referred to
>>>> +in the expansion."
>>>> +-config=MC3R1.R20.12,macros+={deliberate,
>>>> "name(DEFINE)&&loc(file(asm_offsets))"}
>>>> +-doc_end
>>> 
>>> Can you give an example of such a reference? Nothing _in_ 
>>> asm-offsets.c
>>> should be referenced, I'd think. Only stuff in asm-offsets.h as
>>> _generated
>>> from_ asm-offsets.c will, of course, be.
>>> 
>> 
>> Perhaps I could have expressed that more clearly. What I meant is that
>> there are some arguments to DEFINE that are not part of asm-offsets.c,
>> therefore they end up in the violation report, but are not actually
>> relevant, because the macro DEFINE is actually what we want to 
>> exclude.
>> 
>> See for instance at the link below VCPU_TRAP_{NMI,MCE}, which are
>> defined in asm/domain.h and used as arguments to DEFINE inside
>> asm-offsets.c.
>> 
>> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/staging/X86_64-BUGSENG/676/PROJECT.ecd;/by_service/MC3R1.R20.12.html
> 
> I'm afraid I still don't understand: The file being supposed to be
> excluded from scanning, why does it even show up in that report?
> 
> Jan

The report is made up of several source code locations. Three of them 
are within asm-offsets.c, which is excluded from compliance but still 
analyzed, but one references a macro definition in another file (e.g., 
VCPU_TRAP_NMI from asm/domain.h). So in this case the exclusion of 
asm-offsets.c is not enough for the report not to be shown.

The configuration is telling the tool that the macro DEFINE from 
asm-offsets is meant to be deviated, which is what is needed in this 
case.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 4/5] automation/eclair_analysis: address remaining violations of MISRA C Rule 20.12

[Jan Beulich]

On 03.06.2024 21:12, Nicola Vetrini wrote:
> On 2024-06-03 20:52, Jan Beulich wrote:
>> On 03.06.2024 09:13, Nicola Vetrini wrote:
>>> On 2024-06-03 07:58, Jan Beulich wrote:
>>>> On 01.06.2024 12:16, Nicola Vetrini wrote:
>>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>> @@ -483,6 +483,12 @@ leads to a violation of the Rule are deviated."
>>>>>  -config=MC3R1.R20.12,macros+={deliberate,
>>>>> "name(GENERATE_CASE)&&loc(file(deliberate_generate_case))"}
>>>>>  -doc_end
>>>>>
>>>>> +-doc_begin="The macro DEFINE is defined and used in excluded files
>>>>> asm-offsets.c.
>>>>> +This may still cause violations if entities outside these files are
>>>>> referred to
>>>>> +in the expansion."
>>>>> +-config=MC3R1.R20.12,macros+={deliberate,
>>>>> "name(DEFINE)&&loc(file(asm_offsets))"}
>>>>> +-doc_end
>>>>
>>>> Can you give an example of such a reference? Nothing _in_ 
>>>> asm-offsets.c
>>>> should be referenced, I'd think. Only stuff in asm-offsets.h as
>>>> _generated
>>>> from_ asm-offsets.c will, of course, be.
>>>>
>>>
>>> Perhaps I could have expressed that more clearly. What I meant is that
>>> there are some arguments to DEFINE that are not part of asm-offsets.c,
>>> therefore they end up in the violation report, but are not actually
>>> relevant, because the macro DEFINE is actually what we want to 
>>> exclude.
>>>
>>> See for instance at the link below VCPU_TRAP_{NMI,MCE}, which are
>>> defined in asm/domain.h and used as arguments to DEFINE inside
>>> asm-offsets.c.
>>>
>>> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/staging/X86_64-BUGSENG/676/PROJECT.ecd;/by_service/MC3R1.R20.12.html
>>
>> I'm afraid I still don't understand: The file being supposed to be
>> excluded from scanning, why does it even show up in that report?
> 
> The report is made up of several source code locations. Three of them 
> are within asm-offsets.c, which is excluded from compliance but still 
> analyzed, but one references a macro definition in another file (e.g., 
> VCPU_TRAP_NMI from asm/domain.h). So in this case the exclusion of 
> asm-offsets.c is not enough for the report not to be shown.

But the (would-be-)violation is in asm-offsets.c. The other locations
pointed at are providing context. To report a violation, it should be
enough to exclude the file where the violation itself is?

Jan

Re: [XEN PATCH 4/5] automation/eclair_analysis: address remaining violations of MISRA C Rule 20.12

[Nicola Vetrini]

On 2024-06-03 23:24, Jan Beulich wrote:
> On 03.06.2024 21:12, Nicola Vetrini wrote:
>> On 2024-06-03 20:52, Jan Beulich wrote:
>>> On 03.06.2024 09:13, Nicola Vetrini wrote:
>>>> On 2024-06-03 07:58, Jan Beulich wrote:
>>>>> On 01.06.2024 12:16, Nicola Vetrini wrote:
>>>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>>> @@ -483,6 +483,12 @@ leads to a violation of the Rule are 
>>>>>> deviated."
>>>>>>  -config=MC3R1.R20.12,macros+={deliberate,
>>>>>> "name(GENERATE_CASE)&&loc(file(deliberate_generate_case))"}
>>>>>>  -doc_end
>>>>>> 
>>>>>> +-doc_begin="The macro DEFINE is defined and used in excluded 
>>>>>> files
>>>>>> asm-offsets.c.
>>>>>> +This may still cause violations if entities outside these files 
>>>>>> are
>>>>>> referred to
>>>>>> +in the expansion."
>>>>>> +-config=MC3R1.R20.12,macros+={deliberate,
>>>>>> "name(DEFINE)&&loc(file(asm_offsets))"}
>>>>>> +-doc_end
>>>>> 
>>>>> Can you give an example of such a reference? Nothing _in_
>>>>> asm-offsets.c
>>>>> should be referenced, I'd think. Only stuff in asm-offsets.h as
>>>>> _generated
>>>>> from_ asm-offsets.c will, of course, be.
>>>>> 
>>>> 
>>>> Perhaps I could have expressed that more clearly. What I meant is 
>>>> that
>>>> there are some arguments to DEFINE that are not part of 
>>>> asm-offsets.c,
>>>> therefore they end up in the violation report, but are not actually
>>>> relevant, because the macro DEFINE is actually what we want to
>>>> exclude.
>>>> 
>>>> See for instance at the link below VCPU_TRAP_{NMI,MCE}, which are
>>>> defined in asm/domain.h and used as arguments to DEFINE inside
>>>> asm-offsets.c.
>>>> 
>>>> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/staging/X86_64-BUGSENG/676/PROJECT.ecd;/by_service/MC3R1.R20.12.html
>>> 
>>> I'm afraid I still don't understand: The file being supposed to be
>>> excluded from scanning, why does it even show up in that report?
>> 
>> The report is made up of several source code locations. Three of them
>> are within asm-offsets.c, which is excluded from compliance but still
>> analyzed, but one references a macro definition in another file (e.g.,
>> VCPU_TRAP_NMI from asm/domain.h). So in this case the exclusion of
>> asm-offsets.c is not enough for the report not to be shown.
> 
> But the (would-be-)violation is in asm-offsets.c. The other locations
> pointed at are providing context. To report a violation, it should be
> enough to exclude the file where the violation itself is?
> 

In general that may not be the safest choice, or it can limit the 
granularity of the configuration (not in the specific case, but the 
mechanism is general).
It's a design aspect of the tool to show a report unless all its 
locations are not meant to be shown.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 4/5] automation/eclair_analysis: address remaining violations of MISRA C Rule 20.12

[Stefano Stabellini]

On Sat, 1 Jun 2024, Nicola Vetrini wrote:
> The DEFINE macro in asm-offsets.c (for all architectures) still generates
> violations despite the file(s) being excluded from compliance, due to the
> fact that in its expansion it sometimes refers entities in non-excluded files.
> These corner cases are deviated by the configuration.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

[XEN PATCH 5/5] xen: fix MISRA regressions on rule 20.9 and 20.12

[Nicola Vetrini]

ea59e7d780d9 ("xen/bitops: Cleanup and new infrastructure ahead of rearrangements")
introduced new violations on previously clean rules 20.9 and 20.12.

The first is introduced because CONFIG_CC_IS_CLANG in xen/self-tests.h is not
defined in the configuration under analysis. Using "defined()" instead avoids
relying on the preprocessor's behaviour upon encountering an undedfined identifier
and addresses the violation.

The violation of Rule 20.12 is due to "val" being used both as an ordinary argument
in macro RUNTIME_CHECK, and as a stringification operator.

No functional change.

Fixes: ea59e7d780d9 ("xen/bitops: Cleanup and new infrastructure ahead of rearrangements")
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 2 +-
 xen/include/xen/self-tests.h                     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index f29db9e08248..e2653f77eb2c 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -473,7 +473,7 @@ deliberate."
 -doc_begin="Uses of a macro parameter for ordinary expansion and as an operand
 to the # or ## operators within the following macros are deliberate, to provide
 useful diagnostic messages to the user."
--config=MC3R1.R20.12,macros+={deliberate, "name(ASSERT||BUILD_BUG_ON||BUILD_BUG_ON_ZERO)"}
+-config=MC3R1.R20.12,macros+={deliberate, "name(ASSERT||BUILD_BUG_ON||BUILD_BUG_ON_ZERO||RUNTIME_CHECK)"}
 -doc_end
 
 -doc_begin="The helper macro GENERATE_CASE may use a macro parameter for ordinary
diff --git a/xen/include/xen/self-tests.h b/xen/include/xen/self-tests.h
index 8410db7aaaae..42a4cc4d17fe 100644
--- a/xen/include/xen/self-tests.h
+++ b/xen/include/xen/self-tests.h
@@ -16,7 +16,7 @@
  * Clang < 8 can't fold constants through static inlines, causing this to
  * fail.  Simply skip it for incredibly old compilers.
  */
-#if !CONFIG_CC_IS_CLANG || CONFIG_CLANG_VERSION >= 80000
+#if !defined(CONFIG_CC_IS_CLANG) || CONFIG_CLANG_VERSION >= 80000
 #define COMPILE_CHECK(fn, val, res)                                     \
     do {                                                                \
         typeof(fn(val)) real = fn(val);                                 \
-- 
2.34.1

Re: [XEN PATCH 5/5] xen: fix MISRA regressions on rule 20.9 and 20.12

[Andrew Cooper]

On 01/06/2024 11:16 am, Nicola Vetrini wrote:
> ea59e7d780d9 ("xen/bitops: Cleanup and new infrastructure ahead of rearrangements")
> introduced new violations on previously clean rules 20.9 and 20.12.
>
> The first is introduced because CONFIG_CC_IS_CLANG in xen/self-tests.h is not
> defined in the configuration under analysis. Using "defined()" instead avoids
> relying on the preprocessor's behaviour upon encountering an undedfined identifier
> and addresses the violation.
>
> The violation of Rule 20.12 is due to "val" being used both as an ordinary argument
> in macro RUNTIME_CHECK, and as a stringification operator.
>
> No functional change.
>
> Fixes: ea59e7d780d9 ("xen/bitops: Cleanup and new infrastructure ahead of rearrangements")
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Thankyou for this patch.  I'd seen that I'd broken something.  (Entirely
my fault - I've done a lot of testing in Gitlab for the series, but
never manually ran the Eclair jobs.  I'll try to remember better next time.)

One question though. 
https://gitlab.com/xen-project/xen/-/jobs/6994213979 says:

Failure: 1 regressions found for clean guidelines
  service MC3R1.R20.9: (required) All identifiers used in the
controlling expression of `#if' or `#elif' preprocessing directives
shall be #define'd before evaluation:
   violation: 1

While there is a report for 20.12, it's not clean yet (so the first
sentence wants adjusting), and RUNTIME_CHECK doesn't show up newly in it.

So, while I agree that RUNTIME_CHECK() should be included in the 20.12
exclusions, why is current Gitlab Run not reporting it?

~Andrew

Re: [XEN PATCH 5/5] xen: fix MISRA regressions on rule 20.9 and 20.12

[Nicola Vetrini]

On 2024-06-01 14:47, Andrew Cooper wrote:
> On 01/06/2024 11:16 am, Nicola Vetrini wrote:
>> ea59e7d780d9 ("xen/bitops: Cleanup and new infrastructure ahead of 
>> rearrangements")
>> introduced new violations on previously clean rules 20.9 and 20.12.
>> 
>> The first is introduced because CONFIG_CC_IS_CLANG in xen/self-tests.h 
>> is not
>> defined in the configuration under analysis. Using "defined()" instead 
>> avoids
>> relying on the preprocessor's behaviour upon encountering an 
>> undedfined identifier
>> and addresses the violation.
>> 
>> The violation of Rule 20.12 is due to "val" being used both as an 
>> ordinary argument
>> in macro RUNTIME_CHECK, and as a stringification operator.
>> 
>> No functional change.
>> 
>> Fixes: ea59e7d780d9 ("xen/bitops: Cleanup and new infrastructure ahead 
>> of rearrangements")
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Thankyou for this patch.  I'd seen that I'd broken something.  
> (Entirely
> my fault - I've done a lot of testing in Gitlab for the series, but
> never manually ran the Eclair jobs.  I'll try to remember better next 
> time.)
> 
> One question though. 
> https://gitlab.com/xen-project/xen/-/jobs/6994213979 says:
> 
> Failure: 1 regressions found for clean guidelines
>   service MC3R1.R20.9: (required) All identifiers used in the
> controlling expression of `#if' or `#elif' preprocessing directives
> shall be #define'd before evaluation:
>    violation: 1
> 
> While there is a report for 20.12, it's not clean yet (so the first
> sentence wants adjusting), and RUNTIME_CHECK doesn't show up newly in 
> it.
> 
> So, while I agree that RUNTIME_CHECK() should be included in the 20.12
> exclusions, why is current Gitlab Run not reporting it?
> 
> ~Andrew

While Rule 20.12 wasn't clean on x86, but it was on arm, so in the arm64 
run it is reported as such

https://gitlab.com/xen-project/xen/-/jobs/6994213980

with the other patches in the series the rule should be clean on both 
architectures, so this imbalance should disappear rather shortly.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 5/5] xen: fix MISRA regressions on rule 20.9 and 20.12

[Andrew Cooper]

On 01/06/2024 1:58 pm, Nicola Vetrini wrote:
> On 2024-06-01 14:47, Andrew Cooper wrote:
>> On 01/06/2024 11:16 am, Nicola Vetrini wrote:
>>> ea59e7d780d9 ("xen/bitops: Cleanup and new infrastructure ahead of
>>> rearrangements")
>>> introduced new violations on previously clean rules 20.9 and 20.12.
>>>
>>> The first is introduced because CONFIG_CC_IS_CLANG in
>>> xen/self-tests.h is not
>>> defined in the configuration under analysis. Using "defined()"
>>> instead avoids
>>> relying on the preprocessor's behaviour upon encountering an
>>> undedfined identifier
>>> and addresses the violation.
>>>
>>> The violation of Rule 20.12 is due to "val" being used both as an
>>> ordinary argument
>>> in macro RUNTIME_CHECK, and as a stringification operator.
>>>
>>> No functional change.
>>>
>>> Fixes: ea59e7d780d9 ("xen/bitops: Cleanup and new infrastructure
>>> ahead of rearrangements")
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>
>> Thankyou for this patch.  I'd seen that I'd broken something.  (Entirely
>> my fault - I've done a lot of testing in Gitlab for the series, but
>> never manually ran the Eclair jobs.  I'll try to remember better next
>> time.)
>>
>> One question though. 
>> https://gitlab.com/xen-project/xen/-/jobs/6994213979 says:
>>
>> Failure: 1 regressions found for clean guidelines
>>   service MC3R1.R20.9: (required) All identifiers used in the
>> controlling expression of `#if' or `#elif' preprocessing directives
>> shall be #define'd before evaluation:
>>    violation: 1
>>
>> While there is a report for 20.12, it's not clean yet (so the first
>> sentence wants adjusting), and RUNTIME_CHECK doesn't show up newly in
>> it.
>>
>> So, while I agree that RUNTIME_CHECK() should be included in the 20.12
>> exclusions, why is current Gitlab Run not reporting it?
>>
>> ~Andrew
>
> While Rule 20.12 wasn't clean on x86, but it was on arm, so in the
> arm64 run it is reported as such
>
> https://gitlab.com/xen-project/xen/-/jobs/6994213980
>
> with the other patches in the series the rule should be clean on both
> architectures, so this imbalance should disappear rather shortly.
>

Thanks - I'd just found the ARM report and was replying - I'll rebase
onto this thread.

I still don't understand why the violation doesn't show up on the x86
build.  Specifically, why it's missing from here:
https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/xen/ECLAIR_normal/staging/X86_64/6994213979/prev/PROJECT.ecd;/by_service/MC3R1.R20.12.html

From the ARM report, one is xen/lib/generic-ffsl.c:61 and checking with
the x86 build log, we are building generic-ffsl.c too (which is expected.)

~Andrew

Re: [XEN PATCH 5/5] xen: fix MISRA regressions on rule 20.9 and 20.12

[Nicola Vetrini]

On 2024-06-01 15:08, Andrew Cooper wrote:
> On 01/06/2024 1:58 pm, Nicola Vetrini wrote:
>> On 2024-06-01 14:47, Andrew Cooper wrote:
>>> On 01/06/2024 11:16 am, Nicola Vetrini wrote:
>>>> ea59e7d780d9 ("xen/bitops: Cleanup and new infrastructure ahead of
>>>> rearrangements")
>>>> introduced new violations on previously clean rules 20.9 and 20.12.
>>>> 
>>>> The first is introduced because CONFIG_CC_IS_CLANG in
>>>> xen/self-tests.h is not
>>>> defined in the configuration under analysis. Using "defined()"
>>>> instead avoids
>>>> relying on the preprocessor's behaviour upon encountering an
>>>> undedfined identifier
>>>> and addresses the violation.
>>>> 
>>>> The violation of Rule 20.12 is due to "val" being used both as an
>>>> ordinary argument
>>>> in macro RUNTIME_CHECK, and as a stringification operator.
>>>> 
>>>> No functional change.
>>>> 
>>>> Fixes: ea59e7d780d9 ("xen/bitops: Cleanup and new infrastructure
>>>> ahead of rearrangements")
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> 
>>> Thankyou for this patch.  I'd seen that I'd broken something.  
>>> (Entirely
>>> my fault - I've done a lot of testing in Gitlab for the series, but
>>> never manually ran the Eclair jobs.  I'll try to remember better next
>>> time.)
>>> 
>>> One question though. 
>>> https://gitlab.com/xen-project/xen/-/jobs/6994213979 says:
>>> 
>>> Failure: 1 regressions found for clean guidelines
>>>   service MC3R1.R20.9: (required) All identifiers used in the
>>> controlling expression of `#if' or `#elif' preprocessing directives
>>> shall be #define'd before evaluation:
>>>    violation: 1
>>> 
>>> While there is a report for 20.12, it's not clean yet (so the first
>>> sentence wants adjusting), and RUNTIME_CHECK doesn't show up newly in
>>> it.
>>> 
>>> So, while I agree that RUNTIME_CHECK() should be included in the 
>>> 20.12
>>> exclusions, why is current Gitlab Run not reporting it?
>>> 
>>> ~Andrew
>> 
>> While Rule 20.12 wasn't clean on x86, but it was on arm, so in the
>> arm64 run it is reported as such
>> 
>> https://gitlab.com/xen-project/xen/-/jobs/6994213980
>> 
>> with the other patches in the series the rule should be clean on both
>> architectures, so this imbalance should disappear rather shortly.
>> 
> 
> Thanks - I'd just found the ARM report and was replying - I'll rebase
> onto this thread.
> 
> I still don't understand why the violation doesn't show up on the x86
> build.  Specifically, why it's missing from here:
> https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/xen/ECLAIR_normal/staging/X86_64/6994213979/prev/PROJECT.ecd;/by_service/MC3R1.R20.12.html
> 

Note the "prev" here in the URL: this means you're looking at the 
analysis run before your series was merged (on 03147e6837ff045db)

this is the latest run for x86 on staging:

https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/xen/ECLAIR_normal/staging/X86_64/6994213979/PROJECT.ecd;/by_service/MC3R1.R20.12.html

> From the ARM report, one is xen/lib/generic-ffsl.c:61 and checking with
> the x86 build log, we are building generic-ffsl.c too (which is 
> expected.)
> 
> ~Andrew

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 5/5] xen: fix MISRA regressions on rule 20.9 and 20.12

[Andrew Cooper]

On 01/06/2024 2:52 pm, Nicola Vetrini wrote:
> On 2024-06-01 15:08, Andrew Cooper wrote:
>> On 01/06/2024 1:58 pm, Nicola Vetrini wrote:
>>> On 2024-06-01 14:47, Andrew Cooper wrote:
>>>> On 01/06/2024 11:16 am, Nicola Vetrini wrote:
>>>>> ea59e7d780d9 ("xen/bitops: Cleanup and new infrastructure ahead of
>>>>> rearrangements")
>>>>> introduced new violations on previously clean rules 20.9 and 20.12.
>>>>>
>>>>> The first is introduced because CONFIG_CC_IS_CLANG in
>>>>> xen/self-tests.h is not
>>>>> defined in the configuration under analysis. Using "defined()"
>>>>> instead avoids
>>>>> relying on the preprocessor's behaviour upon encountering an
>>>>> undedfined identifier
>>>>> and addresses the violation.
>>>>>
>>>>> The violation of Rule 20.12 is due to "val" being used both as an
>>>>> ordinary argument
>>>>> in macro RUNTIME_CHECK, and as a stringification operator.
>>>>>
>>>>> No functional change.
>>>>>
>>>>> Fixes: ea59e7d780d9 ("xen/bitops: Cleanup and new infrastructure
>>>>> ahead of rearrangements")
>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>
>>>> Thankyou for this patch.  I'd seen that I'd broken something. 
>>>> (Entirely
>>>> my fault - I've done a lot of testing in Gitlab for the series, but
>>>> never manually ran the Eclair jobs.  I'll try to remember better next
>>>> time.)
>>>>
>>>> One question though. 
>>>> https://gitlab.com/xen-project/xen/-/jobs/6994213979 says:
>>>>
>>>> Failure: 1 regressions found for clean guidelines
>>>>   service MC3R1.R20.9: (required) All identifiers used in the
>>>> controlling expression of `#if' or `#elif' preprocessing directives
>>>> shall be #define'd before evaluation:
>>>>    violation: 1
>>>>
>>>> While there is a report for 20.12, it's not clean yet (so the first
>>>> sentence wants adjusting), and RUNTIME_CHECK doesn't show up newly in
>>>> it.
>>>>
>>>> So, while I agree that RUNTIME_CHECK() should be included in the 20.12
>>>> exclusions, why is current Gitlab Run not reporting it?
>>>>
>>>> ~Andrew
>>>
>>> While Rule 20.12 wasn't clean on x86, but it was on arm, so in the
>>> arm64 run it is reported as such
>>>
>>> https://gitlab.com/xen-project/xen/-/jobs/6994213980
>>>
>>> with the other patches in the series the rule should be clean on both
>>> architectures, so this imbalance should disappear rather shortly.
>>>
>>
>> Thanks - I'd just found the ARM report and was replying - I'll rebase
>> onto this thread.
>>
>> I still don't understand why the violation doesn't show up on the x86
>> build.  Specifically, why it's missing from here:
>> https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/xen/ECLAIR_normal/staging/X86_64/6994213979/prev/PROJECT.ecd;/by_service/MC3R1.R20.12.html
>>
>>
>
> Note the "prev" here in the URL: this means you're looking at the
> analysis run before your series was merged (on 03147e6837ff045db)
>
> this is the latest run for x86 on staging:
>
> https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/xen/ECLAIR_normal/staging/X86_64/6994213979/PROJECT.ecd;/by_service/MC3R1.R20.12.html

Oh - thankyou for explaining.

~Andrew

Re: [XEN PATCH 0/5] address violations of MISRA C rules

[Andrew Cooper]

On 01/06/2024 11:16 am, Nicola Vetrini wrote:
> Patches 1 to 4 address violations of MISRA C Rule 20.12 by deviating certain
> uses of some macros, while the last patch addresses some regressions introduced
> by the latest bitops series
>
> Nicola Vetrini (5):
>   xen/domain: deviate violation of MISRA C Rule 20.12
>   x86/domain: deviate violation of MISRA C Rule 20.12
>   x86: deviate violation of MISRA C Rule 20.12
>   automation/eclair_analysis: address remaining violations of MISRA C
>     Rule 20.12
>   xen: fix MISRA regressions on rule 20.9 and 20.12

I've committed patch 5 because it fixes a blocking failure in Gitlab CI
from content already accepted for Xen 4.19.

The others look fine to me, but you'll need to negotiate with Oleksii
(CC'd) to get them in, at this point in the release.

Given that this series makes x86 clean to Rule 20.12, shouldn't there be
a final patch making it blocking, to bring x86 in line with ARM?

~Andrew

Re: [XEN PATCH 0/5] address violations of MISRA C rules

[Nicola Vetrini]

On 2024-06-01 16:37, Andrew Cooper wrote:
> On 01/06/2024 11:16 am, Nicola Vetrini wrote:
>> Patches 1 to 4 address violations of MISRA C Rule 20.12 by deviating 
>> certain
>> uses of some macros, while the last patch addresses some regressions 
>> introduced
>> by the latest bitops series
>> 
>> Nicola Vetrini (5):
>>   xen/domain: deviate violation of MISRA C Rule 20.12
>>   x86/domain: deviate violation of MISRA C Rule 20.12
>>   x86: deviate violation of MISRA C Rule 20.12
>>   automation/eclair_analysis: address remaining violations of MISRA C
>>     Rule 20.12
>>   xen: fix MISRA regressions on rule 20.9 and 20.12
> 
> I've committed patch 5 because it fixes a blocking failure in Gitlab CI
> from content already accepted for Xen 4.19.
> 

Thanks

> The others look fine to me, but you'll need to negotiate with Oleksii
> (CC'd) to get them in, at this point in the release.
> 

Well, having one more clean rule does look better for Xen, and the 
changes to the codebase are harmless enough, but ultimately given the 
closeness with the deadline I didn't really see a need to.

> Given that this series makes x86 clean to Rule 20.12, shouldn't there 
> be
> a final patch making it blocking, to bring x86 in line with ARM?
> 

Good point, I should have done that in patch 4. I'll do a follow-up 
patch.

> ~Andrew

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 0/5] address violations of MISRA C rules

[Andrew Cooper]

On 01/06/2024 6:19 pm, Nicola Vetrini wrote:
> On 2024-06-01 16:37, Andrew Cooper wrote:
>> On 01/06/2024 11:16 am, Nicola Vetrini wrote:
>>> Patches 1 to 4 address violations of MISRA C Rule 20.12 by deviating
>>> certain
>>> uses of some macros, while the last patch addresses some regressions
>>> introduced
>>> by the latest bitops series
>>>
>>> Nicola Vetrini (5):
>>>   xen/domain: deviate violation of MISRA C Rule 20.12
>>>   x86/domain: deviate violation of MISRA C Rule 20.12
>>>   x86: deviate violation of MISRA C Rule 20.12
>>>   automation/eclair_analysis: address remaining violations of MISRA C
>>>     Rule 20.12
>>>   xen: fix MISRA regressions on rule 20.9 and 20.12
>>
>> I've committed patch 5 because it fixes a blocking failure in Gitlab CI
>> from content already accepted for Xen 4.19.
>>
>
> Thanks
>
>> The others look fine to me, but you'll need to negotiate with Oleksii
>> (CC'd) to get them in, at this point in the release.
>>
>
> Well, having one more clean rule does look better for Xen, and the
> changes to the codebase are harmless enough, but ultimately given the
> closeness with the deadline I didn't really see a need to.
>
>> Given that this series makes x86 clean to Rule 20.12, shouldn't there be
>> a final patch making it blocking, to bring x86 in line with ARM?
>>
>
> Good point, I should have done that in patch 4. I'll do a follow-up
> patch.

FWIW, given how simple this series is, I'm +1 for including it in 4.19,
even at this point.  It is definitely nicer to have the disposition of
Rule 20.12 the same between ARM and x86.

Still - it's very much Oleksii's call.

~Andrew