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

[XEN PATCH v3 00/12] 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 (12):
  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/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                    | 2 ++
 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                           | 5 +++++
 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                           | 4 +++-
 xen/arch/x86/mm.c                                | 1 +
 xen/arch/x86/mpparse.c                           | 1 +
 xen/arch/x86/traps.c                             | 3 +++
 18 files changed, 35 insertions(+), 7 deletions(-)

-- 
2.34.1

[XEN PATCH v3 01/12] 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: a128d8da913b ("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>
---
Changes in v3:
- truncate to 12 digits.

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 c8bff0e057..9b994be63d 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -416,7 +416,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

[XEN PATCH v3 02/12] 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Jan Beulich <jbeulich@suse.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

[XEN PATCH v3 03/12] 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Jan Beulich <jbeulich@suse.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

[XEN PATCH v3 04/12] 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>
---
Changes in v3:
- addressed all violations of R16.3 in vpmu_intel.c
---
 xen/arch/x86/cpu/vpmu.c       | 3 +++
 xen/arch/x86/cpu/vpmu_intel.c | 2 ++
 2 files changed, 5 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..26dd3a9358 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -666,6 +666,7 @@ static int cf_check core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
 
             xen_pmu_cntr_pair[tmp].control = msr_content;
         }
+        break;
     }
 
     if ( type != MSR_TYPE_GLOBAL )
@@ -713,6 +714,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 v3 04/12] x86/vpmu: address violations of MISRA C Rule 16.3

[Stefano Stabellini]

On Wed, 26 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>

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

[Jan Beulich]

On 26.06.2024 11:27, 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: Jan Beulich <jbeulich@suse.com>

[XEN PATCH v3 05/12] 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>
---
Changes in v3:
- use break instead of fallthrough.
---
 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..d62598a4c2 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);
+        break;
     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);
+        break;
     case 'i': /* 'ignore' */
         break;
     default:  /* 'fatal' */
-- 
2.34.1

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

[Stefano Stabellini]

On Wed, 26 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>
> ---
> Changes in v3:
> - use break instead of fallthrough.
> ---
>  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..d62598a4c2 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;

FYI the ASSERT_UNREACHABLE is still being discussed

Other than that:

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


>      }
>  }
>  
> @@ -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);
> +        break;
>      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);
> +        break;
>      case 'i': /* 'ignore' */
>          break;
>      default:  /* 'fatal' */

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

[Jan Beulich]

On 27.06.2024 02:52, Stefano Stabellini wrote:
> On Wed, 26 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>
>> ---
>> Changes in v3:
>> - use break instead of fallthrough.
>> ---
>>  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..d62598a4c2 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;
> 
> FYI the ASSERT_UNREACHABLE is still being discussed

Could you clarify what this means for this patch and you R-b? Is it somehow
conditional upon the outcome of that discussion, and hence you'd rather not
see this patch go in yet?

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

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

Jan

[XEN PATCH v3 06/12] 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Jan Beulich <jbeulich@suse.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

[XEN PATCH v3 07/12] 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 v3:
- squashed here modifications of pmtimer.c;
- no blank line after fallthrough;
- better indentation of fallthrough.
---
 xen/arch/x86/hvm/emulate.c   | 9 ++++++---
 xen/arch/x86/hvm/hvm.c       | 5 +++++
 xen/arch/x86/hvm/hypercall.c | 1 +
 xen/arch/x86/hvm/irq.c       | 1 +
 xen/arch/x86/hvm/pmtimer.c   | 1 +
 5 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 02e378365b..f5dd08f510 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;
@@ -2798,11 +2800,12 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
         hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn);
         memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
                hvio->mmio_insn_bytes);
+        fallthrough;
     }
-    /* Fall-through */
     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..d7f195ba9a 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,7 @@ 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;
     }
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 v3 07/12] x86/hvm: address violations of MISRA C Rule 16.3

[Stefano Stabellini]

On Wed, 26 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>

Aside from the ASSERT_UNREACHABLE which is still under discussion:

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

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

[Jan Beulich]

On 26.06.2024 11:28, Federico Serafini wrote:
> @@ -2798,11 +2800,12 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
>          hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn);
>          memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
>                 hvio->mmio_insn_bytes);
> +        fallthrough;
>      }
> -    /* Fall-through */
>      default:

Can you clarify for me please whether this arrangement actually helps?
I'm pretty sure it'll result in a Coverity complaint, as my understanding
is that for them the marker (comment or pseudo-keyword) has to immediately
precede the subsequent label. IOW even if you confirmed that Eclair is
smarter in this regard, it may still need converting to

        hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn);
        memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
               hvio->mmio_insn_bytes);
    }
        fallthrough;
    default:

> --- 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 &&

Arguably the comment could then be dropped in exchange. Yet I won't
insist on you doing so (and others may also disagree).

Jan

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

[Federico Serafini]

On 01/07/24 10:47, Jan Beulich wrote:
> On 26.06.2024 11:28, Federico Serafini wrote:
>> @@ -2798,11 +2800,12 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
>>           hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn);
>>           memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
>>                  hvio->mmio_insn_bytes);
>> +        fallthrough;
>>       }
>> -    /* Fall-through */
>>       default:
> 
> Can you clarify for me please whether this arrangement actually helps?
> I'm pretty sure it'll result in a Coverity complaint, as my understanding
> is that for them the marker (comment or pseudo-keyword) has to immediately
> precede the subsequent label. IOW even if you confirmed that Eclair is
> smarter in this regard, it may still need converting to
> 
>          hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn);
>          memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
>                 hvio->mmio_insn_bytes);
>      }
>          fallthrough;
>      default:
>

Yes, this is ok for ECLAIR.

-- 
Federico Serafini, M.Sc.

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

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

[Stefano Stabellini]

On Tue, 2 Jul 2024, Federico Serafini wrote:
> On 01/07/24 10:47, Jan Beulich wrote:
> > On 26.06.2024 11:28, Federico Serafini wrote:
> > > @@ -2798,11 +2800,12 @@ void hvm_emulate_one_vm_event(enum emul_kind kind,
> > > unsigned int trapnr,
> > >           hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn);
> > >           memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
> > >                  hvio->mmio_insn_bytes);
> > > +        fallthrough;
> > >       }
> > > -    /* Fall-through */
> > >       default:
> > 
> > Can you clarify for me please whether this arrangement actually helps?
> > I'm pretty sure it'll result in a Coverity complaint, as my understanding
> > is that for them the marker (comment or pseudo-keyword) has to immediately
> > precede the subsequent label. IOW even if you confirmed that Eclair is
> > smarter in this regard, it may still need converting to
> > 
> >          hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn);
> >          memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
> >                 hvio->mmio_insn_bytes);
> >      }
> >          fallthrough;
> >      default:
> > 
> 
> Yes, this is ok for ECLAIR.

Given that Jan might be right that Coverity and others would prefer the
keyword on the line immediately above "default", and given that it works
anyway for ECLAIR, then I think it would be better to stay on the safe
side and move the "fallback" right on top of default.

If you are OK with it, please resend this patch and following patches.
Patches 1-6 are fully acked and I'd be happy to take them in my for-4.20
branch.

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

[Federico Serafini]

On 13/07/24 01:25, Stefano Stabellini wrote:
> On Tue, 2 Jul 2024, Federico Serafini wrote:
>> On 01/07/24 10:47, Jan Beulich wrote:
>>> On 26.06.2024 11:28, Federico Serafini wrote:
>>>> @@ -2798,11 +2800,12 @@ void hvm_emulate_one_vm_event(enum emul_kind kind,
>>>> unsigned int trapnr,
>>>>            hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn);
>>>>            memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
>>>>                   hvio->mmio_insn_bytes);
>>>> +        fallthrough;
>>>>        }
>>>> -    /* Fall-through */
>>>>        default:
>>>
>>> Can you clarify for me please whether this arrangement actually helps?
>>> I'm pretty sure it'll result in a Coverity complaint, as my understanding
>>> is that for them the marker (comment or pseudo-keyword) has to immediately
>>> precede the subsequent label. IOW even if you confirmed that Eclair is
>>> smarter in this regard, it may still need converting to
>>>
>>>           hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn);
>>>           memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data,
>>>                  hvio->mmio_insn_bytes);
>>>       }
>>>           fallthrough;
>>>       default:
>>>
>>
>> Yes, this is ok for ECLAIR.
> 
> Given that Jan might be right that Coverity and others would prefer the
> keyword on the line immediately above "default", and given that it works
> anyway for ECLAIR, then I think it would be better to stay on the safe
> side and move the "fallback" right on top of default.
> 
> If you are OK with it, please resend this patch and following patches.
> Patches 1-6 are fully acked and I'd be happy to take them in my for-4.20
> branch.

V4 sent:
https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00823.html

-- 
Federico Serafini, M.Sc.

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

[XEN PATCH v3 08/12] 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>
---
Changes in v3:
- better indentation of fallthrough.
---
 xen/arch/x86/hvm/vpt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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

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

[Stefano Stabellini]

On Wed, 26 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>

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

[Jan Beulich]

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

See question on patch 7. Plus the blank line may want purging here along
with the comment, to be consistent with what you're doing elsewhere.

Jan

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

[Federico Serafini]

On 01/07/24 10:49, Jan Beulich wrote:
> On 26.06.2024 11:28, Federico Serafini wrote:
>> --- a/xen/arch/x86/hvm/vpt.c
>> +++ b/xen/arch/x86/hvm/vpt.c
>> @@ -118,9 +118,11 @@ static int pt_irq_masked(struct periodic_time *pt)
>>               return 0;
>>   
>>           gsi = hvm_isa_irq_to_gsi(pt->irq);
>> +
>> +        /* Fallthrough to check if the interrupt is masked on the IO APIC. */
>> +        fallthrough;
>>       }
>>   
>> -    /* Fallthrough to check if the interrupt is masked on the IO APIC. */
>>       case PTSRC_ioapic:
>>       {
>>           int mask = vioapic_get_mask(v->domain, gsi);
> 
> See question on patch 7. Plus the blank line may want purging here along
> with the comment, to be consistent with what you're doing elsewhere.

When in doubt, I think it is better to clarify rather than not,
however, if no one has anything to say, I will follow your instruction.

--
Federico Serafini, M.Sc.

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

[XEN PATCH v3 09/12] 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>
---
Changes in v3:
- do not return 0 (success).

Al least this version returns an error code but I am not sure about
which one to use.
---
 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..a1e28b3360 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 -EPERM;
             }
         }
         else if ( l1f & _PAGE_RW )
-- 
2.34.1

Re: [XEN PATCH v3 09/12] x86/mm: add defensive return

[Jan Beulich]

On 26.06.2024 11:28, 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>

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

> --- 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 -EPERM;
>              }
>          }
>          else if ( l1f & _PAGE_RW )

I don't like the use of -EPERM here very much, but I understand that there's
no really suitable errno value. I wonder though whether something far more
"exotic" wouldn't be better in such a case, say -EBADMSG or -EADDRNOTAVAIL.
Just to mention it: -EPERM is what failed XSM checks would typically yield,
so from that perspective alone even switching to -EACCES might be a little
bit better.

I further wonder whether, with the assertion catching an issue with the
implementation, we shouldn't consider using BUG() here instead. Input from
in particular the other x86 maintainers appreciated.

Jan

Re: [XEN PATCH v3 09/12] x86/mm: add defensive return

[Alejandro Vallejo]

On Mon Jul 1, 2024 at 9:57 AM BST, Jan Beulich wrote:
> On 26.06.2024 11:28, 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>
>
> Tentatively
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> > --- 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 -EPERM;
> >              }
> >          }
> >          else if ( l1f & _PAGE_RW )
>
> I don't like the use of -EPERM here very much, but I understand that there's
> no really suitable errno value. I wonder though whether something far more
> "exotic" wouldn't be better in such a case, say -EBADMSG or -EADDRNOTAVAIL.
> Just to mention it: -EPERM is what failed XSM checks would typically yield,
> so from that perspective alone even switching to -EACCES might be a little
> bit better.
>

fwiw: EACCES, being typically used for interface version mismatches, would
confuse me a lot.

> I further wonder whether, with the assertion catching an issue with the
> implementation, we shouldn't consider using BUG() here instead. Input from
> in particular the other x86 maintainers appreciated.
>
> Jan

Cheers,
Alejandro

Re: [XEN PATCH v3 09/12] x86/mm: add defensive return

[Jan Beulich]

On 09.07.2024 13:21, Alejandro Vallejo wrote:
> On Mon Jul 1, 2024 at 9:57 AM BST, Jan Beulich wrote:
>> On 26.06.2024 11:28, Federico Serafini wrote:
>>> --- 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 -EPERM;
>>>              }
>>>          }
>>>          else if ( l1f & _PAGE_RW )
>>
>> I don't like the use of -EPERM here very much, but I understand that there's
>> no really suitable errno value. I wonder though whether something far more
>> "exotic" wouldn't be better in such a case, say -EBADMSG or -EADDRNOTAVAIL.
>> Just to mention it: -EPERM is what failed XSM checks would typically yield,
>> so from that perspective alone even switching to -EACCES might be a little
>> bit better.
> 
> fwiw: EACCES, being typically used for interface version mismatches, would
> confuse me a lot.

There's no interface version check anywhere in hypercalls involving
get_page_from_l1e(), I don't think. So I see little room for confusion.

Jan

[XEN PATCH v3 10/12] 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Jan Beulich <jbeulich@suse.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

[XEN PATCH v3 11/12] 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Jan Beulich <jbeulich@suse.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

[XEN PATCH v3 12/12] 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Jan Beulich <jbeulich@suse.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 v3 00/12] x86: address some violations of MISRA C Rule 16.3

[Jan Beulich]

On 26.06.2024 11:27, Federico Serafini wrote:
> This patch series fixes a missing escape in a deviation and addresses some
> violations.
> 
> Federico Serafini (12):
>   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

Just a remark, no strict request to make further re-arrangements: Looks like
what was patch 11 in v2 was now folded into this patch. Yet then why were
other HVM parts left separate:

>   x86/vpt: address a violation of MISRA C Rule 16.3

This and ...

>   x86/mm: add defensive return
>   x86/mpparse: 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

... these two. In general I'd expect splitting / keeping together to be
done consistently within a series, unless of course there's something that
wants keeping separate for other than purely mechanical reasons.

Jan