[XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[nicola.vetrini]

From: Nicola Vetrini <nicola.vetrini@bugseng.com>

The xen sources contain several violations of Rule 3.1 from MISRA C:2012,
whose headline states:
"The character sequences '/*' and '//' shall not be used within a comment".

Most of the violations are due to the presence of links to webpages within
C-style comment blocks, such as:

xen/arch/arm/include/asm/smccc.h:37.1-41.3
/*
 * This file provides common defines for ARM SMC Calling Convention as
 * specified in
 * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
 */

In this case, we propose to deviate all of these occurrences with a
project deviation to be captured by a tool configuration.

There are, however, a few other violations that do not fall under this
category, namely:

1. in file "xen/arch/arm/include/asm/arm64/flushtlb.h" we propose to
avoid the usage of a nested comment;
2. in file "xen/common/xmalloc_tlsf.c" we propose to substitute the
commented-out if statement with a "#if 0 .. #endif;
3. in file "xen/include/xen/atomic.h" and
"xen/drivers/passthrough/arm/smmu-v3.c" we propose to split the C-style
comment containing the nested comment into two doxygen comments, clearly
identifying the second as a code sample. This can then be captured with a
project deviation by a tool configuration.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/arm/include/asm/arm64/flushtlb.h | 8 ++++----
 xen/common/xmalloc_tlsf.c                 | 7 ++++---
 xen/drivers/passthrough/arm/smmu-v3.c     | 9 ++++++---
 xen/include/xen/atomic.h                  | 5 ++++-
 4 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/include/asm/arm64/flushtlb.h b/xen/arch/arm/include/asm/arm64/flushtlb.h
index 3a9092b814..90ac3f9809 100644
--- a/xen/arch/arm/include/asm/arm64/flushtlb.h
+++ b/xen/arch/arm/include/asm/arm64/flushtlb.h
@@ -4,10 +4,10 @@
 /*
  * Every invalidation operation use the following patterns:
  *
- * DSB ISHST        // Ensure prior page-tables updates have completed
- * TLBI...          // Invalidate the TLB
- * DSB ISH          // Ensure the TLB invalidation has completed
- * ISB              // See explanation below
+ * DSB ISHST        Ensure prior page-tables updates have completed
+ * TLBI...          Invalidate the TLB
+ * DSB ISH          Ensure the TLB invalidation has completed
+ * ISB              See explanation below
  *
  * ARM64_WORKAROUND_REPEAT_TLBI:
  * Modification of the translation table for a virtual address might lead to
diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
index 75bdf18c4e..ea6ec47a59 100644
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl)
         *fl = flsl(*r) - 1;
         *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
         *fl -= FLI_OFFSET;
-        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
-         *fl = *sl = 0;
-         */
+#if 0
+        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
+        fl = *sl = 0;
+#endif
         *r &= ~t;
     }
 }
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index 720aa69ff2..b1c536e7d9 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -1045,15 +1045,18 @@ static int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
 	/*
 	 * Ensure that we've completed prior invalidation of the main TLBs
 	 * before we read 'nr_ats_masters' in case of a concurrent call to
-	 * arm_smmu_enable_ats():
+	 * arm_smmu_enable_ats().
+	 */
+	/**
+	 * Code sample: Ensures that we always see the incremented
+	 * 'nr_ats_masters' count if ATS was enabled at the PCI device before
+	 * completion of the TLBI.
 	 *
 	 *	// unmap()			// arm_smmu_enable_ats()
 	 *	TLBI+SYNC			atomic_inc(&nr_ats_masters);
 	 *	smp_mb();			[...]
 	 *	atomic_read(&nr_ats_masters);	pci_enable_ats() // writel()
 	 *
-	 * Ensures that we always see the incremented 'nr_ats_masters' count if
-	 * ATS was enabled at the PCI device before completion of the TLBI.
 	 */
 	smp_mb();
 	if (!atomic_read(&smmu_domain->nr_ats_masters))
diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h
index 529213ebbb..829646dda0 100644
--- a/xen/include/xen/atomic.h
+++ b/xen/include/xen/atomic.h
@@ -71,7 +71,10 @@ static inline void _atomic_set(atomic_t *v, int i);
  * Returns the initial value in @v, hence succeeds when the return value
  * matches that of @old.
  *
- * Sample (tries atomic increment of v until the operation succeeds):
+ */
+/**
+ *
+ * Code sample: Tries atomic increment of v until the operation succeeds.
  *
  *  while(1)
  *  {
-- 
2.34.1

Re: [XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[Jan Beulich]

On 12.06.2023 18:10, nicola.vetrini@bugseng.com wrote:
> From: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> The xen sources contain several violations of Rule 3.1 from MISRA C:2012,
> whose headline states:
> "The character sequences '/*' and '//' shall not be used within a comment".
> 
> Most of the violations are due to the presence of links to webpages within
> C-style comment blocks, such as:
> 
> xen/arch/arm/include/asm/smccc.h:37.1-41.3
> /*
>  * This file provides common defines for ARM SMC Calling Convention as
>  * specified in
>  * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
>  */
> 
> In this case, we propose to deviate all of these occurrences with a
> project deviation to be captured by a tool configuration.

Here "propose" is appropriate in the description, as this is something
the patch does not do. Further down, however, you mean to describe what
the patch does, not what an eventual patch might do.

Somewhat similarly you don't want to use past tense in the title, as
that wants to describe what is being done, rather than describing a
patch that has already landed.

> There are, however, a few other violations that do not fall under this
> category, namely:
> 
> 1. in file "xen/arch/arm/include/asm/arm64/flushtlb.h" we propose to
> avoid the usage of a nested comment;
> 2. in file "xen/common/xmalloc_tlsf.c" we propose to substitute the
> commented-out if statement with a "#if 0 .. #endif;
> 3. in file "xen/include/xen/atomic.h" and
> "xen/drivers/passthrough/arm/smmu-v3.c" we propose to split the C-style
> comment containing the nested comment into two doxygen comments, clearly
> identifying the second as a code sample. This can then be captured with a
> project deviation by a tool configuration.

Finally I find the use of "we" somewhat suspicious. Who besides you is
it that you're talking about?

Also please don't forget to Cc relevant maintainers.

> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -1045,15 +1045,18 @@ static int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
>  	/*
>  	 * Ensure that we've completed prior invalidation of the main TLBs
>  	 * before we read 'nr_ats_masters' in case of a concurrent call to
> -	 * arm_smmu_enable_ats():
> +	 * arm_smmu_enable_ats().
> +	 */
> +	/**
> +	 * Code sample: Ensures that we always see the incremented
> +	 * 'nr_ats_masters' count if ATS was enabled at the PCI device before
> +	 * completion of the TLBI.
>  	 *
>  	 *	// unmap()			// arm_smmu_enable_ats()
>  	 *	TLBI+SYNC			atomic_inc(&nr_ats_masters);
>  	 *	smp_mb();			[...]
>  	 *	atomic_read(&nr_ats_masters);	pci_enable_ats() // writel()
>  	 *
> -	 * Ensures that we always see the incremented 'nr_ats_masters' count if
> -	 * ATS was enabled at the PCI device before completion of the TLBI.
>  	 */
>  	smp_mb();
>  	if (!atomic_read(&smmu_domain->nr_ats_masters))

I don't really know this code, but the use of inner comments looks
unnecessary to me here. The same could e.g. be achieved by

	 *	unmap():			arm_smmu_enable_ats():
	 *	TLBI+SYNC			atomic_inc(&nr_ats_masters);
	 *	smp_mb();			[...]
	 *	atomic_read(&nr_ats_masters);	pci_enable_ats(); (i.e. writel())


and it would avoid the somewhat odd splitting of adjacent comments
(which then is at risk of someone later coming forward with a patch
re-combining them).

> --- a/xen/include/xen/atomic.h
> +++ b/xen/include/xen/atomic.h
> @@ -71,7 +71,10 @@ static inline void _atomic_set(atomic_t *v, int i);
>   * Returns the initial value in @v, hence succeeds when the return value
>   * matches that of @old.
>   *
> - * Sample (tries atomic increment of v until the operation succeeds):
> + */
> +/**
> + *
> + * Code sample: Tries atomic increment of v until the operation succeeds.
>   *
>   *  while(1)
>   *  {

Somewhat similarly here: I don't think the inner comment provides
much value, and could hence be dropped instead of splitting the comment.

Jan

[XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[Nicola Vetrini]

The xen sources contain several violations of Rule 3.1 from MISRA C:2012,
whose headline states:
"The character sequences '/*' and '//' shall not be used within a comment".

Most of the violations are due to the presence of links to webpages within
C-style comment blocks, such as:

xen/arch/arm/include/asm/smccc.h:37.1-41.3
/*
 * This file provides common defines for ARM SMC Calling Convention as
 * specified in
 * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
 */

In this case, we propose to deviate all of these occurrences with a
project deviation to be captured by a tool configuration.

There are, however, a few other violations that do not fall under this
category, namely:

1. in file "xen/arch/arm/include/asm/arm64/flushtlb.h" we propose to
avoid the usage of a nested comment;
2. in file "xen/common/xmalloc_tlsf.c" we propose to substitute the
commented-out if statement with a "#if 0 .. #endif;
3. in file "xen/include/xen/atomic.h" and
"xen/drivers/passthrough/arm/smmu-v3.c" we propose to split the C-style
comment containing the nested comment into two doxygen comments, clearly
identifying the second as a code sample. This can then be captured with a
project deviation by a tool configuration.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes:
- Resending the patch with the right maintainers in CC.
---
 xen/arch/arm/include/asm/arm64/flushtlb.h | 8 ++++----
 xen/common/xmalloc_tlsf.c                 | 7 ++++---
 xen/drivers/passthrough/arm/smmu-v3.c     | 9 ++++++---
 xen/include/xen/atomic.h                  | 5 ++++-
 4 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/include/asm/arm64/flushtlb.h b/xen/arch/arm/include/asm/arm64/flushtlb.h
index 3a9092b814..90ac3f9809 100644
--- a/xen/arch/arm/include/asm/arm64/flushtlb.h
+++ b/xen/arch/arm/include/asm/arm64/flushtlb.h
@@ -4,10 +4,10 @@
 /*
  * Every invalidation operation use the following patterns:
  *
- * DSB ISHST        // Ensure prior page-tables updates have completed
- * TLBI...          // Invalidate the TLB
- * DSB ISH          // Ensure the TLB invalidation has completed
- * ISB              // See explanation below
+ * DSB ISHST        Ensure prior page-tables updates have completed
+ * TLBI...          Invalidate the TLB
+ * DSB ISH          Ensure the TLB invalidation has completed
+ * ISB              See explanation below
  *
  * ARM64_WORKAROUND_REPEAT_TLBI:
  * Modification of the translation table for a virtual address might lead to
diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
index 75bdf18c4e..ea6ec47a59 100644
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl)
         *fl = flsl(*r) - 1;
         *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
         *fl -= FLI_OFFSET;
-        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
-         *fl = *sl = 0;
-         */
+#if 0
+        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
+        fl = *sl = 0;
+#endif
         *r &= ~t;
     }
 }
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index 720aa69ff2..b1c536e7d9 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -1045,15 +1045,18 @@ static int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
 	/*
 	 * Ensure that we've completed prior invalidation of the main TLBs
 	 * before we read 'nr_ats_masters' in case of a concurrent call to
-	 * arm_smmu_enable_ats():
+	 * arm_smmu_enable_ats().
+	 */
+	/**
+	 * Code sample: Ensures that we always see the incremented
+	 * 'nr_ats_masters' count if ATS was enabled at the PCI device before
+	 * completion of the TLBI.
 	 *
 	 *	// unmap()			// arm_smmu_enable_ats()
 	 *	TLBI+SYNC			atomic_inc(&nr_ats_masters);
 	 *	smp_mb();			[...]
 	 *	atomic_read(&nr_ats_masters);	pci_enable_ats() // writel()
 	 *
-	 * Ensures that we always see the incremented 'nr_ats_masters' count if
-	 * ATS was enabled at the PCI device before completion of the TLBI.
 	 */
 	smp_mb();
 	if (!atomic_read(&smmu_domain->nr_ats_masters))
diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h
index 529213ebbb..829646dda0 100644
--- a/xen/include/xen/atomic.h
+++ b/xen/include/xen/atomic.h
@@ -71,7 +71,10 @@ static inline void _atomic_set(atomic_t *v, int i);
  * Returns the initial value in @v, hence succeeds when the return value
  * matches that of @old.
  *
- * Sample (tries atomic increment of v until the operation succeeds):
+ */
+/**
+ *
+ * Code sample: Tries atomic increment of v until the operation succeeds.
  *
  *  while(1)
  *  {
-- 
2.34.1

Re: [XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[Jan Beulich]

On 13.06.2023 09:42, Nicola Vetrini wrote:
> The xen sources contain several violations of Rule 3.1 from MISRA C:2012,
> whose headline states:
> "The character sequences '/*' and '//' shall not be used within a comment".
> 
> Most of the violations are due to the presence of links to webpages within
> C-style comment blocks, such as:
> 
> xen/arch/arm/include/asm/smccc.h:37.1-41.3
> /*
>  * This file provides common defines for ARM SMC Calling Convention as
>  * specified in
>  * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
>  */
> 
> In this case, we propose to deviate all of these occurrences with a
> project deviation to be captured by a tool configuration.
> 
> There are, however, a few other violations that do not fall under this
> category, namely:
> 
> 1. in file "xen/arch/arm/include/asm/arm64/flushtlb.h" we propose to
> avoid the usage of a nested comment;
> 2. in file "xen/common/xmalloc_tlsf.c" we propose to substitute the
> commented-out if statement with a "#if 0 .. #endif;
> 3. in file "xen/include/xen/atomic.h" and
> "xen/drivers/passthrough/arm/smmu-v3.c" we propose to split the C-style
> comment containing the nested comment into two doxygen comments, clearly
> identifying the second as a code sample. This can then be captured with a
> project deviation by a tool configuration.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> Changes:
> - Resending the patch with the right maintainers in CC.

But without otherwise addressing comments already given, afaics. One more
remark:

> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl)
>          *fl = flsl(*r) - 1;
>          *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
>          *fl -= FLI_OFFSET;
> -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
> -         *fl = *sl = 0;
> -         */
> +#if 0
> +        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
> +        fl = *sl = 0;

You want to get indentation right here, and you don't want to lose
the indirection on fl.

> +#endif
>          *r &= ~t;
>      }
>  }

If you split this to 4 patches, leaving the URL proposal in just
the cover letter, then I think this one change (with the adjustments)
could go in right away. Similarly I expect the arm64/flushtlb.h
change could be ack-ed right away by an Arm maintainer.

Jan

Re: [XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[Julien Grall]

Hi,

On 13/06/2023 09:27, Jan Beulich wrote:
> On 13.06.2023 09:42, Nicola Vetrini wrote:
>> The xen sources contain several violations of Rule 3.1 from MISRA C:2012,
>> whose headline states:
>> "The character sequences '/*' and '//' shall not be used within a comment".
>>
>> Most of the violations are due to the presence of links to webpages within
>> C-style comment blocks, such as:
>>
>> xen/arch/arm/include/asm/smccc.h:37.1-41.3
>> /*
>>   * This file provides common defines for ARM SMC Calling Convention as
>>   * specified in
>>   * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
>>   */
>>
>> In this case, we propose to deviate all of these occurrences with a
>> project deviation to be captured by a tool configuration.
>>
>> There are, however, a few other violations that do not fall under this
>> category, namely:
>>
>> 1. in file "xen/arch/arm/include/asm/arm64/flushtlb.h" we propose to
>> avoid the usage of a nested comment;
>> 2. in file "xen/common/xmalloc_tlsf.c" we propose to substitute the
>> commented-out if statement with a "#if 0 .. #endif;
>> 3. in file "xen/include/xen/atomic.h" and
>> "xen/drivers/passthrough/arm/smmu-v3.c" we propose to split the C-style
>> comment containing the nested comment into two doxygen comments, clearly
>> identifying the second as a code sample. This can then be captured with a
>> project deviation by a tool configuration.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> Changes:
>> - Resending the patch with the right maintainers in CC.
> 
> But without otherwise addressing comments already given, afaics. One more
> remark:
> 
>> --- a/xen/common/xmalloc_tlsf.c
>> +++ b/xen/common/xmalloc_tlsf.c
>> @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl)
>>           *fl = flsl(*r) - 1;
>>           *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
>>           *fl -= FLI_OFFSET;
>> -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>> -         *fl = *sl = 0;
>> -         */
>> +#if 0
>> +        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>> +        fl = *sl = 0;
> 
> You want to get indentation right here, and you don't want to lose
> the indirection on fl.
> 
>> +#endif
>>           *r &= ~t;
>>       }
>>   }
> 
> If you split this to 4 patches, leaving the URL proposal in just
> the cover letter, then I think this one change (with the adjustments)
> could go in right away. Similarly I expect the arm64/flushtlb.h
> change could be ack-ed right away by an Arm maintainer.

I actually dislike the proposal. In this case, the code is meant to look 
like assembly code. I would replace the // with ;. Also, I would like to 
keep the comment style in sync in arm32/flushtlb.h. So can this be 
updated as well?

Cheers,

-- 
Julien Grall

Re: [XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[nicola]

[-- Attachment #1: Type: text/plain, Size: 3189 bytes --]


On 13/06/23 10:27, Jan Beulich wrote:

> On 13.06.2023 09:42, Nicola Vetrini wrote:
>> --- a/xen/common/xmalloc_tlsf.c
>> +++ b/xen/common/xmalloc_tlsf.c
>> @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl)
>>           *fl = flsl(*r) - 1;
>>           *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
>>           *fl -= FLI_OFFSET;
>> -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>> -         *fl = *sl = 0;
>> -         */
>> +#if 0
>> +        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>> +        fl = *sl = 0;
> You want to get indentation right here, and you don't want to lose
> the indirection on fl.

Understood.


>> +#endif
>>           *r &= ~t;
>>       }
>>   }
> If you split this to 4 patches, leaving the URL proposal in just
> the cover letter, then I think this one change (with the adjustments)
> could go in right away. Similarly I expect the arm64/flushtlb.h
> change could be ack-ed right away by an Arm maintainer.
Ok. I do not understand what you mean by "leaving the URL proposal in 
just the cover letter". Which URL?



I'm sorry, I didn't notice your previous reply. I'm going to address 
your observations now:

> Here "propose" is appropriate in the description, as this is something 
> the patch does not do. Further down, however, you mean to describe 
> what the patch does, not what an eventual patch might do.
>
To my knowledge, there is not a standard format to define a project 
deviation for a certain MISRA rule in Xen right now (this can also be 
discussed in a separate thread). To clarify, I meant to describe why I 
wasn't addressing these violations in the patch (they are the vast 
majority, but they do not have any implication w.r.t. functional safety 
and can therefore be safely deviated with an appropriate written 
justification).


> Somewhat similarly you don't want to use past tense in the title, as
> that wants to describe what is being done, rather than describing a
> patch that has already landed.

Understood.


> and it would avoid the somewhat odd splitting of adjacent comments
> (which then is at risk of someone later coming forward with a patch
> re-combining them).
>
> >/--- a/xen/include/xen/atomic.h/
> >/+++ b/xen/include/xen/atomic.h/
> >/@@ -71,7 +71,10 @@ static inline void _atomic_set(atomic_t *v, int i);/
> >/* Returns the initial value in @v, hence succeeds when the return value/
> >/* matches that of @old./
> >/*/
> >/- * Sample (tries atomic increment of v until the operation succeeds):/
> >/+ *//
> >/+/**/
> >/+ */
> >/+ * Code sample: Tries atomic increment of v until the operation 
> succeeds./
> >/*/
> >/* while(1)/
> >/* {/
>
> Somewhat similarly here: I don't think the inner comment provides
> much value, and could hence be dropped instead of splitting the comment.

The rationale behind these modifications was to separate clearly 
comments and code samples, so that the latter can be easily deviated by 
tool configurations. I'm ok with the suggestion of removing the nested 
comments, and otherwise leave the code as is, but i'll probably do this 
by splitting the patch as you suggested above.

Regards,

   Nicola

[-- Attachment #2: Type: text/html, Size: 4505 bytes --]

Re: [XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[Jan Beulich]

First of all - please don't drop Cc-s when replying, unless you have a
specific reason to.

On 13.06.2023 11:56, nicola wrote:
> On 13/06/23 10:27, Jan Beulich wrote:
>> If you split this to 4 patches, leaving the URL proposal in just
>> the cover letter, then I think this one change (with the adjustments)
>> could go in right away. Similarly I expect the arm64/flushtlb.h
>> change could be ack-ed right away by an Arm maintainer.
> Ok. I do not understand what you mean by "leaving the URL proposal in 
> just the cover letter". Which URL?

In your description you had a proposal to deviate the // occurring
in URLs. The latest when splitting the patch, this doesn't belong
into any of the patches anymore, but just in the cover letter.

>> Here "propose" is appropriate in the description, as this is something 
>> the patch does not do. Further down, however, you mean to describe 
>> what the patch does, not what an eventual patch might do.
>>
> To my knowledge, there is not a standard format to define a project 
> deviation for a certain MISRA rule in Xen right now (this can also be 
> discussed in a separate thread). To clarify, I meant to describe why I 
> wasn't addressing these violations in the patch (they are the vast 
> majority, but they do not have any implication w.r.t. functional safety 
> and can therefore be safely deviated with an appropriate written 
> justification).

And as said, for what you're not doing in the patch, using "propose"
is quite fine (as per above, whether that actually belongs in the
description is another question). I view the word as inapplicable
though when you describe what you're actually doing in a patch. But
I'm not a native speaker, so I may be wrong here.

Jan

Re: [XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[nicola]

On 13/06/23 11:44, Julien Grall wrote:
> Hi,
>
> On 13/06/2023 09:27, Jan Beulich wrote:
>> On 13.06.2023 09:42, Nicola Vetrini wrote:
>>> The xen sources contain several violations of Rule 3.1 from MISRA 
>>> C:2012,
>>> whose headline states:
>>> "The character sequences '/*' and '//' shall not be used within a 
>>> comment".
>>>
>>> Most of the violations are due to the presence of links to webpages 
>>> within
>>> C-style comment blocks, such as:
>>>
>>> xen/arch/arm/include/asm/smccc.h:37.1-41.3
>>> /*
>>>   * This file provides common defines for ARM SMC Calling Convention as
>>>   * specified in
>>>   * 
>>> http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
>>>   */
>>>
>>> In this case, we propose to deviate all of these occurrences with a
>>> project deviation to be captured by a tool configuration.
>>>
>>> There are, however, a few other violations that do not fall under this
>>> category, namely:
>>>
>>> 1. in file "xen/arch/arm/include/asm/arm64/flushtlb.h" we propose to
>>> avoid the usage of a nested comment;
>>> 2. in file "xen/common/xmalloc_tlsf.c" we propose to substitute the
>>> commented-out if statement with a "#if 0 .. #endif;
>>> 3. in file "xen/include/xen/atomic.h" and
>>> "xen/drivers/passthrough/arm/smmu-v3.c" we propose to split the C-style
>>> comment containing the nested comment into two doxygen comments, 
>>> clearly
>>> identifying the second as a code sample. This can then be captured 
>>> with a
>>> project deviation by a tool configuration.
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> ---
>>> Changes:
>>> - Resending the patch with the right maintainers in CC.
>>
>> But without otherwise addressing comments already given, afaics. One 
>> more
>> remark:
>>
>>> --- a/xen/common/xmalloc_tlsf.c
>>> +++ b/xen/common/xmalloc_tlsf.c
>>> @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long 
>>> *r, int *fl, int *sl)
>>>           *fl = flsl(*r) - 1;
>>>           *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
>>>           *fl -= FLI_OFFSET;
>>> -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>>> -         *fl = *sl = 0;
>>> -         */
>>> +#if 0
>>> +        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>>> +        fl = *sl = 0;
>>
>> You want to get indentation right here, and you don't want to lose
>> the indirection on fl.
>>
>>> +#endif
>>>           *r &= ~t;
>>>       }
>>>   }
>>
>> If you split this to 4 patches, leaving the URL proposal in just
>> the cover letter, then I think this one change (with the adjustments)
>> could go in right away. Similarly I expect the arm64/flushtlb.h
>> change could be ack-ed right away by an Arm maintainer.
>
> I actually dislike the proposal. In this case, the code is meant to 
> look like assembly code. I would replace the // with ;. Also, I would 
> like to keep the comment style in sync in arm32/flushtlb.h. So can 
> this be updated as well?
>
> Cheers,
>
Hi, Julien.

I'm not authorized to send patches about files in the arm32 tree, but 
surely the patch can be easily replicated in any place where it makes 
sense for consistency.

Regards,

   Nicola

Re: [XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[Andrew Cooper]

On 13/06/2023 8:42 am, Nicola Vetrini wrote:
> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> index 75bdf18c4e..ea6ec47a59 100644
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl)
>          *fl = flsl(*r) - 1;
>          *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
>          *fl -= FLI_OFFSET;
> -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
> -         *fl = *sl = 0;
> -         */
> +#if 0
> +        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
> +        fl = *sl = 0;
> +#endif
>          *r &= ~t;
>      }
>  }

This logic has been commented out right from it's introduction in c/s
9736b76d829b2d in 2008, and never touched since.

I think it can safely be deleted, and not placed inside an #if 0.

~Andrew

Re: [XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[Jan Beulich]

On 14.06.2023 16:28, Andrew Cooper wrote:
> On 13/06/2023 8:42 am, Nicola Vetrini wrote:
>> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
>> index 75bdf18c4e..ea6ec47a59 100644
>> --- a/xen/common/xmalloc_tlsf.c
>> +++ b/xen/common/xmalloc_tlsf.c
>> @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl)
>>          *fl = flsl(*r) - 1;
>>          *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
>>          *fl -= FLI_OFFSET;
>> -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>> -         *fl = *sl = 0;
>> -         */
>> +#if 0
>> +        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>> +        fl = *sl = 0;
>> +#endif
>>          *r &= ~t;
>>      }
>>  }
> 
> This logic has been commented out right from it's introduction in c/s
> 9736b76d829b2d in 2008, and never touched since.
> 
> I think it can safely be deleted, and not placed inside an #if 0.

I have to admit that I wouldn't be happy with deleting without any
replacement. Instead of the commented out code, how about instead
having ASSERT(*fl >= 0)? (What isn't clear to me is whether the
commented out code is actually meant to replace the earlier line,
rather than (optionally) be there in addition - at least it very
much looks like so. With such an uncertainty I'd be further
inclined to not remove what's there.)

Jan