[XEN PATCH 0/7] xen/arm: address violations of MISRA C:2012 Rule 16.3

[XEN PATCH 0/7] xen/arm: address violations of MISRA C:2012 Rule 16.3

[Federico Serafini]

This patch series addresses violations of MISRA C:2012 Rule 16.3 on the Arm
code. No fucntional changes are introduced.

Federico Serafini (7):
  xen/arm: gic-v3: address violations of MISRA C:2012 Rule 16.3
  xen/arm: traps: address violations of MISRA C:2012 Rule 16.3
  xen/arm: guest_walk: address violations of MISRA C:2012 Rule 16.3
  xen/arm: mem_access: address violations of MISRA C:2012 Rule 16.3
  xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3
  xen/arm: mmu: address a violations of MISRA C:2012 Rule 16.3
  xen/arm: smmu-v3: address violations of MISRA C:2012 Rule 16.3

 xen/arch/arm/arm64/vsysreg.c          |  4 ++--
 xen/arch/arm/gic-v3.c                 | 30 +++++++++++++++++++++++++++
 xen/arch/arm/guest_walk.c             |  4 ++++
 xen/arch/arm/mem_access.c             | 12 +++++------
 xen/arch/arm/mmu/p2m.c                |  1 +
 xen/arch/arm/traps.c                  | 18 ++++++++++++----
 xen/arch/arm/vcpreg.c                 |  4 ++--
 xen/drivers/passthrough/arm/smmu-v3.c |  2 ++
 8 files changed, 61 insertions(+), 14 deletions(-)

-- 
2.34.1

[XEN PATCH 1/7] xen/arm: gic-v3: address violations of MISRA C:2012 Rule 16.3

[Federico Serafini]

Add the pseudo-keyword fallthrough to meet the requirements to
deviate Rule 16.3 ("An unconditional `break' statement shall terminate
every switch-clause").
No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/arm/gic-v3.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 18289cd645..bf0e5c1b75 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -65,34 +65,49 @@ static inline void gicv3_save_lrs(struct vcpu *v)
     {
     case 16:
         v->arch.gic.v3.lr[15] = READ_SYSREG_LR(15);
+        fallthrough;
     case 15:
         v->arch.gic.v3.lr[14] = READ_SYSREG_LR(14);
+        fallthrough;
     case 14:
         v->arch.gic.v3.lr[13] = READ_SYSREG_LR(13);
+        fallthrough;
     case 13:
         v->arch.gic.v3.lr[12] = READ_SYSREG_LR(12);
+        fallthrough;
     case 12:
         v->arch.gic.v3.lr[11] = READ_SYSREG_LR(11);
+        fallthrough;
     case 11:
         v->arch.gic.v3.lr[10] = READ_SYSREG_LR(10);
+        fallthrough;
     case 10:
         v->arch.gic.v3.lr[9] = READ_SYSREG_LR(9);
+        fallthrough;
     case 9:
         v->arch.gic.v3.lr[8] = READ_SYSREG_LR(8);
+        fallthrough;
     case 8:
         v->arch.gic.v3.lr[7] = READ_SYSREG_LR(7);
+        fallthrough;
     case 7:
         v->arch.gic.v3.lr[6] = READ_SYSREG_LR(6);
+        fallthrough;
     case 6:
         v->arch.gic.v3.lr[5] = READ_SYSREG_LR(5);
+        fallthrough;
     case 5:
         v->arch.gic.v3.lr[4] = READ_SYSREG_LR(4);
+        fallthrough;
     case 4:
         v->arch.gic.v3.lr[3] = READ_SYSREG_LR(3);
+        fallthrough;
     case 3:
         v->arch.gic.v3.lr[2] = READ_SYSREG_LR(2);
+        fallthrough;
     case 2:
         v->arch.gic.v3.lr[1] = READ_SYSREG_LR(1);
+        fallthrough;
     case 1:
          v->arch.gic.v3.lr[0] = READ_SYSREG_LR(0);
          break;
@@ -112,34 +127,49 @@ static inline void gicv3_restore_lrs(const struct vcpu *v)
     {
     case 16:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[15], 15);
+        fallthrough;
     case 15:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[14], 14);
+        fallthrough;
     case 14:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[13], 13);
+        fallthrough;
     case 13:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[12], 12);
+        fallthrough;
     case 12:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[11], 11);
+        fallthrough;
     case 11:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[10], 10);
+        fallthrough;
     case 10:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[9], 9);
+        fallthrough;
     case 9:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[8], 8);
+        fallthrough;
     case 8:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[7], 7);
+        fallthrough;
     case 7:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[6], 6);
+        fallthrough;
     case 6:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[5], 5);
+        fallthrough;
     case 5:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[4], 4);
+        fallthrough;
     case 4:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[3], 3);
+        fallthrough;
     case 3:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[2], 2);
+        fallthrough;
     case 2:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[1], 1);
+        fallthrough;
     case 1:
         WRITE_SYSREG_LR(v->arch.gic.v3.lr[0], 0);
         break;
-- 
2.34.1

[XEN PATCH 2/7] xen/arm: traps: address violations of MISRA C:2012 Rule 16.3

[Federico Serafini]

MISRA C:2012 Rule 16.3 states: "An unconditional `break' statement
shall terminate every switch-clause".

Add missing break statements to address violations of Rule 16.3.
Add pseudo-keyword "fallthrough" to make explicit the intended
behavior and satisfy the requirements to deviate Rule 16.3.
No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/arm/traps.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 77220ba092..9c10e8f78c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1414,10 +1414,18 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
     {
         /* Deliberately corrupt parameter regs used by this hypercall. */
         switch ( hypercall_args[*nr] ) {
-        case 5: HYPERCALL_ARG5(regs) = 0xDEADBEEFU;
-        case 4: HYPERCALL_ARG4(regs) = 0xDEADBEEFU;
-        case 3: HYPERCALL_ARG3(regs) = 0xDEADBEEFU;
-        case 2: HYPERCALL_ARG2(regs) = 0xDEADBEEFU;
+        case 5:
+            HYPERCALL_ARG5(regs) = 0xDEADBEEFU;
+            fallthrough;
+        case 4:
+            HYPERCALL_ARG4(regs) = 0xDEADBEEFU;
+            fallthrough;
+        case 3:
+            HYPERCALL_ARG3(regs) = 0xDEADBEEFU;
+            fallthrough;
+        case 2:
+            HYPERCALL_ARG2(regs) = 0xDEADBEEFU;
+            fallthrough;
         case 1: /* Don't clobber x0/r0 -- it's the return value */
         case 0: /* -ENOSYS case */
             break;
@@ -1929,6 +1937,7 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
         gprintk(XENLOG_WARNING,
                 "Unsupported FSC: HSR=%#"PRIregister" DFSC=%#x\n",
                 hsr.bits, xabt.fsc);
+        break;
     }
 
 inject_abt:
@@ -2136,6 +2145,7 @@ void asmlinkage do_trap_guest_sync(struct cpu_user_regs *regs)
                 "Unknown Guest Trap. HSR=%#"PRIregister" EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
                 hsr.bits, hsr.ec, hsr.len, hsr.iss);
         inject_undef_exception(regs, hsr);
+        break;
     }
 }
 
-- 
2.34.1

[XEN PATCH 3/7] xen/arm: guest_walk: address violations of MISRA C:2012 Rule 16.3

[Federico Serafini]

Add missing break statements to address violations of Rule 16.3
("An unconditional `break' statement shall terminate every
switch-clause").
No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/arm/guest_walk.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index d99b411f3b..2f7d3a0da7 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -165,6 +165,8 @@ static bool guest_walk_sd(const struct vcpu *v,
             *perms |= GV2M_WRITE;
         if ( !pte.sec.xn )
             *perms |= GV2M_EXEC;
+
+        break;
     }
 
     return true;
@@ -260,6 +262,7 @@ static bool get_ttbr_and_gran_64bit(uint64_t *ttbr, unsigned int *gran,
              * fall back to 4K by default.
              */
             *gran = GRANULE_SIZE_INDEX_4K;
+            break;
         }
 
         /* Use TTBR0 for GVA to IPA translation. */
@@ -291,6 +294,7 @@ static bool get_ttbr_and_gran_64bit(uint64_t *ttbr, unsigned int *gran,
              * fall back to 4K by default.
              */
             *gran = GRANULE_SIZE_INDEX_4K;
+            break;
         }
 
         /* Use TTBR1 for GVA to IPA translation. */
-- 
2.34.1

[XEN PATCH 4/7] xen/arm: mem_access: address violations of MISRA C:2012 Rule 16.3

[Federico Serafini]

Refactor of the code to have a break statement at the end of the
switch-clause. This addresses violations of Rule 16.3
("An unconditional `break' statement shall terminate every
switch-clause").
No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/arm/mem_access.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
index 31db846354..fbcb5471f7 100644
--- a/xen/arch/arm/mem_access.c
+++ b/xen/arch/arm/mem_access.c
@@ -168,10 +168,10 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
          * If this was a read then it was because of mem_access, but if it was
          * a write then the original get_page_from_gva fault was correct.
          */
-        if ( flag == GV2M_READ )
-            break;
-        else
+        if ( flag != GV2M_READ )
             goto err;
+
+        break;
     case XENMEM_access_rx2rw:
     case XENMEM_access_rx:
     case XENMEM_access_r:
@@ -179,10 +179,10 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
          * If this was a write then it was because of mem_access, but if it was
          * a read then the original get_page_from_gva fault was correct.
          */
-        if ( flag == GV2M_WRITE )
-            break;
-        else
+        if ( flag != GV2M_WRITE )
             goto err;
+
+        break;
     }
 
     /*
-- 
2.34.1

[XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3

[Federico Serafini]

Refactor of the switch-clauses to have a return statement at the end.
This satisfies the requirements to deviate Rule 16.3 ("An unconditional
`break' statement shall terminate every switch-clause).
No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/arm/arm64/vsysreg.c | 4 ++--
 xen/arch/arm/vcpreg.c        | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index b5d54c569b..247f08ad8d 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs,
         /* RO at EL0. RAZ/WI at EL1 */
         if ( regs_mode_is_user(regs) )
             return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
-        else
-            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
+
+        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
     case HSR_SYSREG_PMCR_EL0:
     case HSR_SYSREG_PMCNTENSET_EL0:
     case HSR_SYSREG_PMCNTENCLR_EL0:
diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index a2d0500704..685609f825 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -289,8 +289,8 @@ void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
         /* RO at EL0. RAZ/WI at EL1 */
         if ( regs_mode_is_user(regs) )
             return handle_ro_raz(regs, regidx, cp32.read, hsr, 0);
-        else
-            return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
+
+        return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
     case HSR_CPREG32(PMINTENSET):
     case HSR_CPREG32(PMINTENCLR):
         /* EL1 only, however MDCR_EL2.TPM==1 means EL0 may trap here also. */
-- 
2.34.1

[XEN PATCH 6/7] xen/arm: mmu: address a violations of MISRA C:2012 Rule 16.3

[Federico Serafini]

Add missing break at the end of the switch-clause to address a
violation of MISRA C:2012 Rule 16.3 ("An unconditional `break'
statement shall terminate every switch-clause").
No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/arm/mmu/p2m.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
index 6a5a080307..41fcca011c 100644
--- a/xen/arch/arm/mmu/p2m.c
+++ b/xen/arch/arm/mmu/p2m.c
@@ -657,6 +657,7 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, p2m_access_t a)
     default:
         e.p2m.mattr = MATTR_MEM;
         e.p2m.sh = LPAE_SH_INNER;
+        break;
     }
 
     p2m_set_permission(&e, t, a);
-- 
2.34.1

[XEN PATCH 7/7] xen/arm: smmu-v3: address violations of MISRA C:2012 Rule 16.3

[Federico Serafini]

Add a break statement at the end of the switch-clauses to address
violations of MISRA C:2012 Rule 16.3 ("An unconditional `break'
statement shall terminate every switch-clause").
No funtional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/drivers/passthrough/arm/smmu-v3.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index cdbb505134..bca5866b35 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -2200,6 +2200,7 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
 		fallthrough;
 	case IDR0_STALL_MODEL_STALL:
 		smmu->features |= ARM_SMMU_FEAT_STALLS;
+		break;
 	}
 
 	if (reg & IDR0_S2P)
@@ -2301,6 +2302,7 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
 		fallthrough;
 	case IDR5_OAS_48_BIT:
 		smmu->oas = 48;
+		break;
 	}
 
 	smmu->oas = min_t(unsigned long, PADDR_BITS, smmu->oas);
-- 
2.34.1

Re: [XEN PATCH 1/7] xen/arm: gic-v3: address violations of MISRA C:2012 Rule 16.3

[Julien Grall]

Hi,

On 20/12/2023 11:03, Federico Serafini wrote:
> Add the pseudo-keyword fallthrough to meet the requirements to
> deviate Rule 16.3 ("An unconditional `break' statement shall terminate
> every switch-clause").
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 2/7] xen/arm: traps: address violations of MISRA C:2012 Rule 16.3

[Julien Grall]

Hi,

On 20/12/2023 11:03, Federico Serafini wrote:
> MISRA C:2012 Rule 16.3 states: "An unconditional `break' statement
> shall terminate every switch-clause".
> 
> Add missing break statements to address violations of Rule 16.3.
> Add pseudo-keyword "fallthrough" to make explicit the intended
> behavior and satisfy the requirements to deviate Rule 16.3.
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 3/7] xen/arm: guest_walk: address violations of MISRA C:2012 Rule 16.3

[Julien Grall]

Hi,

On 20/12/2023 11:03, Federico Serafini wrote:
> Add missing break statements to address violations of Rule 16.3
> ("An unconditional `break' statement shall terminate every
> switch-clause").
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3

[Julien Grall]

Hi Federico,

On 20/12/2023 11:03, Federico Serafini wrote:
> Refactor of the switch-clauses to have a return statement at the end.
> This satisfies the requirements to deviate Rule 16.3 ("An unconditional
> `break' statement shall terminate every switch-clause).
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
>   xen/arch/arm/arm64/vsysreg.c | 4 ++--
>   xen/arch/arm/vcpreg.c        | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index b5d54c569b..247f08ad8d 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs,
>           /* RO at EL0. RAZ/WI at EL1 */
>           if ( regs_mode_is_user(regs) )
>               return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
> -        else
> -            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
> +
> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);

I don't 100% like this change (mostly because I find if/else clearer 
here). But I have the feeling any other solution would probably be 
worse. So:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 7/7] xen/arm: smmu-v3: address violations of MISRA C:2012 Rule 16.3

[Julien Grall]

Hi Federico,

On 20/12/2023 11:03, Federico Serafini wrote:
> Add a break statement at the end of the switch-clauses to address
> violations of MISRA C:2012 Rule 16.3 ("An unconditional `break'
> statement shall terminate every switch-clause").
> No funtional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 6/7] xen/arm: mmu: address a violations of MISRA C:2012 Rule 16.3

[Julien Grall]

Hi Federico,

On 20/12/2023 11:03, Federico Serafini wrote:
> Add missing break at the end of the switch-clause to address a
> violation of MISRA C:2012 Rule 16.3 ("An unconditional `break'
> statement shall terminate every switch-clause").
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 4/7] xen/arm: mem_access: address violations of MISRA C:2012 Rule 16.3

[Julien Grall]

Hi Federico,

On 20/12/2023 11:03, Federico Serafini wrote:
> Refactor of the code to have a break statement at the end of the
> switch-clause. This addresses violations of Rule 16.3
> ("An unconditional `break' statement shall terminate every
> switch-clause").
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
>   xen/arch/arm/mem_access.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> index 31db846354..fbcb5471f7 100644
> --- a/xen/arch/arm/mem_access.c
> +++ b/xen/arch/arm/mem_access.c
> @@ -168,10 +168,10 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
>            * If this was a read then it was because of mem_access, but if it was
>            * a write then the original get_page_from_gva fault was correct.
>            */
> -        if ( flag == GV2M_READ )
> -            break;
> -        else
> +        if ( flag != GV2M_READ )
>               goto err;
> +
> +        break;

On both hunks, I find the original version better as it is easier to 
match with the comment. I also understand that it wouldn't be great to 
add a deviation for this construct. So maybe we should tweak a bit the 
comment?

Anyway, this code is maintained by Tamas, so I will let him confirm 
which version he prefers.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3

[Jan Beulich]

On 20.12.2023 12:48, Julien Grall wrote:
> On 20/12/2023 11:03, Federico Serafini wrote:
>> --- a/xen/arch/arm/arm64/vsysreg.c
>> +++ b/xen/arch/arm/arm64/vsysreg.c
>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs,
>>           /* RO at EL0. RAZ/WI at EL1 */
>>           if ( regs_mode_is_user(regs) )
>>               return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
>> -        else
>> -            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>> +
>> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
> 
> I don't 100% like this change (mostly because I find if/else clearer 
> here).

While (it doesn't matter here) my view on this is different, I'm still
puzzled why the tool would complain / why a change here is necessary.
It is not _one_ return statement, but there's still (and obviously) no
way of falling through.

> But I have the feeling any other solution would probably be 
> worse.

Use the conditional operator?

Jan

> So:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Cheers,
> 

Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3

[Federico Serafini]

On 20/12/23 12:55, Jan Beulich wrote:
> On 20.12.2023 12:48, Julien Grall wrote:
>> On 20/12/2023 11:03, Federico Serafini wrote:
>>> --- a/xen/arch/arm/arm64/vsysreg.c
>>> +++ b/xen/arch/arm/arm64/vsysreg.c
>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs,
>>>            /* RO at EL0. RAZ/WI at EL1 */
>>>            if ( regs_mode_is_user(regs) )
>>>                return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
>>> -        else
>>> -            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>>> +
>>> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>>
>> I don't 100% like this change (mostly because I find if/else clearer
>> here).
> 
> While (it doesn't matter here) my view on this is different, I'm still
> puzzled why the tool would complain / why a change here is necessary.
> It is not _one_ return statement, but there's still (and obviously) no
> way of falling through.

The tool is configurable:
if you prefer deviate these cases instead of refactoring the code
I can update the configuration.


>> But I have the feeling any other solution would probably be
>> worse.
> 
> Use the conditional operator?
> 
> Jan
> 
>> So:
>>
>> Acked-by: Julien Grall <jgrall@amazon.com>
>>
>> Cheers,
>>
> 

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [XEN PATCH 4/7] xen/arm: mem_access: address violations of MISRA C:2012 Rule 16.3

[Tamas K Lengyel]

On Wed, Dec 20, 2023 at 6:53 AM Julien Grall <julien@xen.org> wrote:
>
> Hi Federico,
>
> On 20/12/2023 11:03, Federico Serafini wrote:
> > Refactor of the code to have a break statement at the end of the
> > switch-clause. This addresses violations of Rule 16.3
> > ("An unconditional `break' statement shall terminate every
> > switch-clause").
> > No functional change.
> >
> > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> > ---
> >   xen/arch/arm/mem_access.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> > index 31db846354..fbcb5471f7 100644
> > --- a/xen/arch/arm/mem_access.c
> > +++ b/xen/arch/arm/mem_access.c
> > @@ -168,10 +168,10 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
> >            * If this was a read then it was because of mem_access, but if it was
> >            * a write then the original get_page_from_gva fault was correct.
> >            */
> > -        if ( flag == GV2M_READ )
> > -            break;
> > -        else
> > +        if ( flag != GV2M_READ )
> >               goto err;
> > +
> > +        break;
>
> On both hunks, I find the original version better as it is easier to
> match with the comment. I also understand that it wouldn't be great to
> add a deviation for this construct. So maybe we should tweak a bit the
> comment?

Simplifying the if-else to a single if is fine by me. Adjusting the
comment to reflect the new logic would help though, so +1 to Julien's
comment.

Thanks,
Tamas

Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3

[Stefano Stabellini]

On Wed, 20 Dec 2023, Federico Serafini wrote:
> On 20/12/23 12:55, Jan Beulich wrote:
> > On 20.12.2023 12:48, Julien Grall wrote:
> > > On 20/12/2023 11:03, Federico Serafini wrote:
> > > > --- a/xen/arch/arm/arm64/vsysreg.c
> > > > +++ b/xen/arch/arm/arm64/vsysreg.c
> > > > @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs,
> > > >            /* RO at EL0. RAZ/WI at EL1 */
> > > >            if ( regs_mode_is_user(regs) )
> > > >                return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr,
> > > > 0);
> > > > -        else
> > > > -            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr,
> > > > 1);
> > > > +
> > > > +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
> > > 
> > > I don't 100% like this change (mostly because I find if/else clearer
> > > here).
> > 
> > While (it doesn't matter here) my view on this is different, I'm still
> > puzzled why the tool would complain / why a change here is necessary.
> > It is not _one_ return statement, but there's still (and obviously) no
> > way of falling through.
> 
> The tool is configurable:
> if you prefer deviate these cases instead of refactoring the code
> I can update the configuration.
 
If you say "deviation", it implies that there is a violation. I think
Jan's point was that both code versions shouldn't be any different.

# option-1
if (a)
  return f1();
else
  return f2();

# option-2
if (a)
  return f1();
return f2();

Both options are equally guaranteed to always return. It looks like a
peculiar limitation to only recognize option-2 as something that returns
100% of the times. Also option-1 returns 100% of the times. What am I
missing?

Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3

[Andrew Cooper]

On 20/12/2023 6:24 pm, Stefano Stabellini wrote:
> On Wed, 20 Dec 2023, Federico Serafini wrote:
>> On 20/12/23 12:55, Jan Beulich wrote:
>>> On 20.12.2023 12:48, Julien Grall wrote:
>>>> On 20/12/2023 11:03, Federico Serafini wrote:
>>>>> --- a/xen/arch/arm/arm64/vsysreg.c
>>>>> +++ b/xen/arch/arm/arm64/vsysreg.c
>>>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs,
>>>>>            /* RO at EL0. RAZ/WI at EL1 */
>>>>>            if ( regs_mode_is_user(regs) )
>>>>>                return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr,
>>>>> 0);
>>>>> -        else
>>>>> -            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr,
>>>>> 1);
>>>>> +
>>>>> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>>>> I don't 100% like this change (mostly because I find if/else clearer
>>>> here).
>>> While (it doesn't matter here) my view on this is different, I'm still
>>> puzzled why the tool would complain / why a change here is necessary.
>>> It is not _one_ return statement, but there's still (and obviously) no
>>> way of falling through.
>> The tool is configurable:
>> if you prefer deviate these cases instead of refactoring the code
>> I can update the configuration.
>  
> If you say "deviation", it implies that there is a violation. I think
> Jan's point was that both code versions shouldn't be any different.
>
> # option-1
> if (a)
>   return f1();
> else
>   return f2();
>
> # option-2
> if (a)
>   return f1();
> return f2();
>
> Both options are equally guaranteed to always return. It looks like a
> peculiar limitation to only recognize option-2 as something that returns
> 100% of the times. Also option-1 returns 100% of the times. What am I
> missing?

For completeness, it's worth saying that there is an option-3:

    return a ? f1() : f2();

which is very clearly only a single return, but I personally don't like
this as I often find it to be less clear than either other option.

All options have a guaranteed return, but there cases including this one
where option-1 is clearest way to write the logic.

~Andrew

Re: [XEN PATCH 0/7] xen/arm: address violations of MISRA C:2012 Rule 16.3

[Andrew Cooper]

On 20/12/2023 11:03 am, Federico Serafini wrote:
> This patch series addresses violations of MISRA C:2012 Rule 16.3 on the Arm
> code. No fucntional changes are introduced.
>
> Federico Serafini (7):
>   xen/arm: gic-v3: address violations of MISRA C:2012 Rule 16.3
>   xen/arm: traps: address violations of MISRA C:2012 Rule 16.3
>   xen/arm: guest_walk: address violations of MISRA C:2012 Rule 16.3
>   xen/arm: mem_access: address violations of MISRA C:2012 Rule 16.3
>   xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3
>   xen/arm: mmu: address a violations of MISRA C:2012 Rule 16.3
>   xen/arm: smmu-v3: address violations of MISRA C:2012 Rule 16.3
>
>  xen/arch/arm/arm64/vsysreg.c          |  4 ++--
>  xen/arch/arm/gic-v3.c                 | 30 +++++++++++++++++++++++++++
>  xen/arch/arm/guest_walk.c             |  4 ++++
>  xen/arch/arm/mem_access.c             | 12 +++++------
>  xen/arch/arm/mmu/p2m.c                |  1 +
>  xen/arch/arm/traps.c                  | 18 ++++++++++++----
>  xen/arch/arm/vcpreg.c                 |  4 ++--
>  xen/drivers/passthrough/arm/smmu-v3.c |  2 ++
>  8 files changed, 61 insertions(+), 14 deletions(-)
>

Just a couple of notes on style.  This isn't a request to change
anything in this series, particularly as most is already committed, but
bear it in mind for what I expect will be similar patches in other areas.

We explicitly permit tabulation when it aids readibility, so patch 2
could have been written:

        switch ( hypercall_args[*nr] ) {
        case 5: HYPERCALL_ARG5(regs) = 0xDEADBEEFU; fallthrough;
        case 4: HYPERCALL_ARG4(regs) = 0xDEADBEEFU; fallthrough;
        case 3: HYPERCALL_ARG3(regs) = 0xDEADBEEFU; fallthrough;
        case 2: HYPERCALL_ARG2(regs) = 0xDEADBEEFU; fallthrough;
        case 1: /* Don't clobber x0/r0 -- it's the return value */
        case 0: /* -ENOSYS case */
            break;
        default: BUG();
        }

(give or take the brace placement other style issue)  We also have cases
where a break before a new case statement is preferred, i.e.:

        ...
        break;

    case ...:

This is to prevent larger switch statements from being a straight wall
of text.

If in doubt, match the style around it.  Please don't de-tabulate
examples which are already tabulated.  (i.e. don't de-tabulate the x86
versions of patch 2.)

~Andrew

Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3

[Jan Beulich]

On 20.12.2023 13:15, Federico Serafini wrote:
> On 20/12/23 12:55, Jan Beulich wrote:
>> On 20.12.2023 12:48, Julien Grall wrote:
>>> On 20/12/2023 11:03, Federico Serafini wrote:
>>>> --- a/xen/arch/arm/arm64/vsysreg.c
>>>> +++ b/xen/arch/arm/arm64/vsysreg.c
>>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs,
>>>>            /* RO at EL0. RAZ/WI at EL1 */
>>>>            if ( regs_mode_is_user(regs) )
>>>>                return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
>>>> -        else
>>>> -            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>>>> +
>>>> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>>>
>>> I don't 100% like this change (mostly because I find if/else clearer
>>> here).
>>
>> While (it doesn't matter here) my view on this is different, I'm still
>> puzzled why the tool would complain / why a change here is necessary.
>> It is not _one_ return statement, but there's still (and obviously) no
>> way of falling through.
> 
> The tool is configurable:
> if you prefer deviate these cases instead of refactoring the code
> I can update the configuration.

I guess this then needs to be discussed on the first call in the new year.
Stefano - can you take note of that, please?

Jan

Re: [XEN PATCH 0/7] xen/arm: address violations of MISRA C:2012 Rule 16.3

[Jan Beulich]

On 20.12.2023 22:35, Andrew Cooper wrote:
> On 20/12/2023 11:03 am, Federico Serafini wrote:
>> This patch series addresses violations of MISRA C:2012 Rule 16.3 on the Arm
>> code. No fucntional changes are introduced.
>>
>> Federico Serafini (7):
>>   xen/arm: gic-v3: address violations of MISRA C:2012 Rule 16.3
>>   xen/arm: traps: address violations of MISRA C:2012 Rule 16.3
>>   xen/arm: guest_walk: address violations of MISRA C:2012 Rule 16.3
>>   xen/arm: mem_access: address violations of MISRA C:2012 Rule 16.3
>>   xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3
>>   xen/arm: mmu: address a violations of MISRA C:2012 Rule 16.3
>>   xen/arm: smmu-v3: address violations of MISRA C:2012 Rule 16.3
>>
>>  xen/arch/arm/arm64/vsysreg.c          |  4 ++--
>>  xen/arch/arm/gic-v3.c                 | 30 +++++++++++++++++++++++++++
>>  xen/arch/arm/guest_walk.c             |  4 ++++
>>  xen/arch/arm/mem_access.c             | 12 +++++------
>>  xen/arch/arm/mmu/p2m.c                |  1 +
>>  xen/arch/arm/traps.c                  | 18 ++++++++++++----
>>  xen/arch/arm/vcpreg.c                 |  4 ++--
>>  xen/drivers/passthrough/arm/smmu-v3.c |  2 ++
>>  8 files changed, 61 insertions(+), 14 deletions(-)
>>
> 
> Just a couple of notes on style.  This isn't a request to change
> anything in this series, particularly as most is already committed, but
> bear it in mind for what I expect will be similar patches in other areas.
> 
> We explicitly permit tabulation when it aids readibility, so patch 2
> could have been written:
> 
>         switch ( hypercall_args[*nr] ) {
>         case 5: HYPERCALL_ARG5(regs) = 0xDEADBEEFU; fallthrough;
>         case 4: HYPERCALL_ARG4(regs) = 0xDEADBEEFU; fallthrough;
>         case 3: HYPERCALL_ARG3(regs) = 0xDEADBEEFU; fallthrough;
>         case 2: HYPERCALL_ARG2(regs) = 0xDEADBEEFU; fallthrough;
>         case 1: /* Don't clobber x0/r0 -- it's the return value */
>         case 0: /* -ENOSYS case */
>             break;
>         default: BUG();
>         }
> 
> (give or take the brace placement other style issue)  We also have cases
> where a break before a new case statement is preferred, i.e.:

Did you mean "blank line" here, seeing ...

>         ...
>         break;
> 
>     case ...:
> 
> This is to prevent larger switch statements from being a straight wall
> of text.

... this as the further explanation?

Jan

Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3

[Federico Serafini]

On 20/12/23 22:23, Andrew Cooper wrote:
> On 20/12/2023 6:24 pm, Stefano Stabellini wrote:
>> On Wed, 20 Dec 2023, Federico Serafini wrote:
>>> On 20/12/23 12:55, Jan Beulich wrote:
>>>> On 20.12.2023 12:48, Julien Grall wrote:
>>>>> On 20/12/2023 11:03, Federico Serafini wrote:
>>>>>> --- a/xen/arch/arm/arm64/vsysreg.c
>>>>>> +++ b/xen/arch/arm/arm64/vsysreg.c
>>>>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs,
>>>>>>             /* RO at EL0. RAZ/WI at EL1 */
>>>>>>             if ( regs_mode_is_user(regs) )
>>>>>>                 return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr,
>>>>>> 0);
>>>>>> -        else
>>>>>> -            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr,
>>>>>> 1);
>>>>>> +
>>>>>> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>>>>> I don't 100% like this change (mostly because I find if/else clearer
>>>>> here).
>>>> While (it doesn't matter here) my view on this is different, I'm still
>>>> puzzled why the tool would complain / why a change here is necessary.
>>>> It is not _one_ return statement, but there's still (and obviously) no
>>>> way of falling through.
>>> The tool is configurable:
>>> if you prefer deviate these cases instead of refactoring the code
>>> I can update the configuration.
>>   
>> If you say "deviation", it implies that there is a violation. I think
>> Jan's point was that both code versions shouldn't be any different.
>>
>> # option-1
>> if (a)
>>    return f1();
>> else
>>    return f2();
>>
>> # option-2
>> if (a)
>>    return f1();
>> return f2();
>>
>> Both options are equally guaranteed to always return. It looks like a
>> peculiar limitation to only recognize option-2 as something that returns
>> 100% of the times. Also option-1 returns 100% of the times. What am I
>> missing?

I don't think this is necessarily a limitation because it highlights
cases where an if-else could be replaced with a simple if:
some may find an if-else clearer, other may find the single if clearer.

 From a safety point of view both options are safe because there
is no risk of unintentional fall through.

If you all agree on the fact that small code refactoring like the ones I
proposed are counterproductive, then I can update the tool configuration
to consider also option-1 as safe.

> 
> For completeness, it's worth saying that there is an option-3:
> 
>      return a ? f1() : f2();
> 
> which is very clearly only a single return, but I personally don't like
> this as I often find it to be less clear than either other option.

Option-3 is currently considered as safe.

> 
> All options have a guaranteed return, but there cases including this one
> where option-1 is clearest way to write the logic.
> 
> ~Andrew

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [XEN PATCH 0/7] xen/arm: address violations of MISRA C:2012 Rule 16.3

[Federico Serafini]

On 20/12/23 22:35, Andrew Cooper wrote:
> On 20/12/2023 11:03 am, Federico Serafini wrote:
>> This patch series addresses violations of MISRA C:2012 Rule 16.3 on the Arm
>> code. No fucntional changes are introduced.
>>
>> Federico Serafini (7):
>>    xen/arm: gic-v3: address violations of MISRA C:2012 Rule 16.3
>>    xen/arm: traps: address violations of MISRA C:2012 Rule 16.3
>>    xen/arm: guest_walk: address violations of MISRA C:2012 Rule 16.3
>>    xen/arm: mem_access: address violations of MISRA C:2012 Rule 16.3
>>    xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3
>>    xen/arm: mmu: address a violations of MISRA C:2012 Rule 16.3
>>    xen/arm: smmu-v3: address violations of MISRA C:2012 Rule 16.3
>>
>>   xen/arch/arm/arm64/vsysreg.c          |  4 ++--
>>   xen/arch/arm/gic-v3.c                 | 30 +++++++++++++++++++++++++++
>>   xen/arch/arm/guest_walk.c             |  4 ++++
>>   xen/arch/arm/mem_access.c             | 12 +++++------
>>   xen/arch/arm/mmu/p2m.c                |  1 +
>>   xen/arch/arm/traps.c                  | 18 ++++++++++++----
>>   xen/arch/arm/vcpreg.c                 |  4 ++--
>>   xen/drivers/passthrough/arm/smmu-v3.c |  2 ++
>>   8 files changed, 61 insertions(+), 14 deletions(-)
>>
> 
> Just a couple of notes on style.  This isn't a request to change
> anything in this series, particularly as most is already committed, but
> bear it in mind for what I expect will be similar patches in other areas.
> 
> We explicitly permit tabulation when it aids readibility, so patch 2
> could have been written:
> 
>          switch ( hypercall_args[*nr] ) {
>          case 5: HYPERCALL_ARG5(regs) = 0xDEADBEEFU; fallthrough;
>          case 4: HYPERCALL_ARG4(regs) = 0xDEADBEEFU; fallthrough;
>          case 3: HYPERCALL_ARG3(regs) = 0xDEADBEEFU; fallthrough;
>          case 2: HYPERCALL_ARG2(regs) = 0xDEADBEEFU; fallthrough;
>          case 1: /* Don't clobber x0/r0 -- it's the return value */
>          case 0: /* -ENOSYS case */
>              break;
>          default: BUG();
>          }
> 
> (give or take the brace placement other style issue)  We also have cases
> where a break before a new case statement is preferred, i.e.:
> 
>          ...
>          break;
> 
>      case ...:
> 
> This is to prevent larger switch statements from being a straight wall
> of text.
> 
> If in doubt, match the style around it.  Please don't de-tabulate
> examples which are already tabulated.  (i.e. don't de-tabulate the x86
> versions of patch 2.)
> 
> ~Andrew

Understood, thank you.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3

[Andrew Cooper]

On 21/12/2023 10:29 am, Federico Serafini wrote:
> On 20/12/23 22:23, Andrew Cooper wrote:
>> On 20/12/2023 6:24 pm, Stefano Stabellini wrote:
>>> On Wed, 20 Dec 2023, Federico Serafini wrote:
>>>> On 20/12/23 12:55, Jan Beulich wrote:
>>>>> On 20.12.2023 12:48, Julien Grall wrote:
>>>>>> On 20/12/2023 11:03, Federico Serafini wrote:
>>>>>>> --- a/xen/arch/arm/arm64/vsysreg.c
>>>>>>> +++ b/xen/arch/arm/arm64/vsysreg.c
>>>>>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs,
>>>>>>>             /* RO at EL0. RAZ/WI at EL1 */
>>>>>>>             if ( regs_mode_is_user(regs) )
>>>>>>>                 return handle_ro_raz(regs, regidx,
>>>>>>> hsr.sysreg.read, hsr,
>>>>>>> 0);
>>>>>>> -        else
>>>>>>> -            return handle_raz_wi(regs, regidx, hsr.sysreg.read,
>>>>>>> hsr,
>>>>>>> 1);
>>>>>>> +
>>>>>>> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read,
>>>>>>> hsr, 1);
>>>>>> I don't 100% like this change (mostly because I find if/else clearer
>>>>>> here).
>>>>> While (it doesn't matter here) my view on this is different, I'm
>>>>> still
>>>>> puzzled why the tool would complain / why a change here is necessary.
>>>>> It is not _one_ return statement, but there's still (and
>>>>> obviously) no
>>>>> way of falling through.
>>>> The tool is configurable:
>>>> if you prefer deviate these cases instead of refactoring the code
>>>> I can update the configuration.
>>>   If you say "deviation", it implies that there is a violation. I think
>>> Jan's point was that both code versions shouldn't be any different.
>>>
>>> # option-1
>>> if (a)
>>>    return f1();
>>> else
>>>    return f2();
>>>
>>> # option-2
>>> if (a)
>>>    return f1();
>>> return f2();
>>>
>>> Both options are equally guaranteed to always return. It looks like a
>>> peculiar limitation to only recognize option-2 as something that
>>> returns
>>> 100% of the times. Also option-1 returns 100% of the times. What am I
>>> missing?
>
> I don't think this is necessarily a limitation because it highlights
> cases where an if-else could be replaced with a simple if:
> some may find an if-else clearer, other may find the single if clearer.
>
> From a safety point of view both options are safe because there
> is no risk of unintentional fall through.
>
> If you all agree on the fact that small code refactoring like the ones I
> proposed are counterproductive, then I can update the tool configuration
> to consider also option-1 as safe.

I would certainly prefer if we could continue to use option 1.

~Andrew

Re: [XEN PATCH 0/7] xen/arm: address violations of MISRA C:2012 Rule 16.3

[Andrew Cooper]

On 21/12/2023 8:08 am, Jan Beulich wrote:
> On 20.12.2023 22:35, Andrew Cooper wrote:
>> On 20/12/2023 11:03 am, Federico Serafini wrote:
>>> This patch series addresses violations of MISRA C:2012 Rule 16.3 on the Arm
>>> code. No fucntional changes are introduced.
>>>
>>> Federico Serafini (7):
>>>   xen/arm: gic-v3: address violations of MISRA C:2012 Rule 16.3
>>>   xen/arm: traps: address violations of MISRA C:2012 Rule 16.3
>>>   xen/arm: guest_walk: address violations of MISRA C:2012 Rule 16.3
>>>   xen/arm: mem_access: address violations of MISRA C:2012 Rule 16.3
>>>   xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3
>>>   xen/arm: mmu: address a violations of MISRA C:2012 Rule 16.3
>>>   xen/arm: smmu-v3: address violations of MISRA C:2012 Rule 16.3
>>>
>>>  xen/arch/arm/arm64/vsysreg.c          |  4 ++--
>>>  xen/arch/arm/gic-v3.c                 | 30 +++++++++++++++++++++++++++
>>>  xen/arch/arm/guest_walk.c             |  4 ++++
>>>  xen/arch/arm/mem_access.c             | 12 +++++------
>>>  xen/arch/arm/mmu/p2m.c                |  1 +
>>>  xen/arch/arm/traps.c                  | 18 ++++++++++++----
>>>  xen/arch/arm/vcpreg.c                 |  4 ++--
>>>  xen/drivers/passthrough/arm/smmu-v3.c |  2 ++
>>>  8 files changed, 61 insertions(+), 14 deletions(-)
>>>
>> Just a couple of notes on style.  This isn't a request to change
>> anything in this series, particularly as most is already committed, but
>> bear it in mind for what I expect will be similar patches in other areas.
>>
>> We explicitly permit tabulation when it aids readibility, so patch 2
>> could have been written:
>>
>>         switch ( hypercall_args[*nr] ) {
>>         case 5: HYPERCALL_ARG5(regs) = 0xDEADBEEFU; fallthrough;
>>         case 4: HYPERCALL_ARG4(regs) = 0xDEADBEEFU; fallthrough;
>>         case 3: HYPERCALL_ARG3(regs) = 0xDEADBEEFU; fallthrough;
>>         case 2: HYPERCALL_ARG2(regs) = 0xDEADBEEFU; fallthrough;
>>         case 1: /* Don't clobber x0/r0 -- it's the return value */
>>         case 0: /* -ENOSYS case */
>>             break;
>>         default: BUG();
>>         }
>>
>> (give or take the brace placement other style issue)  We also have cases
>> where a break before a new case statement is preferred, i.e.:
> Did you mean "blank line" here, seeing ...
>
>>         ...
>>         break;
>>
>>     case ...:
>>
>> This is to prevent larger switch statements from being a straight wall
>> of text.
> ... this as the further explanation?

Urgh yes - I did mean blank line.  Hopefully the intent was clear.

~Andrew

Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3

[Stefano Stabellini]

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

On Thu, 21 Dec 2023, Andrew Cooper wrote:
> On 21/12/2023 10:29 am, Federico Serafini wrote:
> > On 20/12/23 22:23, Andrew Cooper wrote:
> >> On 20/12/2023 6:24 pm, Stefano Stabellini wrote:
> >>> On Wed, 20 Dec 2023, Federico Serafini wrote:
> >>>> On 20/12/23 12:55, Jan Beulich wrote:
> >>>>> On 20.12.2023 12:48, Julien Grall wrote:
> >>>>>> On 20/12/2023 11:03, Federico Serafini wrote:
> >>>>>>> --- a/xen/arch/arm/arm64/vsysreg.c
> >>>>>>> +++ b/xen/arch/arm/arm64/vsysreg.c
> >>>>>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs,
> >>>>>>>             /* RO at EL0. RAZ/WI at EL1 */
> >>>>>>>             if ( regs_mode_is_user(regs) )
> >>>>>>>                 return handle_ro_raz(regs, regidx,
> >>>>>>> hsr.sysreg.read, hsr,
> >>>>>>> 0);
> >>>>>>> -        else
> >>>>>>> -            return handle_raz_wi(regs, regidx, hsr.sysreg.read,
> >>>>>>> hsr,
> >>>>>>> 1);
> >>>>>>> +
> >>>>>>> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read,
> >>>>>>> hsr, 1);
> >>>>>> I don't 100% like this change (mostly because I find if/else clearer
> >>>>>> here).
> >>>>> While (it doesn't matter here) my view on this is different, I'm
> >>>>> still
> >>>>> puzzled why the tool would complain / why a change here is necessary.
> >>>>> It is not _one_ return statement, but there's still (and
> >>>>> obviously) no
> >>>>> way of falling through.
> >>>> The tool is configurable:
> >>>> if you prefer deviate these cases instead of refactoring the code
> >>>> I can update the configuration.
> >>>   If you say "deviation", it implies that there is a violation. I think
> >>> Jan's point was that both code versions shouldn't be any different.
> >>>
> >>> # option-1
> >>> if (a)
> >>>    return f1();
> >>> else
> >>>    return f2();
> >>>
> >>> # option-2
> >>> if (a)
> >>>    return f1();
> >>> return f2();
> >>>
> >>> Both options are equally guaranteed to always return. It looks like a
> >>> peculiar limitation to only recognize option-2 as something that
> >>> returns
> >>> 100% of the times. Also option-1 returns 100% of the times. What am I
> >>> missing?
> >
> > I don't think this is necessarily a limitation because it highlights
> > cases where an if-else could be replaced with a simple if:
> > some may find an if-else clearer, other may find the single if clearer.
> >
> > From a safety point of view both options are safe because there
> > is no risk of unintentional fall through.
> >
> > If you all agree on the fact that small code refactoring like the ones I
> > proposed are counterproductive, then I can update the tool configuration
> > to consider also option-1 as safe.
> 
> I would certainly prefer if we could continue to use option 1.

Yes, that is my preference too

Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3

[Stefano Stabellini]

On Thu, 21 Dec 2023, Jan Beulich wrote:
> On 20.12.2023 13:15, Federico Serafini wrote:
> > On 20/12/23 12:55, Jan Beulich wrote:
> >> On 20.12.2023 12:48, Julien Grall wrote:
> >>> On 20/12/2023 11:03, Federico Serafini wrote:
> >>>> --- a/xen/arch/arm/arm64/vsysreg.c
> >>>> +++ b/xen/arch/arm/arm64/vsysreg.c
> >>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs,
> >>>>            /* RO at EL0. RAZ/WI at EL1 */
> >>>>            if ( regs_mode_is_user(regs) )
> >>>>                return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
> >>>> -        else
> >>>> -            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
> >>>> +
> >>>> +        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
> >>>
> >>> I don't 100% like this change (mostly because I find if/else clearer
> >>> here).
> >>
> >> While (it doesn't matter here) my view on this is different, I'm still
> >> puzzled why the tool would complain / why a change here is necessary.
> >> It is not _one_ return statement, but there's still (and obviously) no
> >> way of falling through.
> > 
> > The tool is configurable:
> > if you prefer deviate these cases instead of refactoring the code
> > I can update the configuration.
> 
> I guess this then needs to be discussed on the first call in the new year.
> Stefano - can you take note of that, please?

Yes, will do

Re: [XEN PATCH 7/7] xen/arm: smmu-v3: address violations of MISRA C:2012 Rule 16.3

[Federico Serafini]

Hi Julien,

On 20/12/23 12:49, Julien Grall wrote:
> Hi Federico,
> 
> On 20/12/2023 11:03, Federico Serafini wrote:
>> Add a break statement at the end of the switch-clauses to address
>> violations of MISRA C:2012 Rule 16.3 ("An unconditional `break'
>> statement shall terminate every switch-clause").
>> No funtional change.
>>
>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
>

I noticed that this has not yet been committed.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [XEN PATCH 7/7] xen/arm: smmu-v3: address violations of MISRA C:2012 Rule 16.3

[Julien Grall]

Hi,

On 05/01/2024 15:04, Federico Serafini wrote:
> On 20/12/23 12:49, Julien Grall wrote:
>> Hi Federico,
>>
>> On 20/12/2023 11:03, Federico Serafini wrote:
>>> Add a break statement at the end of the switch-clauses to address
>>> violations of MISRA C:2012 Rule 16.3 ("An unconditional `break'
>>> statement shall terminate every switch-clause").
>>> No funtional change.
>>>
>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>>
>> Acked-by: Julien Grall <jgrall@amazon.com>
>>
>  > I noticed that this has not yet been committed.

The SMMUv3 maintainers are not the same as the rest of the Arm code. So 
you technically need an Ack from one of them before this can get merged.

I have pinged Bertrand on Matrix.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 7/7] xen/arm: smmu-v3: address violations of MISRA C:2012 Rule 16.3

[Bertrand Marquis]

Hi Frederico,

Sorry to have missed that one.

> On 20 Dec 2023, at 12:03, Federico Serafini <federico.serafini@bugseng.com> wrote:
> 
> Add a break statement at the end of the switch-clauses to address
> violations of MISRA C:2012 Rule 16.3 ("An unconditional `break'
> statement shall terminate every switch-clause").
> No funtional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com

Cheers
Bertrand

> ---
> xen/drivers/passthrough/arm/smmu-v3.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
> index cdbb505134..bca5866b35 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -2200,6 +2200,7 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
> fallthrough;
> case IDR0_STALL_MODEL_STALL:
> smmu->features |= ARM_SMMU_FEAT_STALLS;
> + break;
> }
> 
> if (reg & IDR0_S2P)
> @@ -2301,6 +2302,7 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
> fallthrough;
> case IDR5_OAS_48_BIT:
> smmu->oas = 48;
> + break;
> }
> 
> smmu->oas = min_t(unsigned long, PADDR_BITS, smmu->oas);
> -- 
> 2.34.1
>