[XEN PATCH v2 00/13] x86: address some violations of MISRA C Rule 16.3

[XEN PATCH v2 00/13] x86: address some violations of MISRA C Rule 16.3

[Federico Serafini]

This patch series fixes a missing escape in a deviation and addresses some
violations.

Federico Serafini (13):
  automation/eclair: fix deviation of MISRA C Rule 16.3
  x86/cpuid: use fallthrough pseudo keyword
  x86/domctl: address a violation of MISRA C Rule 16.3
  x86/vpmu: address violations of MISRA C Rule 16.3
  x86/traps: address violations of MISRA C Rule 16.3
  x86/mce: address violations of MISRA C Rule 16.3
  x86/hvm: address violations of MISRA C Rule 16.3
  x86/vpt: address a violation of MISRA C Rule 16.3
  x86/mm: add defensive return
  x86/mpparse: address a violation of MISRA C Rule 16.3
  x86/pmtimer: address a violation of MISRA C Rule 16.3
  x86/vPIC: address a violation of MISRA C Rule 16.3
  x86/vlapic: address a violation of MISRA C Rule 16.3

 automation/eclair_analysis/ECLAIR/deviations.ecl | 2 +-
 xen/arch/x86/cpu/mcheck/mce_amd.c                | 1 +
 xen/arch/x86/cpu/mcheck/mce_intel.c              | 2 ++
 xen/arch/x86/cpu/vpmu.c                          | 3 +++
 xen/arch/x86/cpu/vpmu_intel.c                    | 1 +
 xen/arch/x86/cpuid.c                             | 3 +--
 xen/arch/x86/domctl.c                            | 1 +
 xen/arch/x86/hvm/emulate.c                       | 9 ++++++---
 xen/arch/x86/hvm/hvm.c                           | 6 ++++++
 xen/arch/x86/hvm/hypercall.c                     | 1 +
 xen/arch/x86/hvm/irq.c                           | 1 +
 xen/arch/x86/hvm/pmtimer.c                       | 1 +
 xen/arch/x86/hvm/vlapic.c                        | 1 +
 xen/arch/x86/hvm/vpic.c                          | 1 +
 xen/arch/x86/hvm/vpt.c                           | 2 ++
 xen/arch/x86/mm.c                                | 1 +
 xen/arch/x86/mpparse.c                           | 1 +
 xen/arch/x86/traps.c                             | 3 +++
 18 files changed, 34 insertions(+), 6 deletions(-)

-- 
2.34.1

[XEN PATCH v2 01/13] automation/eclair: fix deviation of MISRA C Rule 16.3

[Federico Serafini]

Escape the final dot of the comment and extend the search of a
fallthrough comment up to 2 lines after the last statement.

Fixes: a128d8da913b21eff6c6d2e2a7d4c54c054b78db "automation/eclair: add deviations for MISRA C:2012 Rule 16.3"
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
Changes in v2:
- instead of introducing the hypened fallthrough, insert the missing escape.
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index b8f9155267..9df3e0f0c4 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -400,7 +400,7 @@ safe."
 -doc_end
 
 -doc_begin="Switch clauses ending with an explicit comment indicating the fallthrough intention are safe."
--config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(^(?s).*/\\* [fF]all ?through.? \\*/.*$,0..1))))"}
+-config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(^(?s).*/\\* [fF]all ?through\\.? \\*/.*$,0..2))))"}
 -doc_end
 
 -doc_begin="Switch statements having a controlling expression of enum type deliberately do not have a default case: gcc -Wall enables -Wswitch which warns (and breaks the build as we use -Werror) if one of the enum labels is missing from the switch."
-- 
2.34.1

Re: [XEN PATCH v2 01/13] automation/eclair: fix deviation of MISRA C Rule 16.3

[Jan Beulich]

On 24.06.2024 11:04, Federico Serafini wrote:
> Escape the final dot of the comment and extend the search of a
> fallthrough comment up to 2 lines after the last statement.
> 
> Fixes: a128d8da913b21eff6c6d2e2a7d4c54c054b78db "automation/eclair: add deviations for MISRA C:2012 Rule 16.3"

Nit: Yes, the respective doc says "at least 12 digits", but please also keep
at that going forward.

Jan

Re: [XEN PATCH v2 01/13] automation/eclair: fix deviation of MISRA C Rule 16.3

[Federico Serafini]

On 24/06/24 12:57, Jan Beulich wrote:
> On 24.06.2024 11:04, Federico Serafini wrote:
>> Escape the final dot of the comment and extend the search of a
>> fallthrough comment up to 2 lines after the last statement.
>>
>> Fixes: a128d8da913b21eff6c6d2e2a7d4c54c054b78db "automation/eclair: add deviations for MISRA C:2012 Rule 16.3"
> 
> Nit: Yes, the respective doc says "at least 12 digits", but please also keep
> at that going forward.

Noted.

-- 
Federico Serafini, M.Sc.

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

Re: [XEN PATCH v2 01/13] automation/eclair: fix deviation of MISRA C Rule 16.3

[Stefano Stabellini]

On Mon, 24 Jun 2024, Federico Serafini wrote:
> Escape the final dot of the comment and extend the search of a
> fallthrough comment up to 2 lines after the last statement.
> 
> Fixes: a128d8da913b21eff6c6d2e2a7d4c54c054b78db "automation/eclair: add deviations for MISRA C:2012 Rule 16.3"
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

[XEN PATCH v2 02/13] x86/cpuid: use fallthrough pseudo keyword

[Federico Serafini]

The current comment making explicit the fallthrough intention does
not follow the agreed syntax: replace it with the pseduo keyword.

No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/x86/cpuid.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index a822e80c7e..2a777436ee 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -97,9 +97,8 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
         if ( is_viridian_domain(d) )
             return cpuid_viridian_leaves(v, leaf, subleaf, res);
 
+        fallthrough;
         /*
-         * Fallthrough.
-         *
          * Intel reserve up until 0x4fffffff for hypervisor use.  AMD reserve
          * only until 0x400000ff, but we already use double that.
          */
-- 
2.34.1

Re: [XEN PATCH v2 02/13] x86/cpuid: use fallthrough pseudo keyword

[Jan Beulich]

On 24.06.2024 11:04, Federico Serafini wrote:
> The current comment making explicit the fallthrough intention does
> not follow the agreed syntax: replace it with the pseduo keyword.
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

Re: [XEN PATCH v2 02/13] x86/cpuid: use fallthrough pseudo keyword

[Stefano Stabellini]

On Mon, 24 Jun 2024, Federico Serafini wrote:
> The current comment making explicit the fallthrough intention does
> not follow the agreed syntax: replace it with the pseduo keyword.
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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


> ---
>  xen/arch/x86/cpuid.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
> index a822e80c7e..2a777436ee 100644
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -97,9 +97,8 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>          if ( is_viridian_domain(d) )
>              return cpuid_viridian_leaves(v, leaf, subleaf, res);
>  
> +        fallthrough;
>          /*
> -         * Fallthrough.
> -         *
>           * Intel reserve up until 0x4fffffff for hypervisor use.  AMD reserve
>           * only until 0x400000ff, but we already use double that.
>           */
> -- 
> 2.34.1
> 
> 

[XEN PATCH v2 03/13] x86/domctl: address a violation of MISRA C Rule 16.3

[Federico Serafini]

Add missing break statement to address a violation of
MISRA C 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/domctl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9190e11faa..68b5b46d1a 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -517,6 +517,7 @@ long arch_do_domctl(
 
         default:
             ret = -ENOSYS;
+            break;
         }
         break;
     }
-- 
2.34.1

Re: [XEN PATCH v2 03/13] x86/domctl: address a violation of MISRA C Rule 16.3

[Jan Beulich]

On 24.06.2024 11:04, Federico Serafini wrote:
> Add missing break statement to address a violation of
> MISRA C 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>

Re: [XEN PATCH v2 03/13] x86/domctl: address a violation of MISRA C Rule 16.3

[Stefano Stabellini]

On Mon, 24 Jun 2024, Federico Serafini wrote:
> Add missing break statement to address a violation of
> MISRA C Rule 16.3: "An unconditional `break' statement shall terminate
> every switch-clause".
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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


> ---
>  xen/arch/x86/domctl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 9190e11faa..68b5b46d1a 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -517,6 +517,7 @@ long arch_do_domctl(
>  
>          default:
>              ret = -ENOSYS;
> +            break;
>          }
>          break;
>      }
> -- 
> 2.34.1
> 
> 

[XEN PATCH v2 04/13] x86/vpmu: address violations of MISRA C Rule 16.3

[Federico Serafini]

Add missing break statements to address violations of MISRA C 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/cpu/vpmu.c       | 3 +++
 xen/arch/x86/cpu/vpmu_intel.c | 1 +
 2 files changed, 4 insertions(+)

diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index a7bc0cd1fc..b2ba999412 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -663,6 +663,8 @@ long do_xenpmu_op(
 
         if ( pmu_params.version.maj != XENPMU_VER_MAJ )
             return -EINVAL;
+
+        break;
     }
 
     switch ( op )
@@ -776,6 +778,7 @@ long do_xenpmu_op(
 
     default:
         ret = -EINVAL;
+        break;
     }
 
     return ret;
diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index cd414165df..46f3ff86e7 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -713,6 +713,7 @@ static int cf_check core2_vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content)
             break;
         default:
             rdmsrl(msr, *msr_content);
+            break;
         }
     }
     else if ( msr == MSR_IA32_MISC_ENABLE )
-- 
2.34.1

Re: [XEN PATCH v2 04/13] x86/vpmu: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 24.06.2024 11:04, Federico Serafini wrote:
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -713,6 +713,7 @@ static int cf_check core2_vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content)
>              break;
>          default:
>              rdmsrl(msr, *msr_content);
> +            break;
>          }
>      }
>      else if ( msr == MSR_IA32_MISC_ENABLE )

Up from here, in core2_vpmu_do_wrmsr() there's a pretty long default
block with no terminating break. Is there a reason that you don't put
one there?

Jan

Re: [XEN PATCH v2 04/13] x86/vpmu: address violations of MISRA C Rule 16.3

[Federico Serafini]

On 24/06/24 17:16, Jan Beulich wrote:
> On 24.06.2024 11:04, Federico Serafini wrote:
>> --- a/xen/arch/x86/cpu/vpmu_intel.c
>> +++ b/xen/arch/x86/cpu/vpmu_intel.c
>> @@ -713,6 +713,7 @@ static int cf_check core2_vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content)
>>               break;
>>           default:
>>               rdmsrl(msr, *msr_content);
>> +            break;
>>           }
>>       }
>>       else if ( msr == MSR_IA32_MISC_ENABLE )
> 
> Up from here, in core2_vpmu_do_wrmsr() there's a pretty long default
> block with no terminating break. Is there a reason that you don't put
> one there?

I noticed that vpmu_intel.c is a file out-of-scope.
The violation I addressed was shown because it involves
the macro rdmsrl coming from file xen/arch/x86/include/asm/msr.h
(in scope).
I will address also the violation you pointed out in a v3.

-- 
Federico Serafini, M.Sc.

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

Re: [XEN PATCH v2 04/13] x86/vpmu: address violations of MISRA C Rule 16.3

[Stefano Stabellini]

On Mon, 24 Jun 2024, Federico Serafini wrote:
> Add missing break statements to address violations of MISRA C Rule
> 16.3: "An unconditional `break' statement shall terminate every
> switch-clause".
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

[XEN PATCH v2 05/13] x86/traps: address violations of MISRA C Rule 16.3

[Federico Serafini]

Add break or pseudo keyword fallthrough to address violations of
MISRA C 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/traps.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 9906e874d5..cbcec3fafb 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1186,6 +1186,7 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
 
     default:
         ASSERT_UNREACHABLE();
+        break;
     }
 }
 
@@ -1748,6 +1749,7 @@ static void io_check_error(const struct cpu_user_regs *regs)
     {
     case 'd': /* 'dom0' */
         nmi_hwdom_report(_XEN_NMIREASON_io_error);
+        fallthrough;
     case 'i': /* 'ignore' */
         break;
     default:  /* 'fatal' */
@@ -1768,6 +1770,7 @@ static void unknown_nmi_error(const struct cpu_user_regs *regs,
     {
     case 'd': /* 'dom0' */
         nmi_hwdom_report(_XEN_NMIREASON_unknown);
+        fallthrough;
     case 'i': /* 'ignore' */
         break;
     default:  /* 'fatal' */
-- 
2.34.1

Re: [XEN PATCH v2 05/13] x86/traps: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 24.06.2024 11:04, Federico Serafini wrote:
> Add break or pseudo keyword fallthrough to address violations of
> MISRA C Rule 16.3: "An unconditional `break' statement shall terminate
> every switch-clause".
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Technically the change fulfills its purpose, yet:

> @@ -1748,6 +1749,7 @@ static void io_check_error(const struct cpu_user_regs *regs)
>      {
>      case 'd': /* 'dom0' */
>          nmi_hwdom_report(_XEN_NMIREASON_io_error);
> +        fallthrough;
>      case 'i': /* 'ignore' */
>          break;
>      default:  /* 'fatal' */
> @@ -1768,6 +1770,7 @@ static void unknown_nmi_error(const struct cpu_user_regs *regs,
>      {
>      case 'd': /* 'dom0' */
>          nmi_hwdom_report(_XEN_NMIREASON_unknown);
> +        fallthrough;
>      case 'i': /* 'ignore' */
>          break;
>      default:  /* 'fatal' */

Falling through isn't really useful here. In such a case I think it would
be preferable to avoid the pseudo-keyword and use the shorter "break".

Jan

Re: [XEN PATCH v2 05/13] x86/traps: address violations of MISRA C Rule 16.3

[Stefano Stabellini]

On Mon, 24 Jun 2024, Federico Serafini wrote:
> Add break or pseudo keyword fallthrough to address violations of
> MISRA C 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/traps.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 9906e874d5..cbcec3fafb 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1186,6 +1186,7 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
>  
>      default:
>          ASSERT_UNREACHABLE();
> +        break;

Please add ASSERT_UNREACHABLE to the list of "unconditional flow control
statements" that can terminate a case, in addition to break.


>      }
>  }
>  
> @@ -1748,6 +1749,7 @@ static void io_check_error(const struct cpu_user_regs *regs)
>      {
>      case 'd': /* 'dom0' */
>          nmi_hwdom_report(_XEN_NMIREASON_io_error);
> +        fallthrough;
>      case 'i': /* 'ignore' */
>          break;
>      default:  /* 'fatal' */
> @@ -1768,6 +1770,7 @@ static void unknown_nmi_error(const struct cpu_user_regs *regs,
>      {
>      case 'd': /* 'dom0' */
>          nmi_hwdom_report(_XEN_NMIREASON_unknown);
> +        fallthrough;
>      case 'i': /* 'ignore' */
>          break;
>      default:  /* 'fatal' */

These two are nice improvements

Re: [XEN PATCH v2 05/13] x86/traps: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 25.06.2024 02:54, Stefano Stabellini wrote:
> On Mon, 24 Jun 2024, Federico Serafini wrote:
>> Add break or pseudo keyword fallthrough to address violations of
>> MISRA C 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/traps.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index 9906e874d5..cbcec3fafb 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1186,6 +1186,7 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
>>  
>>      default:
>>          ASSERT_UNREACHABLE();
>> +        break;
> 
> Please add ASSERT_UNREACHABLE to the list of "unconditional flow control
> statements" that can terminate a case, in addition to break.

Why? Exactly the opposite is part of the subject of a recent patch, iirc.
Simply because of the rules needing to cover both debug and release builds.

Jan

Re: [XEN PATCH v2 05/13] x86/traps: address violations of MISRA C Rule 16.3

[Stefano Stabellini]

On Tue, 25 Jun 2024, Jan Beulich wrote:
> On 25.06.2024 02:54, Stefano Stabellini wrote:
> > On Mon, 24 Jun 2024, Federico Serafini wrote:
> >> Add break or pseudo keyword fallthrough to address violations of
> >> MISRA C 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/traps.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> >> index 9906e874d5..cbcec3fafb 100644
> >> --- a/xen/arch/x86/traps.c
> >> +++ b/xen/arch/x86/traps.c
> >> @@ -1186,6 +1186,7 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
> >>  
> >>      default:
> >>          ASSERT_UNREACHABLE();
> >> +        break;
> > 
> > Please add ASSERT_UNREACHABLE to the list of "unconditional flow control
> > statements" that can terminate a case, in addition to break.
> 
> Why? Exactly the opposite is part of the subject of a recent patch, iirc.
> Simply because of the rules needing to cover both debug and release builds.

The reason is that ASSERT_UNREACHABLE() might disappear from the release
build but it can still be used as a marker during static analysis. In
my view, ASSERT_UNREACHABLE() is equivalent to a noreturn function call
which has an empty implementation in release builds.

The only reason I can think of to require a break; after an
ASSERT_UNREACHABLE() would be if we think the unreachability only apply
to debug build, not release build:

- debug build: it is unreachable
- release build: it is reachable

I don't think that is meant to be possible so I think we can use
ASSERT_UNREACHABLE() as a marker.

Re: [XEN PATCH v2 05/13] x86/traps: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 26.06.2024 03:11, Stefano Stabellini wrote:
> On Tue, 25 Jun 2024, Jan Beulich wrote:
>> On 25.06.2024 02:54, Stefano Stabellini wrote:
>>> On Mon, 24 Jun 2024, Federico Serafini wrote:
>>>> Add break or pseudo keyword fallthrough to address violations of
>>>> MISRA C 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/traps.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>>>> index 9906e874d5..cbcec3fafb 100644
>>>> --- a/xen/arch/x86/traps.c
>>>> +++ b/xen/arch/x86/traps.c
>>>> @@ -1186,6 +1186,7 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
>>>>  
>>>>      default:
>>>>          ASSERT_UNREACHABLE();
>>>> +        break;
>>>
>>> Please add ASSERT_UNREACHABLE to the list of "unconditional flow control
>>> statements" that can terminate a case, in addition to break.
>>
>> Why? Exactly the opposite is part of the subject of a recent patch, iirc.
>> Simply because of the rules needing to cover both debug and release builds.
> 
> The reason is that ASSERT_UNREACHABLE() might disappear from the release
> build but it can still be used as a marker during static analysis. In
> my view, ASSERT_UNREACHABLE() is equivalent to a noreturn function call
> which has an empty implementation in release builds.
> 
> The only reason I can think of to require a break; after an
> ASSERT_UNREACHABLE() would be if we think the unreachability only apply
> to debug build, not release build:
> 
> - debug build: it is unreachable
> - release build: it is reachable
> 
> I don't think that is meant to be possible so I think we can use
> ASSERT_UNREACHABLE() as a marker.

Well. For one such an assumption takes as a prereq that a debug build will
be run through full coverage testing, i.e. all reachable paths proven to
be taken. I understand that this prereq is intended to somehow be met,
even if I'm having difficulty seeing how such a final proof would look
like: Full coverage would, to me, mean that _every_ line is reachable. Yet
clearly any ASSERT_UNREACHABLE() must never be reached.

And then not covering for such cases takes the further assumption that
debug and release builds are functionally identical. I'm afraid this would
be a wrong assumption to make:
1) We may screw up somewhere, with code wrongly enabled only in one of the
   two build modes.
2) The compiler may screw up, in particular with optimization.

Jan

Re: [XEN PATCH v2 05/13] x86/traps: address violations of MISRA C Rule 16.3

[Stefano Stabellini]

On Wed, 26 Jun 2024, Jan Beulich wrote:
> On 26.06.2024 03:11, Stefano Stabellini wrote:
> > On Tue, 25 Jun 2024, Jan Beulich wrote:
> >> On 25.06.2024 02:54, Stefano Stabellini wrote:
> >>> On Mon, 24 Jun 2024, Federico Serafini wrote:
> >>>> Add break or pseudo keyword fallthrough to address violations of
> >>>> MISRA C 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/traps.c | 3 +++
> >>>>  1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> >>>> index 9906e874d5..cbcec3fafb 100644
> >>>> --- a/xen/arch/x86/traps.c
> >>>> +++ b/xen/arch/x86/traps.c
> >>>> @@ -1186,6 +1186,7 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
> >>>>  
> >>>>      default:
> >>>>          ASSERT_UNREACHABLE();
> >>>> +        break;
> >>>
> >>> Please add ASSERT_UNREACHABLE to the list of "unconditional flow control
> >>> statements" that can terminate a case, in addition to break.
> >>
> >> Why? Exactly the opposite is part of the subject of a recent patch, iirc.
> >> Simply because of the rules needing to cover both debug and release builds.
> > 
> > The reason is that ASSERT_UNREACHABLE() might disappear from the release
> > build but it can still be used as a marker during static analysis. In
> > my view, ASSERT_UNREACHABLE() is equivalent to a noreturn function call
> > which has an empty implementation in release builds.
> > 
> > The only reason I can think of to require a break; after an
> > ASSERT_UNREACHABLE() would be if we think the unreachability only apply
> > to debug build, not release build:
> > 
> > - debug build: it is unreachable
> > - release build: it is reachable
> > 
> > I don't think that is meant to be possible so I think we can use
> > ASSERT_UNREACHABLE() as a marker.
> 
> Well. For one such an assumption takes as a prereq that a debug build will
> be run through full coverage testing, i.e. all reachable paths proven to
> be taken. I understand that this prereq is intended to somehow be met,
> even if I'm having difficulty seeing how such a final proof would look
> like: Full coverage would, to me, mean that _every_ line is reachable. Yet
> clearly any ASSERT_UNREACHABLE() must never be reached.
> 
> And then not covering for such cases takes the further assumption that
> debug and release builds are functionally identical. I'm afraid this would
> be a wrong assumption to make:
> 1) We may screw up somewhere, with code wrongly enabled only in one of the
>    two build modes.
> 2) The compiler may screw up, in particular with optimization.

I think there are two different issues here we are discussing.

One issue, like you said, has to do with coverage. It is important to
mark as "unreachable" any part of the code that is indeed unreachable
so that we can account it properly when we do coverage analysis. At the
moment the only "unreachable" marker that we have is
ASSERT_UNREACHABLE(), and I am hoping we can use it as part of the
coverage analysis we'll do.

However, there is a different separate question about what to do in the
Xen code after an ASSERT_UNREACHABLE(). E.g.:

             default:
                 ASSERT_UNREACHABLE();
                 return -EPERM; /* is it better with or without this? */
             }

Leaving coverage aside, would it be better to be defensive and actually
attempt to report errors back after an ASSERT_UNREACHABLE() like in the
example? Or is it better to assume the code is actually unreachable
hence there is no need to do anything afterwards?

One one hand, being defensive sounds good, on the other hand, any code
we add after ASSERT_UNREACHABLE() is dead code which cannot be tested,
which is also not good. In this example, there is no way to test the
return -EPERM code path. We also need to consider what is the right
thing to do if Xen finds itself in an erroneous situation such as being
in an unreachable code location.

So, after thinking about it and also talking to the safety manager, I
think we should:
- implement ASSERT_UNREACHABLE with a warning in release builds
- have "return -EPERM;" or similar for defensive programming

Re: [XEN PATCH v2 05/13] x86/traps: address violations of MISRA C Rule 16.3

[Stefano Stabellini]

On Wed, 26 Jun 2024, Stefano Stabellini wrote:
> So, after thinking about it and also talking to the safety manager, I
> think we should:
> - implement ASSERT_UNREACHABLE with a warning in release builds
> - have "return -EPERM;" or similar for defensive programming

Federico, as Jan agrees already on the second point, then I withdraw all
my comments about code after ASSERT_UNREACHABLE (you can consider your
R16.3 patches with my acks fully acked).

Re: [XEN PATCH v2 05/13] x86/traps: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 27.06.2024 03:53, Stefano Stabellini wrote:
> On Wed, 26 Jun 2024, Jan Beulich wrote:
>> On 26.06.2024 03:11, Stefano Stabellini wrote:
>>> On Tue, 25 Jun 2024, Jan Beulich wrote:
>>>> On 25.06.2024 02:54, Stefano Stabellini wrote:
>>>>> On Mon, 24 Jun 2024, Federico Serafini wrote:
>>>>>> Add break or pseudo keyword fallthrough to address violations of
>>>>>> MISRA C 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/traps.c | 3 +++
>>>>>>  1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>>>>>> index 9906e874d5..cbcec3fafb 100644
>>>>>> --- a/xen/arch/x86/traps.c
>>>>>> +++ b/xen/arch/x86/traps.c
>>>>>> @@ -1186,6 +1186,7 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
>>>>>>  
>>>>>>      default:
>>>>>>          ASSERT_UNREACHABLE();
>>>>>> +        break;
>>>>>
>>>>> Please add ASSERT_UNREACHABLE to the list of "unconditional flow control
>>>>> statements" that can terminate a case, in addition to break.
>>>>
>>>> Why? Exactly the opposite is part of the subject of a recent patch, iirc.
>>>> Simply because of the rules needing to cover both debug and release builds.
>>>
>>> The reason is that ASSERT_UNREACHABLE() might disappear from the release
>>> build but it can still be used as a marker during static analysis. In
>>> my view, ASSERT_UNREACHABLE() is equivalent to a noreturn function call
>>> which has an empty implementation in release builds.
>>>
>>> The only reason I can think of to require a break; after an
>>> ASSERT_UNREACHABLE() would be if we think the unreachability only apply
>>> to debug build, not release build:
>>>
>>> - debug build: it is unreachable
>>> - release build: it is reachable
>>>
>>> I don't think that is meant to be possible so I think we can use
>>> ASSERT_UNREACHABLE() as a marker.
>>
>> Well. For one such an assumption takes as a prereq that a debug build will
>> be run through full coverage testing, i.e. all reachable paths proven to
>> be taken. I understand that this prereq is intended to somehow be met,
>> even if I'm having difficulty seeing how such a final proof would look
>> like: Full coverage would, to me, mean that _every_ line is reachable. Yet
>> clearly any ASSERT_UNREACHABLE() must never be reached.
>>
>> And then not covering for such cases takes the further assumption that
>> debug and release builds are functionally identical. I'm afraid this would
>> be a wrong assumption to make:
>> 1) We may screw up somewhere, with code wrongly enabled only in one of the
>>    two build modes.
>> 2) The compiler may screw up, in particular with optimization.
> 
> I think there are two different issues here we are discussing.
> 
> One issue, like you said, has to do with coverage. It is important to
> mark as "unreachable" any part of the code that is indeed unreachable
> so that we can account it properly when we do coverage analysis. At the
> moment the only "unreachable" marker that we have is
> ASSERT_UNREACHABLE(), and I am hoping we can use it as part of the
> coverage analysis we'll do.
> 
> However, there is a different separate question about what to do in the
> Xen code after an ASSERT_UNREACHABLE(). E.g.:
> 
>              default:
>                  ASSERT_UNREACHABLE();
>                  return -EPERM; /* is it better with or without this? */
>              }
> 
> Leaving coverage aside, would it be better to be defensive and actually
> attempt to report errors back after an ASSERT_UNREACHABLE() like in the
> example? Or is it better to assume the code is actually unreachable
> hence there is no need to do anything afterwards?
> 
> One one hand, being defensive sounds good, on the other hand, any code
> we add after ASSERT_UNREACHABLE() is dead code which cannot be tested,
> which is also not good. In this example, there is no way to test the
> return -EPERM code path. We also need to consider what is the right
> thing to do if Xen finds itself in an erroneous situation such as being
> in an unreachable code location.
> 
> So, after thinking about it and also talking to the safety manager, I
> think we should:
> - implement ASSERT_UNREACHABLE with a warning in release builds

If at all, then controlled by a default-off Kconfig setting. This would,
after all, raise the question of how ASSERT() should behave then. Imo
the two should be consistent in this regard, and NDEBUG clearly results
in the expectation that ASSERT() expands to nothing. Perhaps this is
finally the time where we need to separate NDEBUG from CONFIG_DEBUG; we
did discuss doing so before. Then in your release builds, you could
actually leave assertions active.

> - have "return -EPERM;" or similar for defensive programming

You don't say how you'd deal with the not-reachable aspect then.

Jan

Re: [XEN PATCH v2 05/13] x86/traps: address violations of MISRA C Rule 16.3

[Stefano Stabellini]

On Thu, 27 Jun 2024, Jan Beulich wrote:
> On 27.06.2024 03:53, Stefano Stabellini wrote:
> > On Wed, 26 Jun 2024, Jan Beulich wrote:
> >> On 26.06.2024 03:11, Stefano Stabellini wrote:
> >>> On Tue, 25 Jun 2024, Jan Beulich wrote:
> >>>> On 25.06.2024 02:54, Stefano Stabellini wrote:
> >>>>> On Mon, 24 Jun 2024, Federico Serafini wrote:
> >>>>>> Add break or pseudo keyword fallthrough to address violations of
> >>>>>> MISRA C 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/traps.c | 3 +++
> >>>>>>  1 file changed, 3 insertions(+)
> >>>>>>
> >>>>>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> >>>>>> index 9906e874d5..cbcec3fafb 100644
> >>>>>> --- a/xen/arch/x86/traps.c
> >>>>>> +++ b/xen/arch/x86/traps.c
> >>>>>> @@ -1186,6 +1186,7 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
> >>>>>>  
> >>>>>>      default:
> >>>>>>          ASSERT_UNREACHABLE();
> >>>>>> +        break;
> >>>>>
> >>>>> Please add ASSERT_UNREACHABLE to the list of "unconditional flow control
> >>>>> statements" that can terminate a case, in addition to break.
> >>>>
> >>>> Why? Exactly the opposite is part of the subject of a recent patch, iirc.
> >>>> Simply because of the rules needing to cover both debug and release builds.
> >>>
> >>> The reason is that ASSERT_UNREACHABLE() might disappear from the release
> >>> build but it can still be used as a marker during static analysis. In
> >>> my view, ASSERT_UNREACHABLE() is equivalent to a noreturn function call
> >>> which has an empty implementation in release builds.
> >>>
> >>> The only reason I can think of to require a break; after an
> >>> ASSERT_UNREACHABLE() would be if we think the unreachability only apply
> >>> to debug build, not release build:
> >>>
> >>> - debug build: it is unreachable
> >>> - release build: it is reachable
> >>>
> >>> I don't think that is meant to be possible so I think we can use
> >>> ASSERT_UNREACHABLE() as a marker.
> >>
> >> Well. For one such an assumption takes as a prereq that a debug build will
> >> be run through full coverage testing, i.e. all reachable paths proven to
> >> be taken. I understand that this prereq is intended to somehow be met,
> >> even if I'm having difficulty seeing how such a final proof would look
> >> like: Full coverage would, to me, mean that _every_ line is reachable. Yet
> >> clearly any ASSERT_UNREACHABLE() must never be reached.
> >>
> >> And then not covering for such cases takes the further assumption that
> >> debug and release builds are functionally identical. I'm afraid this would
> >> be a wrong assumption to make:
> >> 1) We may screw up somewhere, with code wrongly enabled only in one of the
> >>    two build modes.
> >> 2) The compiler may screw up, in particular with optimization.
> > 
> > I think there are two different issues here we are discussing.
> > 
> > One issue, like you said, has to do with coverage. It is important to
> > mark as "unreachable" any part of the code that is indeed unreachable
> > so that we can account it properly when we do coverage analysis. At the
> > moment the only "unreachable" marker that we have is
> > ASSERT_UNREACHABLE(), and I am hoping we can use it as part of the
> > coverage analysis we'll do.
> > 
> > However, there is a different separate question about what to do in the
> > Xen code after an ASSERT_UNREACHABLE(). E.g.:
> > 
> >              default:
> >                  ASSERT_UNREACHABLE();
> >                  return -EPERM; /* is it better with or without this? */
> >              }
> > 
> > Leaving coverage aside, would it be better to be defensive and actually
> > attempt to report errors back after an ASSERT_UNREACHABLE() like in the
> > example? Or is it better to assume the code is actually unreachable
> > hence there is no need to do anything afterwards?
> > 
> > One one hand, being defensive sounds good, on the other hand, any code
> > we add after ASSERT_UNREACHABLE() is dead code which cannot be tested,
> > which is also not good. In this example, there is no way to test the
> > return -EPERM code path. We also need to consider what is the right
> > thing to do if Xen finds itself in an erroneous situation such as being
> > in an unreachable code location.
> > 
> > So, after thinking about it and also talking to the safety manager, I
> > think we should:
> > - implement ASSERT_UNREACHABLE with a warning in release builds
> 
> If at all, then controlled by a default-off Kconfig setting. This would,
> after all, raise the question of how ASSERT() should behave then. Imo
> the two should be consistent in this regard, and NDEBUG clearly results
> in the expectation that ASSERT() expands to nothing. Perhaps this is
> finally the time where we need to separate NDEBUG from CONFIG_DEBUG; we
> did discuss doing so before. Then in your release builds, you could
> actually leave assertions active.
 
Yes, a kconfig to define the behavior of ASSERT_UNREACHABLE in release
builds is fine. And you are right that we should consider doing
something similar for ASSERT too.

I think that in any environment where safety (i.e. correctness of
behavior) is a primary concern, attempting to continue without doing
anything after reaching a point that is supposed to be unreachable is
not a good idea.

I think Xen should do something in response to reaching a point it is
not supposed to reach. I don't know yet what is the best course of
action but printing a warning seems to be the bare minimum.

Crashing the system is not a good idea as it could potentially be
exploited by malicious guests (security might not be the primary concern
but still.)


> > - have "return -EPERM;" or similar for defensive programming
> 
> You don't say how you'd deal with the not-reachable aspect then.

We'll have to find a way to account for all the code that cannot be
tested. We would have a problem anyway due to the ASSERT_UNREACHABLE
checks, but the addition of "return -EPERM;" will make things slightly
worse.

I have been told to prioritize safety of the code and defensive
programming over coverage calculations.

Re: [XEN PATCH v2 05/13] x86/traps: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 28.06.2024 00:59, Stefano Stabellini wrote:
> On Thu, 27 Jun 2024, Jan Beulich wrote:
>> On 27.06.2024 03:53, Stefano Stabellini wrote:
>>> On Wed, 26 Jun 2024, Jan Beulich wrote:
>>>> On 26.06.2024 03:11, Stefano Stabellini wrote:
>>>>> On Tue, 25 Jun 2024, Jan Beulich wrote:
>>>>>> On 25.06.2024 02:54, Stefano Stabellini wrote:
>>>>>>> On Mon, 24 Jun 2024, Federico Serafini wrote:
>>>>>>>> Add break or pseudo keyword fallthrough to address violations of
>>>>>>>> MISRA C 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/traps.c | 3 +++
>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>>>>>>>> index 9906e874d5..cbcec3fafb 100644
>>>>>>>> --- a/xen/arch/x86/traps.c
>>>>>>>> +++ b/xen/arch/x86/traps.c
>>>>>>>> @@ -1186,6 +1186,7 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
>>>>>>>>  
>>>>>>>>      default:
>>>>>>>>          ASSERT_UNREACHABLE();
>>>>>>>> +        break;
>>>>>>>
>>>>>>> Please add ASSERT_UNREACHABLE to the list of "unconditional flow control
>>>>>>> statements" that can terminate a case, in addition to break.
>>>>>>
>>>>>> Why? Exactly the opposite is part of the subject of a recent patch, iirc.
>>>>>> Simply because of the rules needing to cover both debug and release builds.
>>>>>
>>>>> The reason is that ASSERT_UNREACHABLE() might disappear from the release
>>>>> build but it can still be used as a marker during static analysis. In
>>>>> my view, ASSERT_UNREACHABLE() is equivalent to a noreturn function call
>>>>> which has an empty implementation in release builds.
>>>>>
>>>>> The only reason I can think of to require a break; after an
>>>>> ASSERT_UNREACHABLE() would be if we think the unreachability only apply
>>>>> to debug build, not release build:
>>>>>
>>>>> - debug build: it is unreachable
>>>>> - release build: it is reachable
>>>>>
>>>>> I don't think that is meant to be possible so I think we can use
>>>>> ASSERT_UNREACHABLE() as a marker.
>>>>
>>>> Well. For one such an assumption takes as a prereq that a debug build will
>>>> be run through full coverage testing, i.e. all reachable paths proven to
>>>> be taken. I understand that this prereq is intended to somehow be met,
>>>> even if I'm having difficulty seeing how such a final proof would look
>>>> like: Full coverage would, to me, mean that _every_ line is reachable. Yet
>>>> clearly any ASSERT_UNREACHABLE() must never be reached.
>>>>
>>>> And then not covering for such cases takes the further assumption that
>>>> debug and release builds are functionally identical. I'm afraid this would
>>>> be a wrong assumption to make:
>>>> 1) We may screw up somewhere, with code wrongly enabled only in one of the
>>>>    two build modes.
>>>> 2) The compiler may screw up, in particular with optimization.
>>>
>>> I think there are two different issues here we are discussing.
>>>
>>> One issue, like you said, has to do with coverage. It is important to
>>> mark as "unreachable" any part of the code that is indeed unreachable
>>> so that we can account it properly when we do coverage analysis. At the
>>> moment the only "unreachable" marker that we have is
>>> ASSERT_UNREACHABLE(), and I am hoping we can use it as part of the
>>> coverage analysis we'll do.
>>>
>>> However, there is a different separate question about what to do in the
>>> Xen code after an ASSERT_UNREACHABLE(). E.g.:
>>>
>>>              default:
>>>                  ASSERT_UNREACHABLE();
>>>                  return -EPERM; /* is it better with or without this? */
>>>              }
>>>
>>> Leaving coverage aside, would it be better to be defensive and actually
>>> attempt to report errors back after an ASSERT_UNREACHABLE() like in the
>>> example? Or is it better to assume the code is actually unreachable
>>> hence there is no need to do anything afterwards?
>>>
>>> One one hand, being defensive sounds good, on the other hand, any code
>>> we add after ASSERT_UNREACHABLE() is dead code which cannot be tested,
>>> which is also not good. In this example, there is no way to test the
>>> return -EPERM code path. We also need to consider what is the right
>>> thing to do if Xen finds itself in an erroneous situation such as being
>>> in an unreachable code location.
>>>
>>> So, after thinking about it and also talking to the safety manager, I
>>> think we should:
>>> - implement ASSERT_UNREACHABLE with a warning in release builds
>>
>> If at all, then controlled by a default-off Kconfig setting. This would,
>> after all, raise the question of how ASSERT() should behave then. Imo
>> the two should be consistent in this regard, and NDEBUG clearly results
>> in the expectation that ASSERT() expands to nothing. Perhaps this is
>> finally the time where we need to separate NDEBUG from CONFIG_DEBUG; we
>> did discuss doing so before. Then in your release builds, you could
>> actually leave assertions active.
>  
> Yes, a kconfig to define the behavior of ASSERT_UNREACHABLE in release
> builds is fine. And you are right that we should consider doing
> something similar for ASSERT too.
> 
> I think that in any environment where safety (i.e. correctness of
> behavior) is a primary concern, attempting to continue without doing
> anything after reaching a point that is supposed to be unreachable is
> not a good idea.
> 
> I think Xen should do something in response to reaching a point it is
> not supposed to reach. I don't know yet what is the best course of
> action but printing a warning seems to be the bare minimum.
> 
> Crashing the system is not a good idea as it could potentially be
> exploited by malicious guests (security might not be the primary concern
> but still.)

Yet continuing may set the system up for much harder to understand issues,
including crashes and exploitable issues later on.

Jan

[XEN PATCH v2 06/13] x86/mce: address violations of MISRA C Rule 16.3

[Federico Serafini]

Add missing break statements to address violations of
MISRA C 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/cpu/mcheck/mce_amd.c   | 1 +
 xen/arch/x86/cpu/mcheck/mce_intel.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/xen/arch/x86/cpu/mcheck/mce_amd.c b/xen/arch/x86/cpu/mcheck/mce_amd.c
index 3318b8204f..4f06a3153b 100644
--- a/xen/arch/x86/cpu/mcheck/mce_amd.c
+++ b/xen/arch/x86/cpu/mcheck/mce_amd.c
@@ -201,6 +201,7 @@ static void mcequirk_amd_apply(enum mcequirk_amd_flags flags)
 
     default:
         ASSERT(flags == MCEQUIRK_NONE);
+        break;
     }
 }
 
diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c b/xen/arch/x86/cpu/mcheck/mce_intel.c
index dd812f4b8a..9574dedbfd 100644
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -896,6 +896,8 @@ static void intel_init_ppin(const struct cpuinfo_x86 *c)
             ppin_msr = 0;
         else if ( c == &boot_cpu_data )
             ppin_msr = MSR_PPIN;
+
+        break;
     }
 }
 
-- 
2.34.1

Re: [XEN PATCH v2 06/13] x86/mce: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 24.06.2024 11:04, Federico Serafini wrote:
> Add missing break statements to address violations of
> MISRA C 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>

Re: [XEN PATCH v2 06/13] x86/mce: address violations of MISRA C Rule 16.3

[Stefano Stabellini]

On Mon, 24 Jun 2024, Federico Serafini wrote:
> Add missing break statements to address violations of
> MISRA C Rule 16.3: "An unconditional `break' statement shall terminate
> every switch-clause".
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

[XEN PATCH v2 07/13] x86/hvm: address violations of MISRA C Rule 16.3

[Federico Serafini]

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

Add pseudo keyword fallthrough or missing break statement
to address violations of the rule.

As a defensive measure, return -EOPNOTSUPP in case an unreachable
return statement is reached.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
Changes in v2:
- replace hypened fallthrough with the pseudo keyword.
---
 xen/arch/x86/hvm/emulate.c   | 9 ++++++---
 xen/arch/x86/hvm/hvm.c       | 6 ++++++
 xen/arch/x86/hvm/hypercall.c | 1 +
 xen/arch/x86/hvm/irq.c       | 1 +
 4 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 02e378365b..859c21a5ab 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -339,7 +339,7 @@ static int hvmemul_do_io(
     }
     case X86EMUL_UNIMPLEMENTED:
         ASSERT_UNREACHABLE();
-        /* Fall-through */
+        fallthrough;
     default:
         BUG();
     }
@@ -2674,6 +2674,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
 
     default:
         ASSERT_UNREACHABLE();
+        break;
     }
 
     if ( hvmemul_ctxt->ctxt.retire.singlestep )
@@ -2764,6 +2765,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
         /* fallthrough */
     default:
         hvm_emulate_writeback(&ctxt);
+        break;
     }
 
     return rc;
@@ -2799,10 +2801,11 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
         memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
                hvio->mmio_insn_bytes);
     }
-    /* Fall-through */
+    fallthrough;
     default:
         ctx.set_context = (kind == EMUL_KIND_SET_CONTEXT_DATA);
         rc = hvm_emulate_one(&ctx, VIO_no_completion);
+        break;
     }
 
     switch ( rc )
@@ -2818,7 +2821,7 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
     case X86EMUL_UNIMPLEMENTED:
         if ( hvm_monitor_emul_unimplemented() )
             return;
-        /* fall-through */
+        fallthrough;
     case X86EMUL_UNHANDLEABLE:
         hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx, rc);
         hvm_inject_hw_exception(trapnr, errcode);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 7f4b627b1f..c263e562ff 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4919,6 +4919,8 @@ static int do_altp2m_op(
 
     default:
         ASSERT_UNREACHABLE();
+        rc = -EOPNOTSUPP;
+        break;
     }
 
  out:
@@ -5020,6 +5022,8 @@ static int compat_altp2m_op(
 
     default:
         ASSERT_UNREACHABLE();
+        rc = -EOPNOTSUPP;
+        break;
     }
 
     return rc;
@@ -5283,6 +5287,8 @@ void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
          * %cs and %tr are unconditionally present.  SVM ignores these present
          * bits and will happily run without them set.
          */
+        fallthrough;
+
     case x86_seg_cs:
         reg->p = 1;
         break;
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 7fb3136f0c..2271afe02a 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -111,6 +111,7 @@ int hvm_hypercall(struct cpu_user_regs *regs)
     case 8:
         eax = regs->rax;
         /* Fallthrough to permission check. */
+        fallthrough;
     case 4:
     case 2:
         if ( currd->arch.monitor.guest_request_userspace_enabled &&
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 210cebb0e6..1eab44defc 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -282,6 +282,7 @@ static void hvm_set_callback_irq_level(struct vcpu *v)
             __hvm_pci_intx_assert(d, pdev, pintx);
         else
             __hvm_pci_intx_deassert(d, pdev, pintx);
+        break;
     default:
         break;
     }
-- 
2.34.1

Re: [XEN PATCH v2 07/13] x86/hvm: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 24.06.2024 11:04, Federico Serafini wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -339,7 +339,7 @@ static int hvmemul_do_io(
>      }
>      case X86EMUL_UNIMPLEMENTED:
>          ASSERT_UNREACHABLE();
> -        /* Fall-through */
> +        fallthrough;
>      default:
>          BUG();
>      }

This or very similar comment are replaced elsewhere in this patch. I'm
sure we have more of them. Hence an alternative would be to deviate those
variations of what we already deviate. I recall there was a mail from
Julien asking to avoid extending the set, unless some forms are used
pretty frequently. Sadly nothing towards judgement between the
alternatives is said in the description.

> @@ -2674,6 +2674,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
>  
>      default:
>          ASSERT_UNREACHABLE();
> +        break;
>      }
>  
>      if ( hvmemul_ctxt->ctxt.retire.singlestep )
> @@ -2764,6 +2765,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
>          /* fallthrough */

What about this? It doesn't match anything I see in deviations.rst.

>      default:
>          hvm_emulate_writeback(&ctxt);
> +        break;
>      }
>  
>      return rc;
> @@ -2799,10 +2801,11 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
>          memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
>                 hvio->mmio_insn_bytes);
>      }
> -    /* Fall-through */
> +    fallthrough;
>      default:
>          ctx.set_context = (kind == EMUL_KIND_SET_CONTEXT_DATA);
>          rc = hvm_emulate_one(&ctx, VIO_no_completion);
> +        break;
>      }

While not as much of a problem for the comment, I view a statement like
this as mis-indented.

> @@ -5283,6 +5287,8 @@ void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
>           * %cs and %tr are unconditionally present.  SVM ignores these present
>           * bits and will happily run without them set.
>           */
> +        fallthrough;
> +
>      case x86_seg_cs:
>          reg->p = 1;
>          break;

Why the extra blank line here, ...

> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -111,6 +111,7 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>      case 8:
>          eax = regs->rax;
>          /* Fallthrough to permission check. */
> +        fallthrough;
>      case 4:
>      case 2:
>          if ( currd->arch.monitor.guest_request_userspace_enabled &&

... when e.g. here there's none? I'm afraid this goes back to an
unfinished discussion as to whether to have blank lines between blocks
in fall-through situations. My view is that not having them in these
cases is helping to make the falling through visually noticeable.

Jan

Re: [XEN PATCH v2 07/13] x86/hvm: address violations of MISRA C Rule 16.3

[Federico Serafini]

On 24/06/24 17:32, Jan Beulich wrote:
> On 24.06.2024 11:04, Federico Serafini wrote:
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -339,7 +339,7 @@ static int hvmemul_do_io(
>>       }
>>       case X86EMUL_UNIMPLEMENTED:
>>           ASSERT_UNREACHABLE();
>> -        /* Fall-through */
>> +        fallthrough;
>>       default:
>>           BUG();
>>       }
> 
> This or very similar comment are replaced elsewhere in this patch. I'm
> sure we have more of them. Hence an alternative would be to deviate those
> variations of what we already deviate. I recall there was a mail from
> Julien asking to avoid extending the set, unless some forms are used
> pretty frequently. Sadly nothing towards judgement between the
> alternatives is said in the description.

I found few occurrences of the hypened fallthrough,
It doesn't seem like a very used form to me,
and most of them are in emulate.c, a file I needed to touch anyway.

The fact that the pseudo keyword is the one preferred is mentioned
in deviations.rst, but I can mention this also in the description.

>> @@ -2674,6 +2674,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
>>   
>>       default:
>>           ASSERT_UNREACHABLE();
>> +        break;
>>       }
>>   
>>       if ( hvmemul_ctxt->ctxt.retire.singlestep )
>> @@ -2764,6 +2765,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
>>           /* fallthrough */
> 
> What about this? It doesn't match anything I see in deviations.rst.

The last item for R16.3 in deviations.rst explicitly says that
existing comment of this form are considered as safe (i.e., deviated)
but deprecated, meaning that the pseudo keyword should be used for new
cases. We can consider a rephrasing if it is not clear enough.

> 
>>       default:
>>           hvm_emulate_writeback(&ctxt);
>> +        break;
>>       }
>>   
>>       return rc;
>> @@ -2799,10 +2801,11 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
>>           memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
>>                  hvio->mmio_insn_bytes);
>>       }
>> -    /* Fall-through */
>> +    fallthrough;
>>       default:
>>           ctx.set_context = (kind == EMUL_KIND_SET_CONTEXT_DATA);
>>           rc = hvm_emulate_one(&ctx, VIO_no_completion);
>> +        break;
>>       }
> 
> While not as much of a problem for the comment, I view a statement like
> this as mis-indented.
> 
>> @@ -5283,6 +5287,8 @@ void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
>>            * %cs and %tr are unconditionally present.  SVM ignores these present
>>            * bits and will happily run without them set.
>>            */
>> +        fallthrough;
>> +
>>       case x86_seg_cs:
>>           reg->p = 1;
>>           break;
> 
> Why the extra blank line here, ...
> 
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -111,6 +111,7 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>>       case 8:
>>           eax = regs->rax;
>>           /* Fallthrough to permission check. */
>> +        fallthrough;
>>       case 4:
>>       case 2:
>>           if ( currd->arch.monitor.guest_request_userspace_enabled &&
> 
> ... when e.g. here there's none? I'm afraid this goes back to an
> unfinished discussion as to whether to have blank lines between blocks
> in fall-through situations. My view is that not having them in these
> cases is helping to make the falling through visually noticeable.

I looked ad the context to preserve the style
of the existing cases.

What do you think about:
-keep the existing style when a break needs to be inserted;
-no blank line if a fallthrough needs to inserted.

-- 
Federico Serafini, M.Sc.

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

Re: [XEN PATCH v2 07/13] x86/hvm: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 25.06.2024 09:21, Federico Serafini wrote:
> On 24/06/24 17:32, Jan Beulich wrote:
>> On 24.06.2024 11:04, Federico Serafini wrote:
>>> @@ -2674,6 +2674,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
>>>   
>>>       default:
>>>           ASSERT_UNREACHABLE();
>>> +        break;
>>>       }
>>>   
>>>       if ( hvmemul_ctxt->ctxt.retire.singlestep )
>>> @@ -2764,6 +2765,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
>>>           /* fallthrough */
>>
>> What about this? It doesn't match anything I see in deviations.rst.
> 
> The last item for R16.3 in deviations.rst explicitly says that
> existing comment of this form are considered as safe (i.e., deviated)
> but deprecated, meaning that the pseudo keyword should be used for new
> cases. We can consider a rephrasing if it is not clear enough.

Apologies. I mistakenly looked at grep output rather than actual file
contents. Please disregard this comment of mine.

>>> @@ -5283,6 +5287,8 @@ void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
>>>            * %cs and %tr are unconditionally present.  SVM ignores these present
>>>            * bits and will happily run without them set.
>>>            */
>>> +        fallthrough;
>>> +
>>>       case x86_seg_cs:
>>>           reg->p = 1;
>>>           break;
>>
>> Why the extra blank line here, ...
>>
>>> --- a/xen/arch/x86/hvm/hypercall.c
>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>> @@ -111,6 +111,7 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>>>       case 8:
>>>           eax = regs->rax;
>>>           /* Fallthrough to permission check. */
>>> +        fallthrough;
>>>       case 4:
>>>       case 2:
>>>           if ( currd->arch.monitor.guest_request_userspace_enabled &&
>>
>> ... when e.g. here there's none? I'm afraid this goes back to an
>> unfinished discussion as to whether to have blank lines between blocks
>> in fall-through situations. My view is that not having them in these
>> cases is helping to make the falling through visually noticeable.
> 
> I looked ad the context to preserve the style
> of the existing cases.
> 
> What do you think about:
> -keep the existing style when a break needs to be inserted;

Even that may be a judgment call, I'd say. But commonly: Yes.

> -no blank line if a fallthrough needs to inserted.

Yes here, but others (Andrew?) may disagree with me.

Jan

Re: [XEN PATCH v2 07/13] x86/hvm: address violations of MISRA C Rule 16.3

[Stefano Stabellini]

On Mon, 24 Jun 2024, Federico Serafini wrote:
> MISRA C Rule 16.3 states that "An unconditional `break' statement shall
> terminate every switch-clause".
> 
> Add pseudo keyword fallthrough or missing break statement
> to address violations of the rule.
> 
> As a defensive measure, return -EOPNOTSUPP in case an unreachable
> return statement is reached.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
> Changes in v2:
> - replace hypened fallthrough with the pseudo keyword.
> ---
>  xen/arch/x86/hvm/emulate.c   | 9 ++++++---
>  xen/arch/x86/hvm/hvm.c       | 6 ++++++
>  xen/arch/x86/hvm/hypercall.c | 1 +
>  xen/arch/x86/hvm/irq.c       | 1 +
>  4 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 02e378365b..859c21a5ab 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -339,7 +339,7 @@ static int hvmemul_do_io(
>      }
>      case X86EMUL_UNIMPLEMENTED:
>          ASSERT_UNREACHABLE();
> -        /* Fall-through */
> +        fallthrough;
>      default:
>          BUG();
>      }
> @@ -2674,6 +2674,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
>  
>      default:
>          ASSERT_UNREACHABLE();
> +        break;

same here


>      }
>  
>      if ( hvmemul_ctxt->ctxt.retire.singlestep )
> @@ -2764,6 +2765,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
>          /* fallthrough */
>      default:
>          hvm_emulate_writeback(&ctxt);
> +        break;
>      }
>  
>      return rc;
> @@ -2799,10 +2801,11 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
>          memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
>                 hvio->mmio_insn_bytes);
>      }
> -    /* Fall-through */
> +    fallthrough;
>      default:
>          ctx.set_context = (kind == EMUL_KIND_SET_CONTEXT_DATA);
>          rc = hvm_emulate_one(&ctx, VIO_no_completion);
> +        break;
>      }
>  
>      switch ( rc )
> @@ -2818,7 +2821,7 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
>      case X86EMUL_UNIMPLEMENTED:
>          if ( hvm_monitor_emul_unimplemented() )
>              return;
> -        /* fall-through */
> +        fallthrough;
>      case X86EMUL_UNHANDLEABLE:
>          hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx, rc);
>          hvm_inject_hw_exception(trapnr, errcode);
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 7f4b627b1f..c263e562ff 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4919,6 +4919,8 @@ static int do_altp2m_op(
>  
>      default:
>          ASSERT_UNREACHABLE();
> +        rc = -EOPNOTSUPP;
> +        break;

same here


>      }
>  
>   out:
> @@ -5020,6 +5022,8 @@ static int compat_altp2m_op(
>  
>      default:
>          ASSERT_UNREACHABLE();
> +        rc = -EOPNOTSUPP;
> +        break;

same here


>      }
>  
>      return rc;
> @@ -5283,6 +5287,8 @@ void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
>           * %cs and %tr are unconditionally present.  SVM ignores these present
>           * bits and will happily run without them set.
>           */
> +        fallthrough;
> +
>      case x86_seg_cs:
>          reg->p = 1;
>          break;
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index 7fb3136f0c..2271afe02a 100644
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -111,6 +111,7 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>      case 8:
>          eax = regs->rax;
>          /* Fallthrough to permission check. */
> +        fallthrough;
>      case 4:
>      case 2:
>          if ( currd->arch.monitor.guest_request_userspace_enabled &&
> diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
> index 210cebb0e6..1eab44defc 100644
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -282,6 +282,7 @@ static void hvm_set_callback_irq_level(struct vcpu *v)
>              __hvm_pci_intx_assert(d, pdev, pintx);
>          else
>              __hvm_pci_intx_deassert(d, pdev, pintx);
> +        break;
>      default:
>          break;
>      }
> -- 
> 2.34.1
> 
> 

[XEN PATCH v2 08/13] x86/vpt: address a violation of MISRA C Rule 16.3

[Federico Serafini]

Add pseudo keyword fallthrough to meet the requirements to deviate
a violation of MISRA C 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/hvm/vpt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index e1d6845a28..c76a9a272b 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -121,6 +121,8 @@ static int pt_irq_masked(struct periodic_time *pt)
     }
 
     /* Fallthrough to check if the interrupt is masked on the IO APIC. */
+    fallthrough;
+
     case PTSRC_ioapic:
     {
         int mask = vioapic_get_mask(v->domain, gsi);
-- 
2.34.1

Re: [XEN PATCH v2 08/13] x86/vpt: address a violation of MISRA C Rule 16.3

[Jan Beulich]

On 24.06.2024 11:04, Federico Serafini wrote:
> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -121,6 +121,8 @@ static int pt_irq_masked(struct periodic_time *pt)
>      }
>  
>      /* Fallthrough to check if the interrupt is masked on the IO APIC. */
> +    fallthrough;
> +
>      case PTSRC_ioapic:
>      {
>          int mask = vioapic_get_mask(v->domain, gsi);

I'm afraid this is one more case where the (pseudo)keyword wants indenting
by one more level, to match others relative to the case labels. Sure, this
will be a little odd with the preceding figure brace, but what do you do?

Jan

Re: [XEN PATCH v2 08/13] x86/vpt: address a violation of MISRA C Rule 16.3x

[Stefano Stabellini]

On Mon, 24 Jun 2024, Federico Serafini wrote:
> Add pseudo keyword fallthrough to meet the requirements to deviate
> a violation of MISRA C Rule 16.3 ("An unconditional `break'
> statement shall terminate every switch-clause").
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

[XEN PATCH v2 09/13] x86/mm: add defensive return

[Federico Serafini]

Add defensive return statement at the end of an unreachable
default case. Other than improve safety, this meets the requirements
to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
statement shall terminate every switch-clause".

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

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 648d6dd475..2e19ced15e 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -916,6 +916,7 @@ get_page_from_l1e(
                 return 0;
             default:
                 ASSERT_UNREACHABLE();
+                return 0;
             }
         }
         else if ( l1f & _PAGE_RW )
-- 
2.34.1

Re: [XEN PATCH v2 09/13] x86/mm: add defensive return

[Jan Beulich]

On 24.06.2024 11:04, Federico Serafini wrote:
> Add defensive return statement at the end of an unreachable
> default case. Other than improve safety,

It is particularly with this in mind that ...

> this meets the requirements
> to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
> statement shall terminate every switch-clause".
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
>  xen/arch/x86/mm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 648d6dd475..2e19ced15e 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -916,6 +916,7 @@ get_page_from_l1e(
>                  return 0;
>              default:
>                  ASSERT_UNREACHABLE();
> +                return 0;
>              }

... returning "success" in such a case can't be quite right. As indicated
elsewhere, really we want -EINTERNAL for such cases, just that errno.h
doesn't know anything like this, and I'm also unaware of a somewhat
similar identifier that we might "clone" from elsewhere. Hence we'll want
to settle on some existing error code which we then use here and in
similar situations. EINVAL is the primary example of what I would prefer
to _not_ see used for this.

Jan

Re: [XEN PATCH v2 09/13] x86/mm: add defensive return

[Federico Serafini]

On 24/06/24 17:39, Jan Beulich wrote:
> On 24.06.2024 11:04, Federico Serafini wrote:
>> Add defensive return statement at the end of an unreachable
>> default case. Other than improve safety,
> 
> It is particularly with this in mind that ...
> 
>> this meets the requirements
>> to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
>> statement shall terminate every switch-clause".
>>
>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>> ---
>>   xen/arch/x86/mm.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index 648d6dd475..2e19ced15e 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -916,6 +916,7 @@ get_page_from_l1e(
>>                   return 0;
>>               default:
>>                   ASSERT_UNREACHABLE();
>> +                return 0;
>>               }
> 
> ... returning "success" in such a case can't be quite right. As indicated
> elsewhere, really we want -EINTERNAL for such cases, just that errno.h
> doesn't know anything like this, and I'm also unaware of a somewhat
> similar identifier that we might "clone" from elsewhere. Hence we'll want
> to settle on some existing error code which we then use here and in
> similar situations. EINVAL is the primary example of what I would prefer
> to _not_ see used for this.

Sorry, I got confused by the ASSERT_UNREACHABLE followed by a
return 0 few lines above.

-- 
Federico Serafini, M.Sc.

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

Re: [XEN PATCH v2 09/13] x86/mm: add defensive return

[Stefano Stabellini]

On Mon, 24 Jun 2024, Federico Serafini wrote:
> Add defensive return statement at the end of an unreachable
> default case. Other than improve safety, this meets the requirements
> to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
> statement shall terminate every switch-clause".
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
>  xen/arch/x86/mm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 648d6dd475..2e19ced15e 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -916,6 +916,7 @@ get_page_from_l1e(
>                  return 0;
>              default:
>                  ASSERT_UNREACHABLE();
> +                return 0;

Let's avoid this

[XEN PATCH v2 10/13] x86/mpparse: address a violation of MISRA C Rule 16.3

[Federico Serafini]

Add a missing break statement to address a violation of
MISRA C 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/mpparse.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c
index d8ccab2449..306d8ed97a 100644
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -544,6 +544,7 @@ static inline void __init construct_default_ISA_mptable(int mpc_default_type)
 		case 4:
 		case 7:
 			memcpy(bus.mpc_bustype, "MCA   ", 6);
+			break;
 	}
 	MP_bus_info(&bus);
 	if (mpc_default_type > 4) {
-- 
2.34.1

Re: [XEN PATCH v2 10/13] x86/mpparse: address a violation of MISRA C Rule 16.3

[Jan Beulich]

On 24.06.2024 11:04, Federico Serafini wrote:
> Add a missing break statement to address a violation of
> MISRA C 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>

Re: [XEN PATCH v2 10/13] x86/mpparse: address a violation of MISRA C Rule 16.3

[Stefano Stabellini]

On Mon, 24 Jun 2024, Federico Serafini wrote:
> Add a missing break statement to address a violation of
> MISRA C Rule 16.3: "An unconditional `break' statement shall terminate
> every switch-clause".
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

[XEN PATCH v2 11/13] x86/pmtimer: address a violation of MISRA C Rule 16.3

[Federico Serafini]

Add missing break statement to address a violation of MISRA C 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/hvm/pmtimer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/pmtimer.c b/xen/arch/x86/hvm/pmtimer.c
index 97099ac305..87a7a01c9f 100644
--- a/xen/arch/x86/hvm/pmtimer.c
+++ b/xen/arch/x86/hvm/pmtimer.c
@@ -185,6 +185,7 @@ static int cf_check handle_evt_io(
                 gdprintk(XENLOG_WARNING, 
                          "Bad ACPI PM register write: %x bytes (%x) at %x\n", 
                          bytes, *val, port);
+                break;
             }
         }
         /* Fix up the SCI state to match the new register state */
-- 
2.34.1

Re: [XEN PATCH v2 11/13] x86/pmtimer: address a violation of MISRA C Rule 16.3

[Jan Beulich]

On 24.06.2024 11:04, Federico Serafini wrote:
> Add missing break statement to address a violation of MISRA C 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>

I'm curious though on what basis you decided to split this off of
patch 7, ...

> ---
>  xen/arch/x86/hvm/pmtimer.c | 1 +
>  1 file changed, 1 insertion(+)

... touching various other files under xen/arch/x86/hvm/.

Jan

Re: [XEN PATCH v2 11/13] x86/pmtimer: address a violation of MISRA C Rule 16.3

[Federico Serafini]

On 24/06/24 17:43, Jan Beulich wrote:
> On 24.06.2024 11:04, Federico Serafini wrote:
>> Add missing break statement to address a violation of MISRA C 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>
> 
> I'm curious though on what basis you decided to split this off of
> patch 7, ...
> 
>> ---
>>   xen/arch/x86/hvm/pmtimer.c | 1 +
>>   1 file changed, 1 insertion(+)
> 
> ... touching various other files under xen/arch/x86/hvm/.

Looking at the log I found some commits where the component
name was made explicit.
I can squash it into patch 7 in a v3.

-- 
Federico Serafini, M.Sc.

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

Re: [XEN PATCH v2 11/13] x86/pmtimer: address a violation of MISRA C Rule 16.3

[Stefano Stabellini]

On Mon, 24 Jun 2024, Federico Serafini wrote:
> Add missing break statement to address a violation of MISRA C Rule 16.3
> ("An unconditional `break' statement shall terminate every
> switch-clause").
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

[XEN PATCH v2 12/13] x86/vPIC: address a violation of MISRA C Rule 16.3

[Federico Serafini]

Add pseudokeyword fallthrough to meet the requirements to deviate
a violation of MISRA C Rul 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/hvm/vpic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index 7c3b5c7254..6427b08086 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -309,6 +309,7 @@ static void vpic_ioport_write(
             if ( !(vpic->init_state & 8) )
                 break; /* CASCADE mode: wait for write to ICW3. */
             /* SNGL mode: fall through (no ICW3). */
+            fallthrough;
         case 2:
             /* ICW3 */
             vpic->init_state++;
-- 
2.34.1

Re: [XEN PATCH v2 12/13] x86/vPIC: address a violation of MISRA C Rule 16.3

[Jan Beulich]

On 24.06.2024 11:04, Federico Serafini wrote:
> Add pseudokeyword fallthrough to meet the requirements to deviate
> a violation of MISRA C Rul 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>

Re: [XEN PATCH v2 12/13] x86/vPIC: address a violation of MISRA C Rule 16.3

[Stefano Stabellini]

On Mon, 24 Jun 2024, Federico Serafini wrote:
> Add pseudokeyword fallthrough to meet the requirements to deviate
> a violation of MISRA C Rul 16.3: "An unconditional `break' statement
> shall terminate every switch-clause".
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

[XEN PATCH v2 13/13] x86/vlapic: address a violation of MISRA C Rule 16.3

[Federico Serafini]

Add missing break statement to address a violation of MISRA C
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/hvm/vlapic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 9cfc82666a..2ec9594271 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -367,6 +367,7 @@ static void vlapic_accept_irq(struct vcpu *v, uint32_t icr_low)
         gdprintk(XENLOG_ERR, "TODO: unsupported delivery mode in ICR %x\n",
                  icr_low);
         domain_crash(v->domain);
+        break;
     }
 }
 
-- 
2.34.1

Re: [XEN PATCH v2 13/13] x86/vlapic: address a violation of MISRA C Rule 16.3

[Jan Beulich]

On 24.06.2024 11:04, Federico Serafini wrote:
> Add missing break statement to address a violation of MISRA C
> 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>

Re: [XEN PATCH v2 13/13] x86/vlapic: address a violation of MISRA C Rule 16.3

[Stefano Stabellini]

On Mon, 24 Jun 2024, Federico Serafini wrote:
> Add missing break statement to address a violation of MISRA C
> Rule 16.3: "An unconditional `break' statement shall terminate every
> switch-clause".
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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