All of lore.kernel.org
 help / color / mirror / Atom feed
* [XEN PATCH v2 0/3] fix violations of MISRA C:2012 Rule 3.1
@ 2023-06-19  9:56 Nicola Vetrini
  2023-06-19  9:56 ` [XEN PATCH v2 1/3] xen/arch/arm: " Nicola Vetrini
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Nicola Vetrini @ 2023-06-19  9:56 UTC (permalink / raw)
  To: xen-devel
  Cc: michal.orzel, xenia.ragiadakou, ayan.kumar.halder, consulting,
	Nicola Vetrini, Stefano Stabellini, Julien Grall,
	Bertrand Marquis, Volodymyr Babchuk, Rahul Singh, Andrew Cooper,
	George Dunlap, Jan Beulich, Wei Liu

Hi all,

This patch series is about the violations present in the Xen sources of
Rule 3.1 from MISRA C:2012, whose headline states:
"The character sequences '/*' and '//' shall not be used within a comment".

In the context of the effort to bring xen into compliance w.r.t.
MISRA C:2012, and Rule 3.1 being already approved for the project (as
evidenced by `docs/misra/rules.rst'), these violations need to be fixed.

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, I propose to deviate all of these occurrences with a
project deviation to be captured by a tool configuration
(not included in any patch from this series).

There are, however, a few other violations that do not fall under this
category, which are the focus of the following set of patches. They either:

1. remove the nested '//' character sequence within a block comment;
2. remove the surrounding comment.

Thanks,
  Nicola

Nicola Vetrini (3):
  xen/arch/arm: fix violations of MISRA C:2012 Rule 3.1
  xen/drivers/passthrough/arm/smmu-v3.c: fix violations of MISRA C:2012
    Rule 3.1
  xen: fix violations of MISRA C:2012 Rule 3.1

 xen/arch/arm/include/asm/arm32/flushtlb.h | 8 ++++----
 xen/arch/arm/include/asm/arm64/flushtlb.h | 8 ++++----
 xen/common/xmalloc_tlsf.c                 | 3 ---
 xen/drivers/passthrough/arm/smmu-v3.c     | 4 ++--
 xen/include/xen/atomic.h                  | 2 +-
 5 files changed, 11 insertions(+), 14 deletions(-)

-- 
2.34.1



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [XEN PATCH v2 1/3] xen/arch/arm: fix violations of MISRA C:2012 Rule 3.1
  2023-06-19  9:56 [XEN PATCH v2 0/3] fix violations of MISRA C:2012 Rule 3.1 Nicola Vetrini
@ 2023-06-19  9:56 ` Nicola Vetrini
  2023-06-19 10:01   ` Julien Grall
  2023-06-19  9:56 ` [XEN PATCH v2 2/3] xen/drivers/passthrough/arm/smmu-v3.c: " Nicola Vetrini
  2023-06-19  9:56 ` [XEN PATCH v2 3/3] xen: " Nicola Vetrini
  2 siblings, 1 reply; 12+ messages in thread
From: Nicola Vetrini @ 2023-06-19  9:56 UTC (permalink / raw)
  To: xen-devel
  Cc: michal.orzel, xenia.ragiadakou, ayan.kumar.halder, consulting,
	Nicola Vetrini, Stefano Stabellini, Julien Grall,
	Bertrand Marquis, Volodymyr Babchuk

In the files `xen/arch/arm/include/asm/arm(32|64)/flushtlb.h' there are a
few occurrences of nested '//' character sequences inside C-style comment
blocks, which violate Rule 3.1. The patch aims to resolve those by removing
the nested comments.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com
---
Changes:
- Resending the patch with the right maintainers in CC.
Changes in V2:
- Split the patch into a series and reworked the fix.
- Apply the fix to the arm32 `flushtlb.h' file, for consistency
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/arm/include/asm/arm32/flushtlb.h | 8 ++++----
 xen/arch/arm/include/asm/arm64/flushtlb.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/include/asm/arm32/flushtlb.h b/xen/arch/arm/include/asm/arm32/flushtlb.h
index 22ee3b317b..bcbeac590b 100644
--- a/xen/arch/arm/include/asm/arm32/flushtlb.h
+++ b/xen/arch/arm/include/asm/arm32/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
  *
  * For Xen page-tables the ISB will discard any instructions fetched
  * from the old mappings.
diff --git a/xen/arch/arm/include/asm/arm64/flushtlb.h b/xen/arch/arm/include/asm/arm64/flushtlb.h
index 56c6fc763b..6066a2d703 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
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [XEN PATCH v2 2/3] xen/drivers/passthrough/arm/smmu-v3.c: fix violations of MISRA C:2012 Rule 3.1
  2023-06-19  9:56 [XEN PATCH v2 0/3] fix violations of MISRA C:2012 Rule 3.1 Nicola Vetrini
  2023-06-19  9:56 ` [XEN PATCH v2 1/3] xen/arch/arm: " Nicola Vetrini
@ 2023-06-19  9:56 ` Nicola Vetrini
  2023-06-19 10:10   ` Julien Grall
  2023-06-19  9:56 ` [XEN PATCH v2 3/3] xen: " Nicola Vetrini
  2 siblings, 1 reply; 12+ messages in thread
From: Nicola Vetrini @ 2023-06-19  9:56 UTC (permalink / raw)
  To: xen-devel
  Cc: michal.orzel, xenia.ragiadakou, ayan.kumar.halder, consulting,
	Nicola Vetrini, Bertrand Marquis, Rahul Singh,
	Stefano Stabellini, Julien Grall, Volodymyr Babchuk

In the file `xen/drivers/passthrough/arm/smmu-v3.c' there are a few occurrences
of nested '//' character sequences inside C-style comment blocks, which violate
Rule 3.1. The patch aims to resolve those by removing the nested comments.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com
Changes:
- Resending the patch with the right maintainers in CC.
Changes in V2:
- Split the patch into a series and reworked the fix.
- Apply the fix to the arm32 `flushtlb.h' file, for consistency
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/drivers/passthrough/arm/smmu-v3.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index 720aa69ff2..f410863e10 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -1047,10 +1047,10 @@ static int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
 	 * before we read 'nr_ats_masters' in case of a concurrent call to
 	 * arm_smmu_enable_ats():
 	 *
-	 *	// unmap()			// arm_smmu_enable_ats()
+	 *	unmap()				arm_smmu_enable_ats()
 	 *	TLBI+SYNC			atomic_inc(&nr_ats_masters);
 	 *	smp_mb();			[...]
-	 *	atomic_read(&nr_ats_masters);	pci_enable_ats() // writel()
+	 *	atomic_read(&nr_ats_masters);	pci_enable_ats() (i.e. 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.
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [XEN PATCH v2 3/3] xen: fix violations of MISRA C:2012 Rule 3.1
  2023-06-19  9:56 [XEN PATCH v2 0/3] fix violations of MISRA C:2012 Rule 3.1 Nicola Vetrini
  2023-06-19  9:56 ` [XEN PATCH v2 1/3] xen/arch/arm: " Nicola Vetrini
  2023-06-19  9:56 ` [XEN PATCH v2 2/3] xen/drivers/passthrough/arm/smmu-v3.c: " Nicola Vetrini
@ 2023-06-19  9:56 ` Nicola Vetrini
  2023-06-19 10:29   ` Jan Beulich
  2 siblings, 1 reply; 12+ messages in thread
From: Nicola Vetrini @ 2023-06-19  9:56 UTC (permalink / raw)
  To: xen-devel
  Cc: michal.orzel, xenia.ragiadakou, ayan.kumar.halder, consulting,
	Nicola Vetrini, Andrew Cooper, George Dunlap, Jan Beulich,
	Julien Grall, Stefano Stabellini, Wei Liu

In the files modified by this patch there are a few occurrences of nested '//'
character sequences inside C-style comment blocks, which violate Rule 3.1.
The patch aims to resolve those by removing the nested comments.

In the file `xen/common/xmalloc_tlsf.c' the comment has been deleted,
following the suggestion of a review comment.

In the file `xen/include/xen/atomic.h' the nested comment has been removed,
since the code sample is already explained by the preceding comment.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com
Changes:
- Resending the patch with the right maintainers in CC.
Changes in V2:
- Split the patch into a series and reworked the fix.
- Apply the fix to the arm32 `flushtlb.h' file, for consistency
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/common/xmalloc_tlsf.c | 3 ---
 xen/include/xen/atomic.h  | 2 +-
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
index 75bdf18c4e..4f9f60a39d 100644
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -140,9 +140,6 @@ 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;
-         */
         *r &= ~t;
     }
 }
diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h
index 529213ebbb..fa750a18ae 100644
--- a/xen/include/xen/atomic.h
+++ b/xen/include/xen/atomic.h
@@ -78,7 +78,7 @@ static inline void _atomic_set(atomic_t *v, int i);
  *      int old = atomic_read(&v);
  *      int new = old + 1;
  *      if ( likely(old == atomic_cmpxchg(&v, old, new)) )
- *          break; // success!
+ *          break;
  *  }
  */
 static inline int atomic_cmpxchg(atomic_t *v, int old, int new);
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [XEN PATCH v2 1/3] xen/arch/arm: fix violations of MISRA C:2012 Rule 3.1
  2023-06-19  9:56 ` [XEN PATCH v2 1/3] xen/arch/arm: " Nicola Vetrini
@ 2023-06-19 10:01   ` Julien Grall
  2023-06-19 10:25     ` Jan Beulich
  0 siblings, 1 reply; 12+ messages in thread
From: Julien Grall @ 2023-06-19 10:01 UTC (permalink / raw)
  To: Nicola Vetrini, xen-devel
  Cc: michal.orzel, xenia.ragiadakou, ayan.kumar.halder, consulting,
	Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk

Hi,

On 19/06/2023 10:56, Nicola Vetrini wrote:
> In the files `xen/arch/arm/include/asm/arm(32|64)/flushtlb.h' there are a
> few occurrences of nested '//' character sequences inside C-style comment
> blocks, which violate Rule 3.1. The patch aims to resolve those by removing
> the nested comments.

As I wrote in 
https://lore.kernel.org/xen-devel/f3fc1848-68ca-37a1-add2-e100b4773190@xen.org/, 
I am against replacing '//' with nothing. I have proposed to use ';' 
because this is also a valid way to comment in assembly.

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

There is a missing '>' at the end of the list.

> ---
> Changes:
> - Resending the patch with the right maintainers in CC.
> Changes in V2:
> - Split the patch into a series and reworked the fix.
> - Apply the fix to the arm32 `flushtlb.h' file, for consistency
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>   xen/arch/arm/include/asm/arm32/flushtlb.h | 8 ++++----
>   xen/arch/arm/include/asm/arm64/flushtlb.h | 8 ++++----
>   2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/arm32/flushtlb.h b/xen/arch/arm/include/asm/arm32/flushtlb.h
> index 22ee3b317b..bcbeac590b 100644
> --- a/xen/arch/arm/include/asm/arm32/flushtlb.h
> +++ b/xen/arch/arm/include/asm/arm32/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
>    *
>    * For Xen page-tables the ISB will discard any instructions fetched
>    * from the old mappings.
> diff --git a/xen/arch/arm/include/asm/arm64/flushtlb.h b/xen/arch/arm/include/asm/arm64/flushtlb.h
> index 56c6fc763b..6066a2d703 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

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [XEN PATCH v2 2/3] xen/drivers/passthrough/arm/smmu-v3.c: fix violations of MISRA C:2012 Rule 3.1
  2023-06-19  9:56 ` [XEN PATCH v2 2/3] xen/drivers/passthrough/arm/smmu-v3.c: " Nicola Vetrini
@ 2023-06-19 10:10   ` Julien Grall
  2023-06-20  8:06     ` Nicola Vetrini
  0 siblings, 1 reply; 12+ messages in thread
From: Julien Grall @ 2023-06-19 10:10 UTC (permalink / raw)
  To: Nicola Vetrini, xen-devel
  Cc: michal.orzel, xenia.ragiadakou, ayan.kumar.halder, consulting,
	Bertrand Marquis, Rahul Singh, Stefano Stabellini,
	Volodymyr Babchuk

Hi,

On 19/06/2023 10:56, Nicola Vetrini wrote:
> In the file `xen/drivers/passthrough/arm/smmu-v3.c' there are a few occurrences
> of nested '//' character sequences inside C-style comment blocks, which violate
> Rule 3.1. The patch aims to resolve those by removing the nested comments.

I think it is important to understand/explain what was the intention of 
the // before removing them because, IMHO, the new approach doesn't 
convey the same information. Before...

> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com
> Changes:
> - Resending the patch with the right maintainers in CC.
> Changes in V2:
> - Split the patch into a series and reworked the fix.
> - Apply the fix to the arm32 `flushtlb.h' file, for consistency
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>   xen/drivers/passthrough/arm/smmu-v3.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
> index 720aa69ff2..f410863e10 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -1047,10 +1047,10 @@ static int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
>   	 * before we read 'nr_ats_masters' in case of a concurrent call to
>   	 * arm_smmu_enable_ats():
>   	 *
> -	 *	// unmap()			// arm_smmu_enable_ats()
> +	 *	unmap()				arm_smmu_enable_ats()

... with the // it would be clearer that the code below belongs to each 
function. But now, it leads to think there are a call to 'unmap' which 
it then followed by TLBI+SYNC.

In this case, I would propose to use --- <function> ---

>   	 *	TLBI+SYNC			atomic_inc(&nr_ats_masters);
>   	 *	smp_mb();			[...]
> -	 *	atomic_read(&nr_ats_masters);	pci_enable_ats() // writel()
> +	 *	atomic_read(&nr_ats_masters);	pci_enable_ats() (i.e. writel())

NIT: I think 'see' would be better than 'i.e.' because I read it as 
pci_enable_ats() is a simple 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.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [XEN PATCH v2 1/3] xen/arch/arm: fix violations of MISRA C:2012 Rule 3.1
  2023-06-19 10:01   ` Julien Grall
@ 2023-06-19 10:25     ` Jan Beulich
  2023-06-19 10:29       ` Julien Grall
  0 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2023-06-19 10:25 UTC (permalink / raw)
  To: Julien Grall
  Cc: michal.orzel, xenia.ragiadakou, ayan.kumar.halder, consulting,
	Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
	Nicola Vetrini, xen-devel

On 19.06.2023 12:01, Julien Grall wrote:
> On 19/06/2023 10:56, Nicola Vetrini wrote:
>> In the files `xen/arch/arm/include/asm/arm(32|64)/flushtlb.h' there are a
>> few occurrences of nested '//' character sequences inside C-style comment
>> blocks, which violate Rule 3.1. The patch aims to resolve those by removing
>> the nested comments.
> 
> As I wrote in 
> https://lore.kernel.org/xen-devel/f3fc1848-68ca-37a1-add2-e100b4773190@xen.org/, 
> I am against replacing '//' with nothing. I have proposed to use ';' 
> because this is also a valid way to comment in assembly.

Are you sure about this? For gas most targets use ; as a statement separator,
not as a comment character. Afaics arm-* and aarch64-* are no exception there.

Jan


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [XEN PATCH v2 1/3] xen/arch/arm: fix violations of MISRA C:2012 Rule 3.1
  2023-06-19 10:25     ` Jan Beulich
@ 2023-06-19 10:29       ` Julien Grall
  2023-06-20 13:56         ` Nicola Vetrini
  0 siblings, 1 reply; 12+ messages in thread
From: Julien Grall @ 2023-06-19 10:29 UTC (permalink / raw)
  To: Jan Beulich
  Cc: michal.orzel, xenia.ragiadakou, ayan.kumar.halder, consulting,
	Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
	Nicola Vetrini, xen-devel

Hi,

On 19/06/2023 11:25, Jan Beulich wrote:
> On 19.06.2023 12:01, Julien Grall wrote:
>> On 19/06/2023 10:56, Nicola Vetrini wrote:
>>> In the files `xen/arch/arm/include/asm/arm(32|64)/flushtlb.h' there are a
>>> few occurrences of nested '//' character sequences inside C-style comment
>>> blocks, which violate Rule 3.1. The patch aims to resolve those by removing
>>> the nested comments.
>>
>> As I wrote in
>> https://lore.kernel.org/xen-devel/f3fc1848-68ca-37a1-add2-e100b4773190@xen.org/,
>> I am against replacing '//' with nothing. I have proposed to use ';'
>> because this is also a valid way to comment in assembly.
> 
> Are you sure about this? For gas most targets use ; as a statement separator,
> not as a comment character. Afaics arm-* and aarch64-* are no exception there.

GAS will not accept it. But the Arm compiler will [1]. This is good 
enough for me because I want to have a separator between the instruction 
and the comment.

Cheers,

[1] 
https://developer.arm.com/documentation/dui0473/m/structure-of-assembly-language-modules/syntax-of-source-lines-in-assembly-language

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [XEN PATCH v2 3/3] xen: fix violations of MISRA C:2012 Rule 3.1
  2023-06-19  9:56 ` [XEN PATCH v2 3/3] xen: " Nicola Vetrini
@ 2023-06-19 10:29   ` Jan Beulich
  2023-06-20  8:00     ` Nicola Vetrini
  0 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2023-06-19 10:29 UTC (permalink / raw)
  To: Nicola Vetrini
  Cc: michal.orzel, xenia.ragiadakou, ayan.kumar.halder, consulting,
	Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
	Wei Liu, xen-devel

On 19.06.2023 11:56, Nicola Vetrini wrote:
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -140,9 +140,6 @@ 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;
> -         */
>          *r &= ~t;
>      }
>  }

As indicated before, I don't think simply dropping the commented out code
is appropriate here. Personally I'd prefer if it was kept (using #if/#else),
but I'd also be okay with replacing it by a respective assertion. That said,
if other maintainers think this is the way to go, then I don't mean to
stand in the way.

Jan


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [XEN PATCH v2 3/3] xen: fix violations of MISRA C:2012 Rule 3.1
  2023-06-19 10:29   ` Jan Beulich
@ 2023-06-20  8:00     ` Nicola Vetrini
  0 siblings, 0 replies; 12+ messages in thread
From: Nicola Vetrini @ 2023-06-20  8:00 UTC (permalink / raw)
  To: Jan Beulich
  Cc: michal.orzel, xenia.ragiadakou, ayan.kumar.halder, consulting,
	Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
	Wei Liu, xen-devel



On 19/06/23 12:29, Jan Beulich wrote:
> On 19.06.2023 11:56, Nicola Vetrini wrote:
>> --- a/xen/common/xmalloc_tlsf.c
>> +++ b/xen/common/xmalloc_tlsf.c
>> @@ -140,9 +140,6 @@ 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;
>> -         */
>>           *r &= ~t;
>>       }
>>   }
> 
> As indicated before, I don't think simply dropping the commented out code
> is appropriate here. Personally I'd prefer if it was kept (using #if/#else),
> but I'd also be okay with replacing it by a respective assertion. That said,
> if other maintainers think this is the way to go, then I don't mean to
> stand in the way.
> 

As Andrew Cooper suggested in the previous patch revision 
(https://lore.kernel.org/xen-devel/6bac57d5-c30e-f763-3abe-b3f335f366f7@suse.com/T/#m5722285215bb30d7f1202b9921e2c92d5ea98d6a), 
I removed the commented-out code, since it contains unused logic, but I 
would be okay with replacing it with an assertion, if you think it's better.

Regards,

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [XEN PATCH v2 2/3] xen/drivers/passthrough/arm/smmu-v3.c: fix violations of MISRA C:2012 Rule 3.1
  2023-06-19 10:10   ` Julien Grall
@ 2023-06-20  8:06     ` Nicola Vetrini
  0 siblings, 0 replies; 12+ messages in thread
From: Nicola Vetrini @ 2023-06-20  8:06 UTC (permalink / raw)
  To: Julien Grall, xen-devel
  Cc: michal.orzel, xenia.ragiadakou, ayan.kumar.halder, consulting,
	Bertrand Marquis, Rahul Singh, Stefano Stabellini,
	Volodymyr Babchuk



On 19/06/23 12:10, Julien Grall wrote:
> Hi,
> 
> On 19/06/2023 10:56, Nicola Vetrini wrote:
>> In the file `xen/drivers/passthrough/arm/smmu-v3.c' there are a few 
>> occurrences
>> of nested '//' character sequences inside C-style comment blocks, 
>> which violate
>> Rule 3.1. The patch aims to resolve those by removing the nested 
>> comments.
> 
> I think it is important to understand/explain what was the intention of 
> the // before removing them because, IMHO, the new approach doesn't 
> convey the same information. Before...
> 
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com
>> Changes:
>> - Resending the patch with the right maintainers in CC.
>> Changes in V2:
>> - Split the patch into a series and reworked the fix.
>> - Apply the fix to the arm32 `flushtlb.h' file, for consistency
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>   xen/drivers/passthrough/arm/smmu-v3.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
>> b/xen/drivers/passthrough/arm/smmu-v3.c
>> index 720aa69ff2..f410863e10 100644
>> --- a/xen/drivers/passthrough/arm/smmu-v3.c
>> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
>> @@ -1047,10 +1047,10 @@ static int arm_smmu_atc_inv_domain(struct 
>> arm_smmu_domain *smmu_domain,
>>        * before we read 'nr_ats_masters' in case of a concurrent call to
>>        * arm_smmu_enable_ats():
>>        *
>> -     *    // unmap()            // arm_smmu_enable_ats()
>> +     *    unmap()                arm_smmu_enable_ats()
> 
> ... with the // it would be clearer that the code below belongs to each 
> function. But now, it leads to think there are a call to 'unmap' which 
> it then followed by TLBI+SYNC.
> 
> In this case, I would propose to use --- <function> ---

It seems a good suggestion to me.

> 
>>        *    TLBI+SYNC            atomic_inc(&nr_ats_masters);
>>        *    smp_mb();            [...]
>> -     *    atomic_read(&nr_ats_masters);    pci_enable_ats() // writel()
>> +     *    atomic_read(&nr_ats_masters);    pci_enable_ats() (i.e. 
>> writel())
> 
> NIT: I think 'see' would be better than 'i.e.' because I read it as 
> pci_enable_ats() is a simple writel().

Ok.

> 
>>        *
>>        * Ensures that we always see the incremented 'nr_ats_masters' 
>> count if
>>        * ATS was enabled at the PCI device before completion of the TLBI.
> 
> Cheers,
> 

Regards,

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [XEN PATCH v2 1/3] xen/arch/arm: fix violations of MISRA C:2012 Rule 3.1
  2023-06-19 10:29       ` Julien Grall
@ 2023-06-20 13:56         ` Nicola Vetrini
  0 siblings, 0 replies; 12+ messages in thread
From: Nicola Vetrini @ 2023-06-20 13:56 UTC (permalink / raw)
  To: Julien Grall, Jan Beulich
  Cc: michal.orzel, xenia.ragiadakou, ayan.kumar.halder, consulting,
	Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
	xen-devel

Hi,

On 19/06/23 12:29, Julien Grall wrote:
> Hi,
> 
> On 19/06/2023 11:25, Jan Beulich wrote:
>> On 19.06.2023 12:01, Julien Grall wrote:
>>> On 19/06/2023 10:56, Nicola Vetrini wrote:
>>>> In the files `xen/arch/arm/include/asm/arm(32|64)/flushtlb.h' there 
>>>> are a
>>>> few occurrences of nested '//' character sequences inside C-style 
>>>> comment
>>>> blocks, which violate Rule 3.1. The patch aims to resolve those by 
>>>> removing
>>>> the nested comments.
>>>
>>> As I wrote in
>>> https://lore.kernel.org/xen-devel/f3fc1848-68ca-37a1-add2-e100b4773190@xen.org/,
>>> I am against replacing '//' with nothing. I have proposed to use ';'
>>> because this is also a valid way to comment in assembly.
>>
>> Are you sure about this? For gas most targets use ; as a statement 
>> separator,
>> not as a comment character. Afaics arm-* and aarch64-* are no 
>> exception there.
> 
> GAS will not accept it. But the Arm compiler will [1]. This is good 
> enough for me because I want to have a separator between the instruction 
> and the comment.
> 
> Cheers,
> 
> [1] 
> https://developer.arm.com/documentation/dui0473/m/structure-of-assembly-language-modules/syntax-of-source-lines-in-assembly-language
> 

If no one has any objection about this I'll add the ';' comment delimiter.

Regards,

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-06-20 13:56 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-19  9:56 [XEN PATCH v2 0/3] fix violations of MISRA C:2012 Rule 3.1 Nicola Vetrini
2023-06-19  9:56 ` [XEN PATCH v2 1/3] xen/arch/arm: " Nicola Vetrini
2023-06-19 10:01   ` Julien Grall
2023-06-19 10:25     ` Jan Beulich
2023-06-19 10:29       ` Julien Grall
2023-06-20 13:56         ` Nicola Vetrini
2023-06-19  9:56 ` [XEN PATCH v2 2/3] xen/drivers/passthrough/arm/smmu-v3.c: " Nicola Vetrini
2023-06-19 10:10   ` Julien Grall
2023-06-20  8:06     ` Nicola Vetrini
2023-06-19  9:56 ` [XEN PATCH v2 3/3] xen: " Nicola Vetrini
2023-06-19 10:29   ` Jan Beulich
2023-06-20  8:00     ` Nicola Vetrini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.