[XEN PATCH 00/12] x86: address violations of MISRA C Rule 16.3

[XEN PATCH 00/12] x86: address violations of MISRA C Rule 16.3

[Federico Serafini]

This patch series addresses violations of Rule 16.3 on the x86 code.

If all the 12 patches will be committed, only 4 violations will be left:
these are under x86_emulate and involve some fallthrough cases.
I'll wait for instructions from x86 maintainers about the right "format" to make
the fallthrough intention explicit (i.e., a comment or the fallthrough keyword
that is not defined in x86-emulate.h at the moment).

Federico Serafini (12):
  x86/psr: address violation of MISRA C Rule 16.3
  x86/time: address violations of MISRA C Rule 16.3
  x86/vm_event: address violation of MISRA C Rule 16.3
  x86/hypercall: address violations of MISRA C Rule 16.3
  x86/monitor: address violation of MISRA C Rule 16.3
  x86/mm: address violations of MISRA C Rule 16.3
  x86/mmcfg: address violation of MISRA C Rule 16.3
  x86/emul: add defensive code
  x86/emul: address violations of MISRA C Rule 16.3
  efi: address violation of MISRA C Rule 16.3
  xen/vpci: add defensive code
  xen/pci: address a violation of MISRA C Rule 16.3

 xen/arch/x86/include/asm/hypercall.h   | 4 ++--
 xen/arch/x86/include/asm/monitor.h     | 1 +
 xen/arch/x86/mm/guest_walk.c           | 1 +
 xen/arch/x86/mm/hap/hap.c              | 2 +-
 xen/arch/x86/mm/hap/nested_hap.c       | 1 +
 xen/arch/x86/mm/paging.c               | 2 +-
 xen/arch/x86/psr.c                     | 1 +
 xen/arch/x86/time.c                    | 2 ++
 xen/arch/x86/vm_event.c                | 1 +
 xen/arch/x86/x86_64/mmconfig-shared.c  | 1 +
 xen/arch/x86/x86_emulate/fpu.c         | 4 ++++
 xen/arch/x86/x86_emulate/x86_emulate.c | 5 +++++
 xen/common/efi/runtime.c               | 2 +-
 xen/drivers/passthrough/pci.c          | 4 +++-
 xen/drivers/vpci/msix.c                | 4 ++++
 15 files changed, 29 insertions(+), 6 deletions(-)

-- 
2.34.1

[XEN PATCH 01/12] x86/psr: address violation of MISRA C Rule 16.3

[Federico Serafini]

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/x86/psr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 0b9631ac44..5815a35335 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -276,6 +276,7 @@ static enum psr_feat_type psr_type_to_feat_type(enum psr_type type)
 
     default:
         ASSERT_UNREACHABLE();
+        break;
     }
 
     return feat_type;
-- 
2.34.1

Re: [XEN PATCH 01/12] x86/psr: address violation of MISRA C Rule 16.3

[Jan Beulich]

On 10.09.2024 12:08, Federico Serafini wrote:
> 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: Jan Beulich <jbeulich@suse.com>

[XEN PATCH 02/12] x86/time: address violations of MISRA C Rule 16.3

[Federico Serafini]

Address violations 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/x86/time.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index f37300946e..d79b0a6ab9 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1477,6 +1477,7 @@ unsigned int rtc_guest_read(unsigned int port)
 
     default:
         ASSERT_UNREACHABLE();
+        break;
     }
 
     return data;
@@ -1529,6 +1530,7 @@ void rtc_guest_write(unsigned int port, unsigned int data)
 
     default:
         ASSERT_UNREACHABLE();
+        break;
     }
 }
 
-- 
2.34.1

Re: [XEN PATCH 02/12] x86/time: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 10.09.2024 12:08, Federico Serafini wrote:
> Address violations 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: Jan Beulich <jbeulich@suse.com>

[XEN PATCH 03/12] x86/vm_event: address violation of MISRA C Rule 16.3

[Federico Serafini]

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/x86/vm_event.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index e6c7ad5337..0d15d363c3 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -190,6 +190,7 @@ static void vm_event_pack_segment_register(enum x86_segment segment,
 
     default:
         ASSERT_UNREACHABLE();
+        break;
     }
 }
 #endif
-- 
2.34.1

Re: [XEN PATCH 03/12] x86/vm_event: address violation of MISRA C Rule 16.3

[Tamas K Lengyel]

On Tue, Sep 10, 2024 at 6:09 AM Federico Serafini
<federico.serafini@bugseng.com> wrote:
>
> 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: Tamas K Lengyel <tamas@tklengyel.com>

[XEN PATCH 04/12] x86/hypercall: address violations of MISRA C Rule 16.3

[Federico Serafini]

Address violations 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/x86/include/asm/hypercall.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h
index 389fa62af2..f6e9e2313b 100644
--- a/xen/arch/x86/include/asm/hypercall.h
+++ b/xen/arch/x86/include/asm/hypercall.h
@@ -55,7 +55,7 @@ static inline void clobber_regs64(struct cpu_user_regs *regs,
     case 4: regs->r10 = 0xdeadbeefdeadf00dUL; fallthrough;
     case 3: regs->rdx = 0xdeadbeefdeadf00dUL; fallthrough;
     case 2: regs->rsi = 0xdeadbeefdeadf00dUL; fallthrough;
-    case 1: regs->rdi = 0xdeadbeefdeadf00dUL;
+    case 1: regs->rdi = 0xdeadbeefdeadf00dUL; break;
     }
 #endif
 }
@@ -71,7 +71,7 @@ static inline void clobber_regs32(struct cpu_user_regs *regs,
     case 4: regs->esi = 0xdeadf00dU; fallthrough;
     case 3: regs->edx = 0xdeadf00dU; fallthrough;
     case 2: regs->ecx = 0xdeadf00dU; fallthrough;
-    case 1: regs->ebx = 0xdeadf00dU;
+    case 1: regs->ebx = 0xdeadf00dU; break;
     }
 #endif
 }
-- 
2.34.1

Re: [XEN PATCH 04/12] x86/hypercall: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 10.09.2024 12:08, Federico Serafini wrote:
> Address violations 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: Jan Beulich <jbeulich@suse.com>

[XEN PATCH 05/12] x86/monitor: address violation of MISRA C Rule 16.3

[Federico Serafini]

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/x86/include/asm/monitor.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/include/asm/monitor.h b/xen/arch/x86/include/asm/monitor.h
index 96e6a9d0d8..3c64d8258f 100644
--- a/xen/arch/x86/include/asm/monitor.h
+++ b/xen/arch/x86/include/asm/monitor.h
@@ -65,6 +65,7 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
 
     default:
         rc = -EOPNOTSUPP;
+        break;
     }
 
     return rc;
-- 
2.34.1

Re: [XEN PATCH 05/12] x86/monitor: address violation of MISRA C Rule 16.3

[Tamas K Lengyel]

On Tue, Sep 10, 2024 at 6:09 AM Federico Serafini
<federico.serafini@bugseng.com> wrote:
>
> 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: Tamas K Lengyel <tamas@tklengyel.com>

[XEN PATCH 06/12] x86/mm: address violations of MISRA C Rule 16.3

[Federico Serafini]

Address violations 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/x86/mm/guest_walk.c     | 1 +
 xen/arch/x86/mm/hap/hap.c        | 2 +-
 xen/arch/x86/mm/hap/nested_hap.c | 1 +
 xen/arch/x86/mm/paging.c         | 2 +-
 4 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
index fe7393334f..bc032d697c 100644
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -497,6 +497,7 @@ guest_walk_tables(const struct vcpu *v, struct p2m_domain *p2m,
             paging_mark_dirty(d, gw->l4mfn);
             hvmemul_write_cache(v, l4gpa, &gw->l4e, sizeof(gw->l4e));
         }
+        break;
 #endif
     }
 
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index d2011fde24..ec5043a8aa 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -652,7 +652,7 @@ int hap_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
         return rc;
     case XEN_DOMCTL_SHADOW_OP_GET_ALLOCATION:
         sc->mb = hap_get_allocation(d);
-        /* Fall through... */
+        fallthrough;
     case XEN_DOMCTL_SHADOW_OP_OFF:
         return 0;
     default:
diff --git a/xen/arch/x86/mm/hap/nested_hap.c b/xen/arch/x86/mm/hap/nested_hap.c
index 080b60adf0..cc7bc6e5ea 100644
--- a/xen/arch/x86/mm/hap/nested_hap.c
+++ b/xen/arch/x86/mm/hap/nested_hap.c
@@ -217,6 +217,7 @@ int nestedhvm_hap_nested_page_fault(
         p2ma_10 = p2m_access_n;
         /* For safety, remove all permissions. */
         gdprintk(XENLOG_ERR, "Unhandled p2m access type:%d\n", p2ma_10);
+        break;
     }
     /* Use minimal permission for nested p2m. */
     p2ma_10 &= (p2m_access_t)p2ma_21;
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index dd47bde5ce..c77f4c1dac 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -726,7 +726,7 @@ int paging_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
     case XEN_DOMCTL_SHADOW_OP_ENABLE:
         if ( !(sc->mode & XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY) )
             break;
-        /* Else fall through... */
+        fallthrough;
     case XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY:
         return paging_log_dirty_enable(d);
 
-- 
2.34.1

Re: [XEN PATCH 06/12] x86/mm: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 10.09.2024 12:08, Federico Serafini wrote:
> Address violations 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/x86/mm/guest_walk.c     | 1 +
>  xen/arch/x86/mm/hap/hap.c        | 2 +-
>  xen/arch/x86/mm/hap/nested_hap.c | 1 +
>  xen/arch/x86/mm/paging.c         | 2 +-
>  4 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
> index fe7393334f..bc032d697c 100644
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -497,6 +497,7 @@ guest_walk_tables(const struct vcpu *v, struct p2m_domain *p2m,
>              paging_mark_dirty(d, gw->l4mfn);
>              hvmemul_write_cache(v, l4gpa, &gw->l4e, sizeof(gw->l4e));
>          }
> +        break;
>  #endif
>      }

This wants inserting after the #endif, I think. If you agree, I'm happy to
adjust while committing. With the adjustment:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [XEN PATCH 06/12] x86/mm: address violations of MISRA C Rule 16.3

[Federico Serafini]

On 10/09/24 16:55, Jan Beulich wrote:
> On 10.09.2024 12:08, Federico Serafini wrote:
>> Address violations 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/x86/mm/guest_walk.c     | 1 +
>>   xen/arch/x86/mm/hap/hap.c        | 2 +-
>>   xen/arch/x86/mm/hap/nested_hap.c | 1 +
>>   xen/arch/x86/mm/paging.c         | 2 +-
>>   4 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
>> index fe7393334f..bc032d697c 100644
>> --- a/xen/arch/x86/mm/guest_walk.c
>> +++ b/xen/arch/x86/mm/guest_walk.c
>> @@ -497,6 +497,7 @@ guest_walk_tables(const struct vcpu *v, struct p2m_domain *p2m,
>>               paging_mark_dirty(d, gw->l4mfn);
>>               hvmemul_write_cache(v, l4gpa, &gw->l4e, sizeof(gw->l4e));
>>           }
>> +        break;
>>   #endif
>>       }
> 
> This wants inserting after the #endif, I think. If you agree, I'm happy to
> adjust while committing. With the adjustment:
> Acked-by: Jan Beulich <jbeulich@suse.com>

You are right, thanks.

-- 
Federico Serafini, M.Sc.

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

[XEN PATCH 07/12] x86/mmcfg: address violation of MISRA C Rule 16.3

[Federico Serafini]

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/x86/x86_64/mmconfig-shared.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/x86_64/mmconfig-shared.c b/xen/arch/x86/x86_64/mmconfig-shared.c
index b3b2da7362..77ba1e28f1 100644
--- a/xen/arch/x86/x86_64/mmconfig-shared.c
+++ b/xen/arch/x86/x86_64/mmconfig-shared.c
@@ -112,6 +112,7 @@ static const char *__init cf_check pci_mmcfg_intel_945(void)
         break;
     default:
         pci_mmcfg_config_num = 0;
+        break;
     }
 
     /* Errata #2, things break when not aligned on a 256Mb boundary */
-- 
2.34.1

Re: [XEN PATCH 07/12] x86/mmcfg: address violation of MISRA C Rule 16.3

[Jan Beulich]

On 10.09.2024 12:08, Federico Serafini wrote:
> 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: Jan Beulich <jbeulich@suse.com>

[XEN PATCH 08/12] x86/emul: add defensive code

[Federico Serafini]

Add defensive code after unreachable program points.
This also meets the requirements to deviate a violation of MISRA C:2012
Rule 16.3: "An unconditional `break' statement shall terminate every
switch-clause".

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

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index e12fbe3100..2b03d65fce 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -6831,6 +6831,8 @@ x86_emulate(
             break;
         default:
             ASSERT_UNREACHABLE();
+            rc = X86EMUL_UNHANDLEABLE;
+            goto done;
         }
         break;
 #ifdef HAVE_AS_SSE4_2
@@ -6859,6 +6861,8 @@ x86_emulate(
 # endif
         default:
             ASSERT_UNREACHABLE();
+            rc = X86EMUL_UNHANDLEABLE;
+            goto done;
         }
         break;
 #endif
-- 
2.34.1

Re: [XEN PATCH 08/12] x86/emul: add defensive code

[Jan Beulich]

On 10.09.2024 12:09, Federico Serafini wrote:
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -6831,6 +6831,8 @@ x86_emulate(
>              break;
>          default:
>              ASSERT_UNREACHABLE();
> +            rc = X86EMUL_UNHANDLEABLE;
> +            goto done;
>          }
>          break;
>  #ifdef HAVE_AS_SSE4_2
> @@ -6859,6 +6861,8 @@ x86_emulate(
>  # endif
>          default:
>              ASSERT_UNREACHABLE();
> +            rc = X86EMUL_UNHANDLEABLE;
> +            goto done;
>          }
>          break;
>  #endif

"goto unhandleable" please in both cases, if already further goto-s are needed.

Jan

[XEN PATCH 09/12] x86/emul: address violations of MISRA C Rule 16.3

[Federico Serafini]

Address violations 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/x86/x86_emulate/fpu.c         | 4 ++++
 xen/arch/x86/x86_emulate/x86_emulate.c | 1 +
 2 files changed, 5 insertions(+)

diff --git a/xen/arch/x86/x86_emulate/fpu.c b/xen/arch/x86/x86_emulate/fpu.c
index 480d879657..3351f549e7 100644
--- a/xen/arch/x86/x86_emulate/fpu.c
+++ b/xen/arch/x86/x86_emulate/fpu.c
@@ -218,6 +218,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
              */
             if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
                 dst->type = OP_NONE;
+            break;
         }
         break;
 
@@ -296,6 +297,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
             default:
                 generate_exception(X86_EXC_UD);
             }
+            break;
         }
         break;
 
@@ -386,6 +388,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
              */
             if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
                 dst->type = OP_NONE;
+            break;
         }
         break;
 
@@ -457,6 +460,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
             case 7: /* fistp m64i */
                 goto fpu_memdst64;
             }
+            break;
         }
         break;
 
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 2b03d65fce..9fb316e0d7 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -8297,6 +8297,7 @@ x86_emulate(
         }
         if ( rc != 0 )
             goto done;
+        break;
     default:
         break;
     }
-- 
2.34.1

Re: [XEN PATCH 09/12] x86/emul: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 10.09.2024 12:09, Federico Serafini wrote:
> --- a/xen/arch/x86/x86_emulate/fpu.c
> +++ b/xen/arch/x86/x86_emulate/fpu.c
> @@ -218,6 +218,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>               */
>              if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>                  dst->type = OP_NONE;
> +            break;
>          }
>          break;
>  
> @@ -296,6 +297,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>              default:
>                  generate_exception(X86_EXC_UD);
>              }
> +            break;
>          }
>          break;
>  
> @@ -386,6 +388,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>               */
>              if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>                  dst->type = OP_NONE;
> +            break;
>          }
>          break;
>  
> @@ -457,6 +460,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>              case 7: /* fistp m64i */
>                  goto fpu_memdst64;
>              }
> +            break;

Aren't you swapping one violation for another here? Unlike in the earlier
three cases, this new break is unreachable, because of the nature of the
preceding switch() statement (cases being exhaustive and every case ending
in "goto"; this is something even a static analyzer can [in principle]
spot).

Jan

Re: [XEN PATCH 09/12] x86/emul: address violations of MISRA C Rule 16.3

[Federico Serafini]

On 11/09/24 14:42, Jan Beulich wrote:
> On 10.09.2024 12:09, Federico Serafini wrote:
>> --- a/xen/arch/x86/x86_emulate/fpu.c
>> +++ b/xen/arch/x86/x86_emulate/fpu.c
>> @@ -218,6 +218,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>                */
>>               if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>>                   dst->type = OP_NONE;
>> +            break;
>>           }
>>           break;
>>   
>> @@ -296,6 +297,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>               default:
>>                   generate_exception(X86_EXC_UD);
>>               }
>> +            break;
>>           }
>>           break;
>>   
>> @@ -386,6 +388,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>                */
>>               if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>>                   dst->type = OP_NONE;
>> +            break;
>>           }
>>           break;
>>   
>> @@ -457,6 +460,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>               case 7: /* fistp m64i */
>>                   goto fpu_memdst64;
>>               }
>> +            break;
> 
> Aren't you swapping one violation for another here? Unlike in the earlier
> three cases, this new break is unreachable, because of the nature of the
> preceding switch() statement (cases being exhaustive and every case ending
> in "goto"; this is something even a static analyzer can [in principle]
> spot).

You are right, but the resulting violation of Rule 2.1
("A project shall not contain unreachable code") is deviated with the
following justification:
"The compiler implementation guarantees that the unreachable code is
removed. Constant expressions and unreachable branches of if and switch
statements are expected."

-- 
Federico Serafini,

Ph.D. Student, Ca' Foscari University of Venice
Software Engineer, BUGSENG

Re: [XEN PATCH 09/12] x86/emul: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 12.09.2024 11:17, Federico Serafini wrote:
> On 11/09/24 14:42, Jan Beulich wrote:
>> On 10.09.2024 12:09, Federico Serafini wrote:
>>> --- a/xen/arch/x86/x86_emulate/fpu.c
>>> +++ b/xen/arch/x86/x86_emulate/fpu.c
>>> @@ -218,6 +218,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>>                */
>>>               if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>>>                   dst->type = OP_NONE;
>>> +            break;
>>>           }
>>>           break;
>>>   
>>> @@ -296,6 +297,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>>               default:
>>>                   generate_exception(X86_EXC_UD);
>>>               }
>>> +            break;
>>>           }
>>>           break;
>>>   
>>> @@ -386,6 +388,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>>                */
>>>               if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>>>                   dst->type = OP_NONE;
>>> +            break;
>>>           }
>>>           break;
>>>   
>>> @@ -457,6 +460,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>>               case 7: /* fistp m64i */
>>>                   goto fpu_memdst64;
>>>               }
>>> +            break;
>>
>> Aren't you swapping one violation for another here? Unlike in the earlier
>> three cases, this new break is unreachable, because of the nature of the
>> preceding switch() statement (cases being exhaustive and every case ending
>> in "goto"; this is something even a static analyzer can [in principle]
>> spot).
> 
> You are right, but the resulting violation of Rule 2.1
> ("A project shall not contain unreachable code") is deviated with the
> following justification:
> "The compiler implementation guarantees that the unreachable code is
> removed.

I'm not convinced this is the case here in practice.

Instead of "break", wouldn't "unreachable()" be the better construct
to use in situations like this one?

> Constant expressions and unreachable branches of if and switch
> statements are expected."

This I don't think applies in this particular case?

Jan

Re: [XEN PATCH 09/12] x86/emul: address violations of MISRA C Rule 16.3

[Federico Serafini]

On 12/09/24 12:01, Jan Beulich wrote:
> On 12.09.2024 11:17, Federico Serafini wrote:
>> On 11/09/24 14:42, Jan Beulich wrote:
>>> On 10.09.2024 12:09, Federico Serafini wrote:
>>>> --- a/xen/arch/x86/x86_emulate/fpu.c
>>>> +++ b/xen/arch/x86/x86_emulate/fpu.c
>>>> @@ -218,6 +218,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>>>                 */
>>>>                if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>>>>                    dst->type = OP_NONE;
>>>> +            break;
>>>>            }
>>>>            break;
>>>>    
>>>> @@ -296,6 +297,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>>>                default:
>>>>                    generate_exception(X86_EXC_UD);
>>>>                }
>>>> +            break;
>>>>            }
>>>>            break;
>>>>    
>>>> @@ -386,6 +388,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>>>                 */
>>>>                if ( dst->type == OP_MEM && !s->fpu_ctrl && !fpu_check_write() )
>>>>                    dst->type = OP_NONE;
>>>> +            break;
>>>>            }
>>>>            break;
>>>>    
>>>> @@ -457,6 +460,7 @@ int x86emul_fpu(struct x86_emulate_state *s,
>>>>                case 7: /* fistp m64i */
>>>>                    goto fpu_memdst64;
>>>>                }
>>>> +            break;
>>>
>>> Aren't you swapping one violation for another here? Unlike in the earlier
>>> three cases, this new break is unreachable, because of the nature of the
>>> preceding switch() statement (cases being exhaustive and every case ending
>>> in "goto"; this is something even a static analyzer can [in principle]
>>> spot).
>>
>> You are right, but the resulting violation of Rule 2.1
>> ("A project shall not contain unreachable code") is deviated with the
>> following justification:
>> "The compiler implementation guarantees that the unreachable code is
>> removed.
> 
> I'm not convinced this is the case here in practice.
> 
> Instead of "break", wouldn't "unreachable()" be the better construct
> to use in situations like this one?
> 
>> Constant expressions and unreachable branches of if and switch
>> statements are expected."
> 
> This I don't think applies in this particular case?

I agree,
the ECLAIR configuration for the deviation covers
more cases than expected. I'll fix it.

-- 
Federico Serafini,

Ph.D. Student, Ca' Foscari University of Venice
Software Engineer, BUGSENG

[XEN PATCH 10/12] efi: address violation of MISRA C Rule 16.3

[Federico Serafini]

Use agreed syntax for pseudo-keyword fallthrough to meet the
requirements to deviate 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/common/efi/runtime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index d03e5c90ce..376fcbd8e1 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -691,7 +691,7 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
 
         if ( !efi_enabled(EFI_RS) || (efi_rs->Hdr.Revision >> 16) < 2 )
             return -EOPNOTSUPP;
-        /* XXX fall through for now */
+        fallthrough; /* XXX fall through for now */
     default:
         return -ENOSYS;
     }
-- 
2.34.1

Re: [XEN PATCH 10/12] efi: address violation of MISRA C Rule 16.3

[Marek Marczykowski-Górecki]

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

On Tue, Sep 10, 2024 at 12:09:02PM +0200, Federico Serafini wrote:
> Use agreed syntax for pseudo-keyword fallthrough to meet the
> requirements to deviate 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: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> ---
>  xen/common/efi/runtime.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> index d03e5c90ce..376fcbd8e1 100644
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -691,7 +691,7 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
>  
>          if ( !efi_enabled(EFI_RS) || (efi_rs->Hdr.Revision >> 16) < 2 )
>              return -EOPNOTSUPP;
> -        /* XXX fall through for now */
> +        fallthrough; /* XXX fall through for now */
>      default:
>          return -ENOSYS;
>      }
> -- 
> 2.34.1
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[XEN PATCH 11/12] xen/vpci: add defensive code

[Federico Serafini]

Add defensive code in unreachable program points.
This also meets the requirements to deviate a violation of MISRA C:2012
Rule 16.3: "An unconditional `break' statement shall terminate every
switch-clause".

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

diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index fbe710ab92..037f9a0449 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -364,6 +364,8 @@ static int adjacent_read(const struct domain *d, const struct vpci_msix *msix,
 
     default:
         ASSERT_UNREACHABLE();
+        spin_unlock(&vpci->lock);
+        return X86EMUL_UNHANDLEABLE;
     }
     spin_unlock(&vpci->lock);
 
@@ -512,6 +514,8 @@ static int adjacent_write(const struct domain *d, const struct vpci_msix *msix,
 
     default:
         ASSERT_UNREACHABLE();
+        spin_unlock(&vpci->lock);
+        return X86EMUL_UNHANDLEABLE;
     }
     spin_unlock(&vpci->lock);
 
-- 
2.34.1

Re: [XEN PATCH 11/12] xen/vpci: add defensive code

[Jan Beulich]

On 10.09.2024 12:09, Federico Serafini wrote:
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -364,6 +364,8 @@ static int adjacent_read(const struct domain *d, const struct vpci_msix *msix,
>  
>      default:
>          ASSERT_UNREACHABLE();
> +        spin_unlock(&vpci->lock);
> +        return X86EMUL_UNHANDLEABLE;
>      }
>      spin_unlock(&vpci->lock);
>  
> @@ -512,6 +514,8 @@ static int adjacent_write(const struct domain *d, const struct vpci_msix *msix,
>  
>      default:
>          ASSERT_UNREACHABLE();
> +        spin_unlock(&vpci->lock);
> +        return X86EMUL_UNHANDLEABLE;
>      }
>      spin_unlock(&vpci->lock);

Both functions only ever return X86EMUL_OKAY right now. I expect your
change wants to be simple "break" insertions, yet I wonder why these
functions aren't returning void then.

Jan

[XEN PATCH 12/12] xen/pci: address a violation of MISRA C Rule 16.3

[Federico Serafini]

Address violations 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/drivers/passthrough/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 5a446d3dce..a5705def3f 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -170,8 +170,10 @@ static int __init cf_check parse_phantom_dev(const char *str)
     {
     case 1: case 2: case 4:
         if ( *s )
-    default:
             return -EINVAL;
+        break;
+    default:
+        return -EINVAL;
     }
 
     phantom_devs[nr_phantom_devs++] = phantom;
-- 
2.34.1

Re: [XEN PATCH 12/12] xen/pci: address a violation of MISRA C Rule 16.3

[Jan Beulich]

On 10.09.2024 12:09, Federico Serafini wrote:
> Address violations of MISRA C:2012 Rule 16.3:
> "An unconditional `break' statement shall terminate every
> switch-clause".

Since in our interpretation "return" is okay too, why is not sufficient to
simply ...

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -170,8 +170,10 @@ static int __init cf_check parse_phantom_dev(const char *str)
>      {
>      case 1: case 2: case 4:
>          if ( *s )
> -    default:
>              return -EINVAL;
> +        break;

... insert just this one line here?

Jan

> +    default:
> +        return -EINVAL;
>      }
>  
>      phantom_devs[nr_phantom_devs++] = phantom;

Re: [XEN PATCH 12/12] xen/pci: address a violation of MISRA C Rule 16.3

[Jan Beulich]

On 10.09.2024 16:57, Jan Beulich wrote:
> On 10.09.2024 12:09, Federico Serafini wrote:
>> Address violations of MISRA C:2012 Rule 16.3:
>> "An unconditional `break' statement shall terminate every
>> switch-clause".
> 
> Since in our interpretation "return" is okay too, why is not sufficient to
> simply ...
> 
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -170,8 +170,10 @@ static int __init cf_check parse_phantom_dev(const char *str)
>>      {
>>      case 1: case 2: case 4:
>>          if ( *s )
>> -    default:
>>              return -EINVAL;
>> +        break;
> 
> ... insert just this one line here?

I guess the problem is with the description: It's un-annotated fall-through
in this case, not so much the lack of a break (or alike).

Jan

>> +    default:
>> +        return -EINVAL;
>>      }
>>  
>>      phantom_devs[nr_phantom_devs++] = phantom;
> 

Re: [XEN PATCH 12/12] xen/pci: address a violation of MISRA C Rule 16.3

[Federico Serafini]

On 10/09/24 16:59, Jan Beulich wrote:
> On 10.09.2024 16:57, Jan Beulich wrote:
>> On 10.09.2024 12:09, Federico Serafini wrote:
>>> Address violations of MISRA C:2012 Rule 16.3:
>>> "An unconditional `break' statement shall terminate every
>>> switch-clause".
>>
>> Since in our interpretation "return" is okay too, why is not sufficient to
>> simply ...
>>
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -170,8 +170,10 @@ static int __init cf_check parse_phantom_dev(const char *str)
>>>       {
>>>       case 1: case 2: case 4:
>>>           if ( *s )
>>> -    default:
>>>               return -EINVAL;
>>> +        break;
>>
>> ... insert just this one line here?
> 
> I guess the problem is with the description: It's un-annotated fall-through
> in this case, not so much the lack of a break (or alike).
> 
> Jan
> 
>>> +    default:
>>> +        return -EINVAL;
>>>       }
>>>   
>>>       phantom_devs[nr_phantom_devs++] = phantom;
>>
> 

Do you prefer this?

case 1: case 2: case 4:
     if ( *s )
         fallthrough;
     break;
default:
     return -EINVAL;


-- 
Federico Serafini, M.Sc.

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

Re: [XEN PATCH 12/12] xen/pci: address a violation of MISRA C Rule 16.3

[Federico Serafini]

On 10/09/24 19:41, Federico Serafini wrote:
> On 10/09/24 16:59, Jan Beulich wrote:
>> On 10.09.2024 16:57, Jan Beulich wrote:
>>> On 10.09.2024 12:09, Federico Serafini wrote:
>>>> Address violations of MISRA C:2012 Rule 16.3:
>>>> "An unconditional `break' statement shall terminate every
>>>> switch-clause".
>>>
>>> Since in our interpretation "return" is okay too, why is not 
>>> sufficient to
>>> simply ...
>>>
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -170,8 +170,10 @@ static int __init cf_check 
>>>> parse_phantom_dev(const char *str)
>>>>       {
>>>>       case 1: case 2: case 4:
>>>>           if ( *s )
>>>> -    default:
>>>>               return -EINVAL;
>>>> +        break;
>>>
>>> ... insert just this one line here?
>>
>> I guess the problem is with the description: It's un-annotated 
>> fall-through
>> in this case, not so much the lack of a break (or alike).
>>
>> Jan
>>
>>>> +    default:
>>>> +        return -EINVAL;
>>>>       }
>>>>       phantom_devs[nr_phantom_devs++] = phantom;
>>>
>>
> 
> Do you prefer this?
> 
> case 1: case 2: case 4:
>      if ( *s )
>          fallthrough;
>      break;
> default:
>      return -EINVAL;
> 
> 

Oh no, sorry,
this does not make sense.

-- 
Federico Serafini, M.Sc.

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

Re: [XEN PATCH 12/12] xen/pci: address a violation of MISRA C Rule 16.3

[Jan Beulich]

On 10.09.2024 19:46, Federico Serafini wrote:
> On 10/09/24 19:41, Federico Serafini wrote:
>> On 10/09/24 16:59, Jan Beulich wrote:
>>> On 10.09.2024 16:57, Jan Beulich wrote:
>>>> On 10.09.2024 12:09, Federico Serafini wrote:
>>>>> Address violations of MISRA C:2012 Rule 16.3:
>>>>> "An unconditional `break' statement shall terminate every
>>>>> switch-clause".
>>>>
>>>> Since in our interpretation "return" is okay too, why is not 
>>>> sufficient to
>>>> simply ...
>>>>
>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>> @@ -170,8 +170,10 @@ static int __init cf_check 
>>>>> parse_phantom_dev(const char *str)
>>>>>       {
>>>>>       case 1: case 2: case 4:
>>>>>           if ( *s )
>>>>> -    default:
>>>>>               return -EINVAL;
>>>>> +        break;
>>>>
>>>> ... insert just this one line here?
>>>
>>> I guess the problem is with the description: It's un-annotated 
>>> fall-through
>>> in this case, not so much the lack of a break (or alike).
>>>
>>> Jan
>>>
>>>>> +    default:
>>>>> +        return -EINVAL;
>>>>>       }
>>>>>       phantom_devs[nr_phantom_devs++] = phantom;
>>>>
>>>
>>
>> Do you prefer this?
>>
>> case 1: case 2: case 4:
>>      if ( *s )
>>          fallthrough;
>>      break;
>> default:
>>      return -EINVAL;
> 
> Oh no, sorry,
> this does not make sense.

First of all I'd prefer if we could leave such untouched. Moving to the
obvious replacement of what's there 

case 1: case 2: case 4:
     if ( *s )
     {
         fallthrough;
default:
         return -EINVAL;
     }
     break;

is arguably not necessarily better than what you were proposing. So I
guess the conclusion is that you're code change is okay(ish) (i.e. if
already we need to touch this), but the description wants changing to
make clear what the problem here is.

Jan