[XEN PATCH v5 0/8] x86: address some violations of MISRA C Rule 16.3

[XEN PATCH v5 0/8] 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 (8):
  automation/eclair: fix deviation 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/hvm: add defensive statements in unreachable program points
  x86/mm: add defensive return
  x86/mpparse: address a violation of MISRA C Rule 16.3

 automation/eclair_analysis/ECLAIR/deviations.ecl |  5 ++---
 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/hvm/emulate.c                       | 12 +++++++-----
 xen/arch/x86/hvm/hvm.c                           |  5 +++++
 xen/arch/x86/hvm/hypercall.c                     |  2 +-
 xen/arch/x86/hvm/irq.c                           |  1 +
 xen/arch/x86/hvm/pmtimer.c                       |  1 +
 xen/arch/x86/hvm/svm/svm.c                       |  2 ++
 xen/arch/x86/hvm/vlapic.c                        |  1 +
 xen/arch/x86/hvm/vmx/vmcs.c                      |  2 ++
 xen/arch/x86/hvm/vmx/vmx.c                       |  3 ++-
 xen/arch/x86/hvm/vmx/vvmx.c                      |  1 +
 xen/arch/x86/hvm/vpic.c                          |  1 +
 xen/arch/x86/hvm/vpt.c                           |  3 +--
 xen/arch/x86/mm.c                                |  1 +
 xen/arch/x86/mpparse.c                           |  1 +
 xen/arch/x86/traps.c                             |  3 +++
 20 files changed, 40 insertions(+), 12 deletions(-)

-- 
2.34.1

[XEN PATCH v5 1/8] automation/eclair: fix deviation of MISRA C Rule 16.3

[Federico Serafini]

Add missing escape for the final dot of the fallthrough comment,
extend the search of a fallthrough comment up to 2 lines after the last
statement and improve the text of the justification.

Fixes: a128d8da91 ("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>
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index 0af1cb93d1..88b49c23d8 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -482,9 +482,8 @@ safe."
 -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/BUG\\(\\);/))))"}
 -doc_end
 
--doc_begin="Switch clauses not ending with the break statement are safe if an
-explicit comment indicating the fallthrough intention is present."
--config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(^(?s).*/\\* [fF]all ?through.? \\*/.*$,0..1))))"}
+-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..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 v5 2/8] 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 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

[XEN PATCH v5 3/8] 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Jan Beulich <jbeulich@suse.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 ee91fc56b1..7a9299ae6c 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

[XEN PATCH v5 4/8] 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 v5 5/8] 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.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
Changes in v5:
- separate mechanical changes from the ones that report an error to the caller
  (see patch 6 of this series).
Changes in v4:
- do not separate different parts of HVM in different patches;
- re-arrange fallthrough positioning to comply with Coverity.
Changes in v3:
- squashed here modifications of pmtimer.c;
- no blank line after fallthrough;
- better indentation of fallthrough.
---
 xen/arch/x86/hvm/emulate.c   | 11 ++++++-----
 xen/arch/x86/hvm/hvm.c       |  1 +
 xen/arch/x86/hvm/hypercall.c |  2 +-
 xen/arch/x86/hvm/irq.c       |  1 +
 xen/arch/x86/hvm/pmtimer.c   |  1 +
 xen/arch/x86/hvm/svm/svm.c   |  2 ++
 xen/arch/x86/hvm/vlapic.c    |  1 +
 xen/arch/x86/hvm/vmx/vmx.c   |  3 ++-
 xen/arch/x86/hvm/vmx/vvmx.c  |  1 +
 xen/arch/x86/hvm/vpic.c      |  1 +
 xen/arch/x86/hvm/vpt.c       |  3 +--
 11 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 02e378365b..135aa6fc22 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();
     }
@@ -396,8 +396,7 @@ static int hvmemul_acquire_page(unsigned long gmfn, struct page_info **page)
 
     default:
         ASSERT_UNREACHABLE();
-        /* Fallthrough */
-
+        fallthrough;
     case -EINVAL:
         return X86EMUL_UNHANDLEABLE;
     }
@@ -2764,6 +2763,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
         /* fallthrough */
     default:
         hvm_emulate_writeback(&ctxt);
+        break;
     }
 
     return rc;
@@ -2799,10 +2799,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 +2819,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..1d32f473a4 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5283,6 +5283,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..c1bd17571e 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -110,7 +110,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 */
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 988250dbc1..92bb10c504 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -681,6 +681,7 @@ static void cf_check svm_get_segment_register(
         ASSERT_UNREACHABLE();
         domain_crash(v->domain);
         *reg = (struct segment_register){};
+        break;
     }
 }
 
@@ -2416,6 +2417,7 @@ static void cf_check svm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
         printk(XENLOG_G_ERR "%s(%pv, 0x%08x, 0x%016"PRIx64") Bad register\n",
                __func__, v, reg, val);
         domain_crash(d);
+        break;
     }
 }
 
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;
     }
 }
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f16faa6a61..46911c3e1c 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2745,6 +2745,7 @@ static void cf_check vmx_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
         printk(XENLOG_G_ERR "%s(%pv, 0x%08x, 0x%016"PRIx64") Bad register\n",
                __func__, v, reg, val);
         domain_crash(d);
+        break;
     }
     vmx_vmcs_exit(v);
 }
@@ -3393,7 +3394,7 @@ static int cf_check vmx_msr_read_intercept(
         *msr_content |= MSR_IA32_MISC_ENABLE_BTS_UNAVAIL |
                        MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL;
         /* Perhaps vpmu will change some bits. */
-        /* FALLTHROUGH */
+        fallthrough;
     case MSR_P6_PERFCTR(0)...MSR_P6_PERFCTR(7):
     case MSR_P6_EVNTSEL(0)...MSR_P6_EVNTSEL(3):
     case MSR_CORE_PERF_FIXED_CTR0...MSR_CORE_PERF_FIXED_CTR2:
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 39290c9861..c05e0e9326 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2768,6 +2768,7 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
         gprintk(XENLOG_ERR, "Unhandled nested vmexit: reason %u\n",
                 exit_reason);
         domain_crash(v->domain);
+        break;
     }
 
     return ( nvcpu->nv_vmexit_pending == 1 );
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++;
diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index e1d6845a28..5e7b9a9f66 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -119,8 +119,7 @@ static int pt_irq_masked(struct periodic_time *pt)
 
         gsi = hvm_isa_irq_to_gsi(pt->irq);
     }
-
-    /* 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 v5 5/8] x86/hvm: address violations of MISRA C Rule 16.3

[Jan Beulich]

On 29.07.2024 11:00, 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.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

[XEN PATCH v5 6/8] x86/hvm: add defensive statements in unreachable program points

[Federico Serafini]

As a defensive measure, make sure to signal an error to the caller
if an unreachable program point is reached.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/x86/hvm/emulate.c  | 1 +
 xen/arch/x86/hvm/hvm.c      | 4 ++++
 xen/arch/x86/hvm/vmx/vmcs.c | 2 ++
 3 files changed, 7 insertions(+)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 135aa6fc22..b6ca5cb9d1 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2673,6 +2673,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
 
     default:
         ASSERT_UNREACHABLE();
+        return X86EMUL_UNHANDLEABLE;
     }
 
     if ( hvmemul_ctxt->ctxt.retire.singlestep )
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 1d32f473a4..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;
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 9b6dc51f36..5787110a56 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1446,6 +1446,7 @@ struct vmx_msr_entry *vmx_find_msr(const struct vcpu *v, uint32_t msr,
 
     default:
         ASSERT_UNREACHABLE();
+        break;
     }
 
     if ( !start )
@@ -1598,6 +1599,7 @@ int vmx_del_msr(struct vcpu *v, uint32_t msr, enum vmx_msr_list_type type)
 
     default:
         ASSERT_UNREACHABLE();
+        return -EINVAL;
     }
 
     if ( !start )
-- 
2.34.1

Re: [XEN PATCH v5 6/8] x86/hvm: add defensive statements in unreachable program points

[Jan Beulich]

On 29.07.2024 11:00, Federico Serafini wrote:
> As a defensive measure, make sure to signal an error to the caller
> if an unreachable program point is reached.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

[XEN PATCH v5 7/8] 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>
---
No changes from v3 and v4, further feedback on this thread would be appreciated:
https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html
---
 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 95795567f2..8973e76dff 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -917,6 +917,7 @@ get_page_from_l1e(
                 return 0;
             default:
                 ASSERT_UNREACHABLE();
+                return -EPERM;
             }
         }
         else if ( l1f & _PAGE_RW )
-- 
2.34.1

Re: [XEN PATCH v5 7/8] x86/mm: add defensive return

[Stefano Stabellini]

On Mon, 29 Jul 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>

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

> ---
> No changes from v3 and v4, further feedback on this thread would be appreciated:
> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html
> ---
>  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 95795567f2..8973e76dff 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -917,6 +917,7 @@ get_page_from_l1e(
>                  return 0;
>              default:
>                  ASSERT_UNREACHABLE();
> +                return -EPERM;
>              }
>          }
>          else if ( l1f & _PAGE_RW )
> -- 
> 2.34.1
> 
> 

Re: [XEN PATCH v5 7/8] x86/mm: add defensive return

[Stefano Stabellini]

On Mon, 29 Jul 2024, Stefano Stabellini wrote:
> On Mon, 29 Jul 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>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> > ---
> > No changes from v3 and v4, further feedback on this thread would be appreciated:
> > https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html

Looking at the older threads, I looks like Jan suggested EACCES, I also
think it is marginally better. My R-b stands also for EACCES. Jan, I
think you should go ahead and fix on commit


> > ---
> >  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 95795567f2..8973e76dff 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -917,6 +917,7 @@ get_page_from_l1e(
> >                  return 0;
> >              default:
> >                  ASSERT_UNREACHABLE();
> > +                return -EPERM;
> >              }
> >          }
> >          else if ( l1f & _PAGE_RW )
> > -- 
> > 2.34.1
> > 
> > 
> 

Re: [XEN PATCH v5 7/8] x86/mm: add defensive return

[Jan Beulich]

On 29.08.2024 02:35, Stefano Stabellini wrote:
> On Mon, 29 Jul 2024, Stefano Stabellini wrote:
>> On Mon, 29 Jul 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>
>>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>>> ---
>>> No changes from v3 and v4, further feedback on this thread would be appreciated:
>>> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html
> 
> Looking at the older threads, I looks like Jan suggested EACCES, I also
> think it is marginally better. My R-b stands also for EACCES. Jan, I
> think you should go ahead and fix on commit

No, I very definitely want a 2nd x86 maintainer opinion here. Or a better
suggestion for the error code to use by anyone. After all, as you confirm,
EACCES is only marginally better.

Jan

Re: [XEN PATCH v5 7/8] x86/mm: add defensive return

[Roger Pau Monné]

On Thu, Aug 29, 2024 at 09:04:37AM +0200, Jan Beulich wrote:
> On 29.08.2024 02:35, Stefano Stabellini wrote:
> > On Mon, 29 Jul 2024, Stefano Stabellini wrote:
> >> On Mon, 29 Jul 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>
> >>
> >> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>
> >>> ---
> >>> No changes from v3 and v4, further feedback on this thread would be appreciated:
> >>> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html
> > 
> > Looking at the older threads, I looks like Jan suggested EACCES, I also
> > think it is marginally better. My R-b stands also for EACCES. Jan, I
> > think you should go ahead and fix on commit
> 
> No, I very definitely want a 2nd x86 maintainer opinion here. Or a better
> suggestion for the error code to use by anyone. After all, as you confirm,
> EACCES is only marginally better.

Hm, the only alternative I could suggest is using ERANGE, to signal
the value of opt_mmio_relax is out of the expected range, however that
could be confusing for the callers?

One benefit of using ERANGE is that there's currently no return path
in get_page_from_l1e() with that error code, so it would be very easy
to spot when an unexpected value of opt_mmio_relax is found.  However
there are no guarantees that further error paths might use that error
code.

Thanks, Roger.

Re: [XEN PATCH v5 7/8] x86/mm: add defensive return

[Andrew Cooper]

On 30/08/2024 10:18 am, Roger Pau Monné wrote:
> On Thu, Aug 29, 2024 at 09:04:37AM +0200, Jan Beulich wrote:
>> On 29.08.2024 02:35, Stefano Stabellini wrote:
>>> On Mon, 29 Jul 2024, Stefano Stabellini wrote:
>>>> On Mon, 29 Jul 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>
>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>
>>>>> ---
>>>>> No changes from v3 and v4, further feedback on this thread would be appreciated:
>>>>> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html
>>> Looking at the older threads, I looks like Jan suggested EACCES, I also
>>> think it is marginally better. My R-b stands also for EACCES. Jan, I
>>> think you should go ahead and fix on commit
>> No, I very definitely want a 2nd x86 maintainer opinion here. Or a better
>> suggestion for the error code to use by anyone. After all, as you confirm,
>> EACCES is only marginally better.
> Hm, the only alternative I could suggest is using ERANGE, to signal
> the value of opt_mmio_relax is out of the expected range, however that
> could be confusing for the callers?
>
> One benefit of using ERANGE is that there's currently no return path
> in get_page_from_l1e() with that error code, so it would be very easy
> to spot when an unexpected value of opt_mmio_relax is found.  However
> there are no guarantees that further error paths might use that error
> code.

EPERM and EACCES are both very wrong here.  They imply something that is
simply not appropriate in this context.

We use EILSEQ elsewhere for believed-impossible conditions where we need
an errno of some kind.  I suggest we use it here too.

~Andrew

Re: [XEN PATCH v5 7/8] x86/mm: add defensive return

[Roger Pau Monné]

On Fri, Aug 30, 2024 at 08:17:40PM +0100, Andrew Cooper wrote:
> On 30/08/2024 10:18 am, Roger Pau Monné wrote:
> > On Thu, Aug 29, 2024 at 09:04:37AM +0200, Jan Beulich wrote:
> >> On 29.08.2024 02:35, Stefano Stabellini wrote:
> >>> On Mon, 29 Jul 2024, Stefano Stabellini wrote:
> >>>> On Mon, 29 Jul 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>
> >>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>>>
> >>>>> ---
> >>>>> No changes from v3 and v4, further feedback on this thread would be appreciated:
> >>>>> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html
> >>> Looking at the older threads, I looks like Jan suggested EACCES, I also
> >>> think it is marginally better. My R-b stands also for EACCES. Jan, I
> >>> think you should go ahead and fix on commit
> >> No, I very definitely want a 2nd x86 maintainer opinion here. Or a better
> >> suggestion for the error code to use by anyone. After all, as you confirm,
> >> EACCES is only marginally better.
> > Hm, the only alternative I could suggest is using ERANGE, to signal
> > the value of opt_mmio_relax is out of the expected range, however that
> > could be confusing for the callers?
> >
> > One benefit of using ERANGE is that there's currently no return path
> > in get_page_from_l1e() with that error code, so it would be very easy
> > to spot when an unexpected value of opt_mmio_relax is found.  However
> > there are no guarantees that further error paths might use that error
> > code.
> 
> EPERM and EACCES are both very wrong here.  They imply something that is
> simply not appropriate in this context.
> 
> We use EILSEQ elsewhere for believed-impossible conditions where we need
> an errno of some kind.  I suggest we use it here too.

Fine with me.  With that:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

[XEN PATCH v5 8/8] 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