[XEN PATCH v2 0/3] xen: address violations of MISRA C:2012 Rule 13.1

[XEN PATCH v2 0/3] xen: address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin]

This series contains some changes and deviation to address
reports of MISRA C:2012 Rule 13.1:
Initializer lists shall not contain persistent side effects

An assignment has been moved outside the initializer lists, other
violations have been deviated with SAF comments.

Function calls do not necessarily have side-effects, in these cases this
patch proposes to add ECLAIR pure, const or noeffect attributes whenever
possible.

ECLAIR pure and const attributes have the same definition of the corresponding
GCC attributes, noeffect attribute has the following definition:
"like pure but can also read volatile variable not triggering side effects"

It has been decided to avoid GCC/clang attributes to avoid potentially
dangerous optimisations from the compiler.

Changes in v2:
- prefer ECLAIR attributes over GCC attributes;
- replace ECL deviations with equivalent SAF deviations;
- deviate violations caused by harmless volatile asm;
- deviate violations caused by debug and logging macros/functions.

Simone Ballarin (3):
  automation/eclair: tag function calls to address violations of MISRA
    C:2012 Rule 13.1
  xen/arm: add SAF deviation for debugging and logging effects
  xen: address violations of MISRA C:2012 Rule 13.1

 .../ECLAIR/call_properties.ecl                | 22 +++++++++++++++++++
 docs/misra/safe.json                          | 16 ++++++++++++++
 xen/arch/arm/device.c                         |  1 +
 xen/arch/arm/guestcopy.c                      |  4 ++++
 xen/arch/x86/hvm/hvm.c                        |  1 +
 xen/common/sched/core.c                       |  3 +++
 xen/drivers/char/ns16550.c                    |  6 +++--
 7 files changed, 51 insertions(+), 2 deletions(-)

-- 
2.34.1

[XEN PATCH v2 1/3] automation/eclair: tag function calls to address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin]

Rule 13.1: Initializer lists shall not contain persistent side effects

Invocations of functions in initializer lists cause violations of rule
13.1 if the called functions are not tagged with __attribute_pure__ or
__attribute_const__ as they can produce persistent side effects.

Handling these violations with  attributes is not always possible: the
pure and const attributes may cause unwanted and potentially dangerous
optimisations.

To avoid this problem ECLAIR allows using the same attributes in the
-call_properties setting. Additionally, it adds the noeffect attribute
with the following definition:
"like pure but can also read volatile variable not triggering side effects"

These patch tags some functions used in initializer lists to address
violations of Rule 13.1.

No functional changes.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v2:
New patch partly based on "xen/arm: address violations of MISRA C:2012 Rule 13.1"
and "xen/include: add pure and const attributes". This new patch uses
ECL tagging instead of compiler attributes.
---
 .../ECLAIR/call_properties.ecl                | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/call_properties.ecl b/automation/eclair_analysis/ECLAIR/call_properties.ecl
index 3f7794bf8b..c2b2a6182e 100644
--- a/automation/eclair_analysis/ECLAIR/call_properties.ecl
+++ b/automation/eclair_analysis/ECLAIR/call_properties.ecl
@@ -73,6 +73,17 @@
 -call_properties+={"macro(^va_start$)", {"pointee_write(1=always)", "pointee_read(1=never)", "taken()"}}
 -call_properties+={"macro(^memcmp$)", {"pointee_write(1..2=never)", "taken()"}}
 -call_properties+={"macro(^memcpy$)", {"pointee_write(1=always&&2..=never)", "pointee_read(1=never&&2..=always)", "taken()"}}
+-call_properties+={"name(get_cpu_info)",{pure}}
+-call_properties+={"name(pdx_to_pfn)",{pure}}
+-call_properties+={"name(is_pci_passthrough_enabled)",{const}}
+-call_properties+={"name(get_cycles)", {"noeffect"}}
+-call_properties+={"name(msi_gflags)",{const}}
+-call_properties+={"name(hvm_save_size)",{pure}}
+-call_properties+={"name(cpu_has)",{pure}}
+-call_properties+={"name(boot_cpu_has)",{pure}}
+-call_properties+={"name(get_cpu_info)",{pure}}
+-call_properties+={"name(put_pte_flags)",{const}}
+-call_properties+={"name(is_pv_vcpu)",{pure}}
 
 -doc_begin="Property inferred as a consequence of the semantics of device_tree_get_reg"
 -call_properties+={"name(acquire_static_memory_bank)", {"pointee_write(4..=always)", "pointee_read(4..=never)", "taken()"}}
@@ -104,3 +115,14 @@ Furthermore, their uses do initialize the involved variables as needed by futher
 -call_properties+={"macro(^(__)?(raw_)?copy_from_(paddr|guest|compat)(_offset)?$)", {"pointee_write(1=always)", "pointee_read(1=never)", "taken()"}}
 -call_properties+={"macro(^(__)?copy_to_(guest|compat)(_offset)?$)", {"pointee_write(2=always)", "pointee_read(2=never)", "taken()"}}
 -doc_end
+
+-doc_begin="Functions generated by build_atomic_read cannot be considered pure
+since the input pointer is volatile, but they do not produce any persistent side
+effect."
+-call_properties+={"^read_u(8|16|32|64|int)_atomic.*$", {noeffect}}
+-doc_end
+
+-doc_begin="Functions generated by TYPE_SAFE are const."
+-call_properties+={"^(mfn|gfn|pfn)_x\\(.*$",{const}}
+-call_properties+={"^_(mfn|gfn|pfn)\\(.*$",{const}}
+-doc_end
-- 
2.34.1

Re: [XEN PATCH v2 1/3] automation/eclair: tag function calls to address violations of MISRA C:2012 Rule 13.1

[Stefano Stabellini]

On Fri, 24 Nov 2023, Simone Ballarin wrote:
> Rule 13.1: Initializer lists shall not contain persistent side effects
> 
> Invocations of functions in initializer lists cause violations of rule
> 13.1 if the called functions are not tagged with __attribute_pure__ or
> __attribute_const__ as they can produce persistent side effects.
> 
> Handling these violations with  attributes is not always possible: the
> pure and const attributes may cause unwanted and potentially dangerous
> optimisations.
> 
> To avoid this problem ECLAIR allows using the same attributes in the
> -call_properties setting. Additionally, it adds the noeffect attribute
> with the following definition:
> "like pure but can also read volatile variable not triggering side effects"
> 
> These patch tags some functions used in initializer lists to address
> violations of Rule 13.1.
> 
> No functional changes.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

Ideally we should also list them somewhere in a document, maybe
docs/misra/deviations.rst? Or a new doc? It would be best if this info
wouldn't only exist in call_properties.ecl.

But give that the below is OK:
Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Changes in v2:
> New patch partly based on "xen/arm: address violations of MISRA C:2012 Rule 13.1"
> and "xen/include: add pure and const attributes". This new patch uses
> ECL tagging instead of compiler attributes.
> ---
>  .../ECLAIR/call_properties.ecl                | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/call_properties.ecl b/automation/eclair_analysis/ECLAIR/call_properties.ecl
> index 3f7794bf8b..c2b2a6182e 100644
> --- a/automation/eclair_analysis/ECLAIR/call_properties.ecl
> +++ b/automation/eclair_analysis/ECLAIR/call_properties.ecl
> @@ -73,6 +73,17 @@
>  -call_properties+={"macro(^va_start$)", {"pointee_write(1=always)", "pointee_read(1=never)", "taken()"}}
>  -call_properties+={"macro(^memcmp$)", {"pointee_write(1..2=never)", "taken()"}}
>  -call_properties+={"macro(^memcpy$)", {"pointee_write(1=always&&2..=never)", "pointee_read(1=never&&2..=always)", "taken()"}}
> +-call_properties+={"name(get_cpu_info)",{pure}}
> +-call_properties+={"name(pdx_to_pfn)",{pure}}
> +-call_properties+={"name(is_pci_passthrough_enabled)",{const}}
> +-call_properties+={"name(get_cycles)", {"noeffect"}}
> +-call_properties+={"name(msi_gflags)",{const}}
> +-call_properties+={"name(hvm_save_size)",{pure}}
> +-call_properties+={"name(cpu_has)",{pure}}
> +-call_properties+={"name(boot_cpu_has)",{pure}}
> +-call_properties+={"name(get_cpu_info)",{pure}}
> +-call_properties+={"name(put_pte_flags)",{const}}
> +-call_properties+={"name(is_pv_vcpu)",{pure}}
>  
>  -doc_begin="Property inferred as a consequence of the semantics of device_tree_get_reg"
>  -call_properties+={"name(acquire_static_memory_bank)", {"pointee_write(4..=always)", "pointee_read(4..=never)", "taken()"}}
> @@ -104,3 +115,14 @@ Furthermore, their uses do initialize the involved variables as needed by futher
>  -call_properties+={"macro(^(__)?(raw_)?copy_from_(paddr|guest|compat)(_offset)?$)", {"pointee_write(1=always)", "pointee_read(1=never)", "taken()"}}
>  -call_properties+={"macro(^(__)?copy_to_(guest|compat)(_offset)?$)", {"pointee_write(2=always)", "pointee_read(2=never)", "taken()"}}
>  -doc_end
> +
> +-doc_begin="Functions generated by build_atomic_read cannot be considered pure
> +since the input pointer is volatile, but they do not produce any persistent side
> +effect."
> +-call_properties+={"^read_u(8|16|32|64|int)_atomic.*$", {noeffect}}
> +-doc_end
> +
> +-doc_begin="Functions generated by TYPE_SAFE are const."
> +-call_properties+={"^(mfn|gfn|pfn)_x\\(.*$",{const}}
> +-call_properties+={"^_(mfn|gfn|pfn)\\(.*$",{const}}
> +-doc_end
> -- 
> 2.34.1
> 

Re: [XEN PATCH v2 1/3] automation/eclair: tag function calls to address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin]

On 02/12/23 04:19, Stefano Stabellini wrote:
> On Fri, 24 Nov 2023, Simone Ballarin wrote:
>> Rule 13.1: Initializer lists shall not contain persistent side effects
>>
>> Invocations of functions in initializer lists cause violations of rule
>> 13.1 if the called functions are not tagged with __attribute_pure__ or
>> __attribute_const__ as they can produce persistent side effects.
>>
>> Handling these violations with  attributes is not always possible: the
>> pure and const attributes may cause unwanted and potentially dangerous
>> optimisations.
>>
>> To avoid this problem ECLAIR allows using the same attributes in the
>> -call_properties setting. Additionally, it adds the noeffect attribute
>> with the following definition:
>> "like pure but can also read volatile variable not triggering side effects"
>>
>> These patch tags some functions used in initializer lists to address
>> violations of Rule 13.1.
>>
>> No functional changes.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> Ideally we should also list them somewhere in a document, maybe
> docs/misra/deviations.rst? Or a new doc? It would be best if this info
> wouldn't only exist in call_properties.ecl.

They are not actually deviations, but information that can help
document the code: I suggest creating a new document.

Then, ECLAIR or any other tool will be able to retrieve these properties
directly from this new file.

If you agree I will do it in a separate patch.

> 
> But give that the below is OK:
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
>> ---
>> Changes in v2:
>> New patch partly based on "xen/arm: address violations of MISRA C:2012 Rule 13.1"
>> and "xen/include: add pure and const attributes". This new patch uses
>> ECL tagging instead of compiler attributes.
>> ---
>>   .../ECLAIR/call_properties.ecl                | 22 +++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/automation/eclair_analysis/ECLAIR/call_properties.ecl b/automation/eclair_analysis/ECLAIR/call_properties.ecl
>> index 3f7794bf8b..c2b2a6182e 100644
>> --- a/automation/eclair_analysis/ECLAIR/call_properties.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/call_properties.ecl
>> @@ -73,6 +73,17 @@
>>   -call_properties+={"macro(^va_start$)", {"pointee_write(1=always)", "pointee_read(1=never)", "taken()"}}
>>   -call_properties+={"macro(^memcmp$)", {"pointee_write(1..2=never)", "taken()"}}
>>   -call_properties+={"macro(^memcpy$)", {"pointee_write(1=always&&2..=never)", "pointee_read(1=never&&2..=always)", "taken()"}}
>> +-call_properties+={"name(get_cpu_info)",{pure}}
>> +-call_properties+={"name(pdx_to_pfn)",{pure}}
>> +-call_properties+={"name(is_pci_passthrough_enabled)",{const}}
>> +-call_properties+={"name(get_cycles)", {"noeffect"}}
>> +-call_properties+={"name(msi_gflags)",{const}}
>> +-call_properties+={"name(hvm_save_size)",{pure}}
>> +-call_properties+={"name(cpu_has)",{pure}}
>> +-call_properties+={"name(boot_cpu_has)",{pure}}
>> +-call_properties+={"name(get_cpu_info)",{pure}}
>> +-call_properties+={"name(put_pte_flags)",{const}}
>> +-call_properties+={"name(is_pv_vcpu)",{pure}}
>>   
>>   -doc_begin="Property inferred as a consequence of the semantics of device_tree_get_reg"
>>   -call_properties+={"name(acquire_static_memory_bank)", {"pointee_write(4..=always)", "pointee_read(4..=never)", "taken()"}}
>> @@ -104,3 +115,14 @@ Furthermore, their uses do initialize the involved variables as needed by futher
>>   -call_properties+={"macro(^(__)?(raw_)?copy_from_(paddr|guest|compat)(_offset)?$)", {"pointee_write(1=always)", "pointee_read(1=never)", "taken()"}}
>>   -call_properties+={"macro(^(__)?copy_to_(guest|compat)(_offset)?$)", {"pointee_write(2=always)", "pointee_read(2=never)", "taken()"}}
>>   -doc_end
>> +
>> +-doc_begin="Functions generated by build_atomic_read cannot be considered pure
>> +since the input pointer is volatile, but they do not produce any persistent side
>> +effect."
>> +-call_properties+={"^read_u(8|16|32|64|int)_atomic.*$", {noeffect}}
>> +-doc_end
>> +
>> +-doc_begin="Functions generated by TYPE_SAFE are const."
>> +-call_properties+={"^(mfn|gfn|pfn)_x\\(.*$",{const}}
>> +-call_properties+={"^_(mfn|gfn|pfn)\\(.*$",{const}}
>> +-doc_end
>> -- 
>> 2.34.1
>>

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH v2 1/3] automation/eclair: tag function calls to address violations of MISRA C:2012 Rule 13.1

[Stefano Stabellini]

On Mon, 4 Dec 2023, Simone Ballarin wrote:
> On 02/12/23 04:19, Stefano Stabellini wrote:
> > On Fri, 24 Nov 2023, Simone Ballarin wrote:
> > > Rule 13.1: Initializer lists shall not contain persistent side effects
> > > 
> > > Invocations of functions in initializer lists cause violations of rule
> > > 13.1 if the called functions are not tagged with __attribute_pure__ or
> > > __attribute_const__ as they can produce persistent side effects.
> > > 
> > > Handling these violations with  attributes is not always possible: the
> > > pure and const attributes may cause unwanted and potentially dangerous
> > > optimisations.
> > > 
> > > To avoid this problem ECLAIR allows using the same attributes in the
> > > -call_properties setting. Additionally, it adds the noeffect attribute
> > > with the following definition:
> > > "like pure but can also read volatile variable not triggering side
> > > effects"
> > > 
> > > These patch tags some functions used in initializer lists to address
> > > violations of Rule 13.1.
> > > 
> > > No functional changes.
> > > 
> > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> > 
> > Ideally we should also list them somewhere in a document, maybe
> > docs/misra/deviations.rst? Or a new doc? It would be best if this info
> > wouldn't only exist in call_properties.ecl.
> 
> They are not actually deviations, but information that can help
> document the code: I suggest creating a new document.
> 
> Then, ECLAIR or any other tool will be able to retrieve these properties
> directly from this new file.
> 
> If you agree I will do it in a separate patch.

Yes a separate patch is fine. Please don't forget :-)

[XEN PATCH v2 2/3] xen/arm: add SAF deviation for debugging and logging effects

[Simone Ballarin]

Rule 13.1: Initializer lists shall not contain persistent side effects

Effects caused by debug/logging macros and functions (like ASSERT, __bad_atomic_size,
LOG, etc ...) that crash execution or produce logs are not dangerous in initializer
lists. The evaluation order in abnormal conditions is not relevant. Evaluation order
of logging effects is always safe.

This patch deviates violations using SAF commits caused by debug/logging macros and
functions.

Asm volatile statements in initializer lists that do not perform any persistent side
effect are safe: this patch deviates violations caused by uses of the current macro
(that contains an asm volatile) in initializer lists.

No functional changes.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v2:
New patch based on the discussion for "xen/arm: address violations of MISRA C:2012 Rule 13.1".
---
 docs/misra/safe.json     | 16 ++++++++++++++++
 xen/arch/arm/device.c    |  1 +
 xen/arch/arm/guestcopy.c |  4 ++++
 xen/arch/x86/hvm/hvm.c   |  1 +
 xen/common/sched/core.c  |  3 +++
 5 files changed, 25 insertions(+)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 952324f85c..69ab526084 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -28,6 +28,22 @@
         },
         {
             "id": "SAF-3-safe",
+            "analyser": {
+                "eclair": "MC3R1.R13.1"
+            },
+            "name": "MC3R1.R13.1: effects for debugging and logging",
+            "text": "Effects for debugging and loggings reasons that crash execution or produce logs are allowed in initializer lists. The evaluation order in abnormal conditions is not relevant."
+        },
+        {
+            "id": "SAF-4-safe",
+            "analyser": {
+                "eclair": "MC3R1.R13.1"
+            },
+            "name": "MC3R1.R13.1: volatile asm statements that do not perform any persistent side effect",
+            "text": "Volatile asm statements in an initializer list if do not perform persistent side effects are safe."
+        },
+        {
+            "id": "SAF-5-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 1f631d3274..fa331f164d 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -331,6 +331,7 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
         .p2mt = p2mt,
         .skip_mapping = !own_device ||
                         (is_pci_passthrough_enabled() &&
+                        /* SAF-3-safe effects for debugging/logging reasons are safe */
                         (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
         .iomem_ranges = iomem_ranges,
         .irq_ranges = irq_ranges
diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index 6716b03561..31b809ea08 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -110,18 +110,21 @@ static unsigned long copy_guest(void *buf, uint64_t addr, unsigned int len,
 unsigned long raw_copy_to_guest(void *to, const void *from, unsigned int len)
 {
     return copy_guest((void *)from, (vaddr_t)to, len,
+                      /* SAF-4-safe No persistent side effects */
                       GVA_INFO(current), COPY_to_guest | COPY_linear);
 }
 
 unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
                                              unsigned int len)
 {
+    /* SAF-4-safe No persistent side effects */
     return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current),
                       COPY_to_guest | COPY_flush_dcache | COPY_linear);
 }
 
 unsigned long raw_clear_guest(void *to, unsigned int len)
 {
+    /* SAF-4-safe No persistent side effects */
     return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current),
                       COPY_to_guest | COPY_linear);
 }
@@ -129,6 +132,7 @@ unsigned long raw_clear_guest(void *to, unsigned int len)
 unsigned long raw_copy_from_guest(void *to, const void __user *from,
                                   unsigned int len)
 {
+    /* SAF-4-safe No persistent side effects */
     return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current),
                       COPY_from_guest | COPY_linear);
 }
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 35a30df3b1..24fd02b363 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1700,6 +1700,7 @@ void hvm_hlt(unsigned int eflags)
 
     do_sched_op(SCHEDOP_block, guest_handle_from_ptr(NULL, void));
 
+    /* SAF-3-safe effects for debugging/logging reasons are safe */
     HVMTRACE_1D(HLT, /* pending = */ vcpu_runnable(curr));
 }
 
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index eba0cea4bb..b1b94c8689 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -1517,6 +1517,7 @@ long vcpu_yield(void)
 
     SCHED_STAT_CRANK(vcpu_yield);
 
+    /* SAF-4-safe No persistent side effects */
     TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
     raise_softirq(SCHEDULE_SOFTIRQ);
     return 0;
@@ -1895,6 +1896,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( copy_from_guest(&sched_shutdown, arg, 1) )
             break;
 
+        /* SAF-4-safe No persistent side effects */
         TRACE_3D(TRC_SCHED_SHUTDOWN,
                  current->domain->domain_id, current->vcpu_id,
                  sched_shutdown.reason);
@@ -1912,6 +1914,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( copy_from_guest(&sched_shutdown, arg, 1) )
             break;
 
+        /* SAF-4-safe No persistent side effects */
         TRACE_3D(TRC_SCHED_SHUTDOWN_CODE,
                  d->domain_id, current->vcpu_id, sched_shutdown.reason);
 
-- 
2.34.1

Re: [XEN PATCH v2 2/3] xen/arm: add SAF deviation for debugging and logging effects

[Jan Beulich]

On 24.11.2023 18:29, Simone Ballarin wrote:
> Rule 13.1: Initializer lists shall not contain persistent side effects
> 
> Effects caused by debug/logging macros and functions (like ASSERT, __bad_atomic_size,
> LOG, etc ...) that crash execution or produce logs are not dangerous in initializer
> lists. The evaluation order in abnormal conditions is not relevant. Evaluation order
> of logging effects is always safe.
> 
> This patch deviates violations using SAF commits caused by debug/logging macros and
> functions.
> 
> Asm volatile statements in initializer lists that do not perform any persistent side
> effect are safe: this patch deviates violations caused by uses of the current macro
> (that contains an asm volatile) in initializer lists.
> 
> No functional changes.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> ---
> Changes in v2:
> New patch based on the discussion for "xen/arm: address violations of MISRA C:2012 Rule 13.1".
> ---
>  docs/misra/safe.json     | 16 ++++++++++++++++
>  xen/arch/arm/device.c    |  1 +
>  xen/arch/arm/guestcopy.c |  4 ++++
>  xen/arch/x86/hvm/hvm.c   |  1 +
>  xen/common/sched/core.c  |  3 +++

The latter two don't really fit the title prefix.

> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -28,6 +28,22 @@
>          },
>          {
>              "id": "SAF-3-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.R13.1"
> +            },
> +            "name": "MC3R1.R13.1: effects for debugging and logging",
> +            "text": "Effects for debugging and loggings reasons that crash execution or produce logs are allowed in initializer lists. The evaluation order in abnormal conditions is not relevant."
> +        },

I'm wary of this statement. Order may not matter much anymore _after_ an
abnormal condition was encountered, but in the course of determining whether
an abnormal condition might have been reached it may very well still matter.

> +        {
> +            "id": "SAF-4-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.R13.1"
> +            },
> +            "name": "MC3R1.R13.1: volatile asm statements that do not perform any persistent side effect",
> +            "text": "Volatile asm statements in an initializer list if do not perform persistent side effects are safe."

Since each respective comment ought to affect just a single asm(), I think
this wants writing in singular. I further don't think it is useful for
"text" to largely restate what "name" already says.

> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -331,6 +331,7 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
>          .p2mt = p2mt,
>          .skip_mapping = !own_device ||
>                          (is_pci_passthrough_enabled() &&
> +                        /* SAF-3-safe effects for debugging/logging reasons are safe */
>                          (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),

What's the debugging / logging reason on the commented line?

> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -110,18 +110,21 @@ static unsigned long copy_guest(void *buf, uint64_t addr, unsigned int len,
>  unsigned long raw_copy_to_guest(void *to, const void *from, unsigned int len)
>  {
>      return copy_guest((void *)from, (vaddr_t)to, len,
> +                      /* SAF-4-safe No persistent side effects */
>                        GVA_INFO(current), COPY_to_guest | COPY_linear);
>  }
>  
>  unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
>                                               unsigned int len)
>  {
> +    /* SAF-4-safe No persistent side effects */
>      return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current),
>                        COPY_to_guest | COPY_flush_dcache | COPY_linear);
>  }
>  
>  unsigned long raw_clear_guest(void *to, unsigned int len)
>  {
> +    /* SAF-4-safe No persistent side effects */
>      return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current),
>                        COPY_to_guest | COPY_linear);
>  }
> @@ -129,6 +132,7 @@ unsigned long raw_clear_guest(void *to, unsigned int len)
>  unsigned long raw_copy_from_guest(void *to, const void __user *from,
>                                    unsigned int len)
>  {
> +    /* SAF-4-safe No persistent side effects */
>      return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current),
>                        COPY_from_guest | COPY_linear);
>  }

I can only guess that in all four of these it's the use of "current" which
requires the comment. Yet imo that either needs making explicit, or such a
comment shouldn't go on use sites of "current", but on its definition site.

> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -1517,6 +1517,7 @@ long vcpu_yield(void)
>  
>      SCHED_STAT_CRANK(vcpu_yield);
>  
> +    /* SAF-4-safe No persistent side effects */
>      TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
>      raise_softirq(SCHEDULE_SOFTIRQ);
>      return 0;
> @@ -1895,6 +1896,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( copy_from_guest(&sched_shutdown, arg, 1) )
>              break;
>  
> +        /* SAF-4-safe No persistent side effects */
>          TRACE_3D(TRC_SCHED_SHUTDOWN,
>                   current->domain->domain_id, current->vcpu_id,
>                   sched_shutdown.reason);
> @@ -1912,6 +1914,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( copy_from_guest(&sched_shutdown, arg, 1) )
>              break;
>  
> +        /* SAF-4-safe No persistent side effects */
>          TRACE_3D(TRC_SCHED_SHUTDOWN_CODE,
>                   d->domain_id, current->vcpu_id, sched_shutdown.reason);
>  

In at least the former two of these cases pulling out "current" into a local
variable "curr" would likely eliminate the violation and at the same time
improve code a little.

Jan

Re: [XEN PATCH v2 2/3] xen/arm: add SAF deviation for debugging and logging effects

[Simone Ballarin]

On 27/11/23 11:46, Jan Beulich wrote:
> On 24.11.2023 18:29, Simone Ballarin wrote:
>> Rule 13.1: Initializer lists shall not contain persistent side effects
>>
>> Effects caused by debug/logging macros and functions (like ASSERT, __bad_atomic_size,
>> LOG, etc ...) that crash execution or produce logs are not dangerous in initializer
>> lists. The evaluation order in abnormal conditions is not relevant. Evaluation order
>> of logging effects is always safe.
>>
>> This patch deviates violations using SAF commits caused by debug/logging macros and
>> functions.
>>
>> Asm volatile statements in initializer lists that do not perform any persistent side
>> effect are safe: this patch deviates violations caused by uses of the current macro
>> (that contains an asm volatile) in initializer lists.
>>
>> No functional changes.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>
>> ---
>> Changes in v2:
>> New patch based on the discussion for "xen/arm: address violations of MISRA C:2012 Rule 13.1".
>> ---
>>   docs/misra/safe.json     | 16 ++++++++++++++++
>>   xen/arch/arm/device.c    |  1 +
>>   xen/arch/arm/guestcopy.c |  4 ++++
>>   xen/arch/x86/hvm/hvm.c   |  1 +
>>   xen/common/sched/core.c  |  3 +++
> 
> The latter two don't really fit the title prefix.
> 
>> --- a/docs/misra/safe.json
>> +++ b/docs/misra/safe.json
>> @@ -28,6 +28,22 @@
>>           },
>>           {
>>               "id": "SAF-3-safe",
>> +            "analyser": {
>> +                "eclair": "MC3R1.R13.1"
>> +            },
>> +            "name": "MC3R1.R13.1: effects for debugging and logging",
>> +            "text": "Effects for debugging and loggings reasons that crash execution or produce logs are allowed in initializer lists. The evaluation order in abnormal conditions is not relevant."
>> +        },
> 
> I'm wary of this statement. Order may not matter much anymore _after_ an
> abnormal condition was encountered, but in the course of determining whether
> an abnormal condition might have been reached it may very well still matter.
> 

Do you object to the deviation in general? Or just to the wording?

>> +        {
>> +            "id": "SAF-4-safe",
>> +            "analyser": {
>> +                "eclair": "MC3R1.R13.1"
>> +            },
>> +            "name": "MC3R1.R13.1: volatile asm statements that do not perform any persistent side effect",
>> +            "text": "Volatile asm statements in an initializer list if do not perform persistent side effects are safe."
> 
> Since each respective comment ought to affect just a single asm(), I think
> this wants writing in singular. I further don't think it is useful for
> "text" to largely restate what "name" already says.
> 

Ok.

>> --- a/xen/arch/arm/device.c
>> +++ b/xen/arch/arm/device.c
>> @@ -331,6 +331,7 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
>>           .p2mt = p2mt,
>>           .skip_mapping = !own_device ||
>>                           (is_pci_passthrough_enabled() &&
>> +                        /* SAF-3-safe effects for debugging/logging reasons are safe */
>>                           (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
> 
> What's the debugging / logging reason on the commented line?

The "ASSERT(dev != NULL)" in its body.

> 
>> --- a/xen/arch/arm/guestcopy.c
>> +++ b/xen/arch/arm/guestcopy.c
>> @@ -110,18 +110,21 @@ static unsigned long copy_guest(void *buf, uint64_t addr, unsigned int len,
>>   unsigned long raw_copy_to_guest(void *to, const void *from, unsigned int len)
>>   {
>>       return copy_guest((void *)from, (vaddr_t)to, len,
>> +                      /* SAF-4-safe No persistent side effects */
>>                         GVA_INFO(current), COPY_to_guest | COPY_linear);
>>   }
>>   
>>   unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
>>                                                unsigned int len)
>>   {
>> +    /* SAF-4-safe No persistent side effects */
>>       return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current),
>>                         COPY_to_guest | COPY_flush_dcache | COPY_linear);
>>   }
>>   
>>   unsigned long raw_clear_guest(void *to, unsigned int len)
>>   {
>> +    /* SAF-4-safe No persistent side effects */
>>       return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current),
>>                         COPY_to_guest | COPY_linear);
>>   }
>> @@ -129,6 +132,7 @@ unsigned long raw_clear_guest(void *to, unsigned int len)
>>   unsigned long raw_copy_from_guest(void *to, const void __user *from,
>>                                     unsigned int len)
>>   {
>> +    /* SAF-4-safe No persistent side effects */
>>       return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current),
>>                         COPY_from_guest | COPY_linear);
>>   }
> 
> I can only guess that in all four of these it's the use of "current" which
> requires the comment. Yet imo that either needs making explicit, or such a
> comment shouldn't go on use sites of "current", but on its definition site.
> 

"current" does not contain any violation of R13.1. Its expansion
produces a side-effect, but this is not a problem in itself. The real
problem is that GVA_INFO expands it in an initializer list:
#define GVA_INFO(vcpu) ((copy_info_t) { .gva = { vcpu } })

GVA_INFO(current) is the real piece of code that requires to be
deviated.

>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -1517,6 +1517,7 @@ long vcpu_yield(void)
>>   
>>       SCHED_STAT_CRANK(vcpu_yield);
>>   
>> +    /* SAF-4-safe No persistent side effects */
>>       TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
>>       raise_softirq(SCHEDULE_SOFTIRQ);
>>       return 0;
>> @@ -1895,6 +1896,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>           if ( copy_from_guest(&sched_shutdown, arg, 1) )
>>               break;
>>   
>> +        /* SAF-4-safe No persistent side effects */
>>           TRACE_3D(TRC_SCHED_SHUTDOWN,
>>                    current->domain->domain_id, current->vcpu_id,
>>                    sched_shutdown.reason);
>> @@ -1912,6 +1914,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>           if ( copy_from_guest(&sched_shutdown, arg, 1) )
>>               break;
>>   
>> +        /* SAF-4-safe No persistent side effects */
>>           TRACE_3D(TRC_SCHED_SHUTDOWN_CODE,
>>                    d->domain_id, current->vcpu_id, sched_shutdown.reason);
>>   
> 
> In at least the former two of these cases pulling out "current" into a local
> variable "curr" would likely eliminate the violation and at the same time
> improve code a little.
> 

Yes, I agree.

> Jan
> 

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH v2 2/3] xen/arm: add SAF deviation for debugging and logging effects

[Jan Beulich]

On 27.11.2023 15:35, Simone Ballarin wrote:
> On 27/11/23 11:46, Jan Beulich wrote:
>> On 24.11.2023 18:29, Simone Ballarin wrote:
>>> --- a/docs/misra/safe.json
>>> +++ b/docs/misra/safe.json
>>> @@ -28,6 +28,22 @@
>>>           },
>>>           {
>>>               "id": "SAF-3-safe",
>>> +            "analyser": {
>>> +                "eclair": "MC3R1.R13.1"
>>> +            },
>>> +            "name": "MC3R1.R13.1: effects for debugging and logging",
>>> +            "text": "Effects for debugging and loggings reasons that crash execution or produce logs are allowed in initializer lists. The evaluation order in abnormal conditions is not relevant."
>>> +        },
>>
>> I'm wary of this statement. Order may not matter much anymore _after_ an
>> abnormal condition was encountered, but in the course of determining whether
>> an abnormal condition might have been reached it may very well still matter.
> 
> Do you object to the deviation in general? Or just to the wording?

Primarily the wording. Yet the need to adjust the wording also hints at there
needing to be care when actually making use of this deviation. Which it turn
I'm not convinced is in the spirit of Misra.

>>> --- a/xen/arch/arm/device.c
>>> +++ b/xen/arch/arm/device.c
>>> @@ -331,6 +331,7 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
>>>           .p2mt = p2mt,
>>>           .skip_mapping = !own_device ||
>>>                           (is_pci_passthrough_enabled() &&
>>> +                        /* SAF-3-safe effects for debugging/logging reasons are safe */
>>>                           (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
>>
>> What's the debugging / logging reason on the commented line?
> 
> The "ASSERT(dev != NULL)" in its body.

"it" == device_get_class() I assume? Imo to unobvious to deal with like this.

>>> --- a/xen/arch/arm/guestcopy.c
>>> +++ b/xen/arch/arm/guestcopy.c
>>> @@ -110,18 +110,21 @@ static unsigned long copy_guest(void *buf, uint64_t addr, unsigned int len,
>>>   unsigned long raw_copy_to_guest(void *to, const void *from, unsigned int len)
>>>   {
>>>       return copy_guest((void *)from, (vaddr_t)to, len,
>>> +                      /* SAF-4-safe No persistent side effects */
>>>                         GVA_INFO(current), COPY_to_guest | COPY_linear);
>>>   }
>>>   
>>>   unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
>>>                                                unsigned int len)
>>>   {
>>> +    /* SAF-4-safe No persistent side effects */
>>>       return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current),
>>>                         COPY_to_guest | COPY_flush_dcache | COPY_linear);
>>>   }
>>>   
>>>   unsigned long raw_clear_guest(void *to, unsigned int len)
>>>   {
>>> +    /* SAF-4-safe No persistent side effects */
>>>       return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current),
>>>                         COPY_to_guest | COPY_linear);
>>>   }
>>> @@ -129,6 +132,7 @@ unsigned long raw_clear_guest(void *to, unsigned int len)
>>>   unsigned long raw_copy_from_guest(void *to, const void __user *from,
>>>                                     unsigned int len)
>>>   {
>>> +    /* SAF-4-safe No persistent side effects */
>>>       return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current),
>>>                         COPY_from_guest | COPY_linear);
>>>   }
>>
>> I can only guess that in all four of these it's the use of "current" which
>> requires the comment. Yet imo that either needs making explicit, or such a
>> comment shouldn't go on use sites of "current", but on its definition site.
>>
> 
> "current" does not contain any violation of R13.1. Its expansion
> produces a side-effect, but this is not a problem in itself. The real
> problem is that GVA_INFO expands it in an initializer list:
> #define GVA_INFO(vcpu) ((copy_info_t) { .gva = { vcpu } })

But an initializer list doesn't itself constitute a side effect, does it?

> GVA_INFO(current) is the real piece of code that requires to be
> deviated.

In which case I think this such want spelling

    return copy_guest(to, (vaddr_t)from, len,
                      /* SAF-4-safe No persistent side effects */
                      GVA_INFO(current),
                      COPY_from_guest | COPY_linear);

or some such. Not the least because we want to make sure a deviation does
not have wider than necessary scope (which when formatted as above is,
aiui, always the following source line).

Jan

Re: [XEN PATCH v2 2/3] xen/arm: add SAF deviation for debugging and logging effects

[Simone Ballarin]

On 27/11/23 16:09, Jan Beulich wrote:
> On 27.11.2023 15:35, Simone Ballarin wrote:
>> On 27/11/23 11:46, Jan Beulich wrote:
>>> On 24.11.2023 18:29, Simone Ballarin wrote:
>>>> --- a/docs/misra/safe.json
>>>> +++ b/docs/misra/safe.json
>>>> @@ -28,6 +28,22 @@
>>>>            },
>>>>            {
>>>>                "id": "SAF-3-safe",
>>>> +            "analyser": {
>>>> +                "eclair": "MC3R1.R13.1"
>>>> +            },
>>>> +            "name": "MC3R1.R13.1: effects for debugging and logging",
>>>> +            "text": "Effects for debugging and loggings reasons that crash execution or produce logs are allowed in initializer lists. The evaluation order in abnormal conditions is not relevant."
>>>> +        },
>>>
>>> I'm wary of this statement. Order may not matter much anymore _after_ an
>>> abnormal condition was encountered, but in the course of determining whether
>>> an abnormal condition might have been reached it may very well still matter.
>>
>> Do you object to the deviation in general? Or just to the wording?
> 
> Primarily the wording. Yet the need to adjust the wording also hints at there
> needing to be care when actually making use of this deviation. Which it turn
> I'm not convinced is in the spirit of Misra

The rule is really strict, but imho the only real dangerous situation is
when an entry performs a persistent side effect that can change the
behavior of another entry. I.e.:

int y = 0;
int x[2] =
{
   y=1, /* first element will be always 1 */
   y    /* second element can be either 0 or 1 */
};

Above we have a dependency between the first entry and the second.

Now let's consider logging effects:

#define LOG(x) printf("LOG: %s", x);

int x[2] =
{
   ({ LOG("1"); 1; }),
   ({ LOG("2"); 2; })
};


Here the execution can print:
"LOG: 1LOG: 2" or
"LOG: 2LOG: 1".

Do we agree that the evaluation order of prints caused by logging
functions/macros do not normally cause dependencies between the
entries? The execution is still non-deterministic, but does it really
matter?.

In the case of function that crash execution, no dependencies can exist
since no further entries will be evaluated.

In conclusion, I propose the following rewording:

"text": "Effects that crash execution or produce logs are allowed in 
initializer lists. Logging effects do not affect the evaluation of 
subsequent entries. Crash effects are allowed as they represent the
end of the execution.


> 
>>>> --- a/xen/arch/arm/device.c
>>>> +++ b/xen/arch/arm/device.c
>>>> @@ -331,6 +331,7 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
>>>>            .p2mt = p2mt,
>>>>            .skip_mapping = !own_device ||
>>>>                            (is_pci_passthrough_enabled() &&
>>>> +                        /* SAF-3-safe effects for debugging/logging reasons are safe */
>>>>                            (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
>>>
>>> What's the debugging / logging reason on the commented line?
>>
>> The "ASSERT(dev != NULL)" in its body.
> 
> "it" == device_get_class() I assume? Imo to unobvious to deal with like this.
> 

Yes, Julien suggested to simply remove the ASSERT as it isn't really 
useful. I forgot his suggestion. In v3 I will remove it as suggested.

>>>> --- a/xen/arch/arm/guestcopy.c
>>>> +++ b/xen/arch/arm/guestcopy.c
>>>> @@ -110,18 +110,21 @@ static unsigned long copy_guest(void *buf, uint64_t addr, unsigned int len,
>>>>    unsigned long raw_copy_to_guest(void *to, const void *from, unsigned int len)
>>>>    {
>>>>        return copy_guest((void *)from, (vaddr_t)to, len,
>>>> +                      /* SAF-4-safe No persistent side effects */
>>>>                          GVA_INFO(current), COPY_to_guest | COPY_linear);
>>>>    }
>>>>    
>>>>    unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
>>>>                                                 unsigned int len)
>>>>    {
>>>> +    /* SAF-4-safe No persistent side effects */
>>>>        return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current),
>>>>                          COPY_to_guest | COPY_flush_dcache | COPY_linear);
>>>>    }
>>>>    
>>>>    unsigned long raw_clear_guest(void *to, unsigned int len)
>>>>    {
>>>> +    /* SAF-4-safe No persistent side effects */
>>>>        return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current),
>>>>                          COPY_to_guest | COPY_linear);
>>>>    }
>>>> @@ -129,6 +132,7 @@ unsigned long raw_clear_guest(void *to, unsigned int len)
>>>>    unsigned long raw_copy_from_guest(void *to, const void __user *from,
>>>>                                      unsigned int len)
>>>>    {
>>>> +    /* SAF-4-safe No persistent side effects */
>>>>        return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current),
>>>>                          COPY_from_guest | COPY_linear);
>>>>    }
>>>
>>> I can only guess that in all four of these it's the use of "current" which
>>> requires the comment. Yet imo that either needs making explicit, or such a
>>> comment shouldn't go on use sites of "current", but on its definition site.
>>>
>>
>> "current" does not contain any violation of R13.1. Its expansion
>> produces a side-effect, but this is not a problem in itself. The real
>> problem is that GVA_INFO expands it in an initializer list:
>> #define GVA_INFO(vcpu) ((copy_info_t) { .gva = { vcpu } })
> 
> But an initializer list doesn't itself constitute a side effect, does it?
> 

The side effect should be inside the initializer list. { .gva = 1 } is 
not a violation.

>> GVA_INFO(current) is the real piece of code that requires to be
>> deviated.
> 
> In which case I think this such want spelling
> 
>      return copy_guest(to, (vaddr_t)from, len,
>                        /* SAF-4-safe No persistent side effects */
>                        GVA_INFO(current),
>                        COPY_from_guest | COPY_linear);
> 
> or some such. Not the least because we want to make sure a deviation does
> not have wider than necessary scope (which when formatted as above is,
> aiui, always the following source line).
> 

Yes, I agree.

> Jan

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH v2 2/3] xen/arm: add SAF deviation for debugging and logging effects

[Jan Beulich]

On 27.11.2023 18:34, Simone Ballarin wrote:
> On 27/11/23 16:09, Jan Beulich wrote:
>> On 27.11.2023 15:35, Simone Ballarin wrote:
>>> On 27/11/23 11:46, Jan Beulich wrote:
>>>> On 24.11.2023 18:29, Simone Ballarin wrote:
>>>>> --- a/docs/misra/safe.json
>>>>> +++ b/docs/misra/safe.json
>>>>> @@ -28,6 +28,22 @@
>>>>>            },
>>>>>            {
>>>>>                "id": "SAF-3-safe",
>>>>> +            "analyser": {
>>>>> +                "eclair": "MC3R1.R13.1"
>>>>> +            },
>>>>> +            "name": "MC3R1.R13.1: effects for debugging and logging",
>>>>> +            "text": "Effects for debugging and loggings reasons that crash execution or produce logs are allowed in initializer lists. The evaluation order in abnormal conditions is not relevant."
>>>>> +        },
>>>>
>>>> I'm wary of this statement. Order may not matter much anymore _after_ an
>>>> abnormal condition was encountered, but in the course of determining whether
>>>> an abnormal condition might have been reached it may very well still matter.
>>>
>>> Do you object to the deviation in general? Or just to the wording?
>>
>> Primarily the wording. Yet the need to adjust the wording also hints at there
>> needing to be care when actually making use of this deviation. Which it turn
>> I'm not convinced is in the spirit of Misra
> 
> The rule is really strict, but imho the only real dangerous situation is
> when an entry performs a persistent side effect that can change the
> behavior of another entry. I.e.:
> 
> int y = 0;
> int x[2] =
> {
>    y=1, /* first element will be always 1 */
>    y    /* second element can be either 0 or 1 */
> };
> 
> Above we have a dependency between the first entry and the second.
> 
> Now let's consider logging effects:
> 
> #define LOG(x) printf("LOG: %s", x);
> 
> int x[2] =
> {
>    ({ LOG("1"); 1; }),
>    ({ LOG("2"); 2; })
> };
> 
> 
> Here the execution can print:
> "LOG: 1LOG: 2" or
> "LOG: 2LOG: 1".
> 
> Do we agree that the evaluation order of prints caused by logging
> functions/macros do not normally cause dependencies between the
> entries? The execution is still non-deterministic, but does it really
> matter?.
> 
> In the case of function that crash execution, no dependencies can exist
> since no further entries will be evaluated.
> 
> In conclusion, I propose the following rewording:
> 
> "text": "Effects that crash execution or produce logs are allowed in 
> initializer lists. Logging effects do not affect the evaluation of 
> subsequent entries. Crash effects are allowed as they represent the
> end of the execution.

Let's assume we have a BUG_ON() (as the "crash effect") the condition of
which depends on where in the sequence of operations it actually executes,
i.e. (potentially) dependent upon another part of the initializer. I hope
we agree that's not something that should be deviated? Yet even the re-
worded statement would - according to my reading - permit doing so.

I guess I should try to remember to bring this up on this afternoon's call.

>>>>> --- a/xen/arch/arm/guestcopy.c
>>>>> +++ b/xen/arch/arm/guestcopy.c
>>>>> @@ -110,18 +110,21 @@ static unsigned long copy_guest(void *buf, uint64_t addr, unsigned int len,
>>>>>    unsigned long raw_copy_to_guest(void *to, const void *from, unsigned int len)
>>>>>    {
>>>>>        return copy_guest((void *)from, (vaddr_t)to, len,
>>>>> +                      /* SAF-4-safe No persistent side effects */
>>>>>                          GVA_INFO(current), COPY_to_guest | COPY_linear);
>>>>>    }
>>>>>    
>>>>>    unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
>>>>>                                                 unsigned int len)
>>>>>    {
>>>>> +    /* SAF-4-safe No persistent side effects */
>>>>>        return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current),
>>>>>                          COPY_to_guest | COPY_flush_dcache | COPY_linear);
>>>>>    }
>>>>>    
>>>>>    unsigned long raw_clear_guest(void *to, unsigned int len)
>>>>>    {
>>>>> +    /* SAF-4-safe No persistent side effects */
>>>>>        return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current),
>>>>>                          COPY_to_guest | COPY_linear);
>>>>>    }
>>>>> @@ -129,6 +132,7 @@ unsigned long raw_clear_guest(void *to, unsigned int len)
>>>>>    unsigned long raw_copy_from_guest(void *to, const void __user *from,
>>>>>                                      unsigned int len)
>>>>>    {
>>>>> +    /* SAF-4-safe No persistent side effects */
>>>>>        return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current),
>>>>>                          COPY_from_guest | COPY_linear);
>>>>>    }
>>>>
>>>> I can only guess that in all four of these it's the use of "current" which
>>>> requires the comment. Yet imo that either needs making explicit, or such a
>>>> comment shouldn't go on use sites of "current", but on its definition site.
>>>>
>>>
>>> "current" does not contain any violation of R13.1. Its expansion
>>> produces a side-effect, but this is not a problem in itself. The real
>>> problem is that GVA_INFO expands it in an initializer list:
>>> #define GVA_INFO(vcpu) ((copy_info_t) { .gva = { vcpu } })
>>
>> But an initializer list doesn't itself constitute a side effect, does it?
> 
> The side effect should be inside the initializer list. { .gva = 1 } is 
> not a violation.

I'm afraid I don't see what would be constituting a violation here.

Jan

Re: [XEN PATCH v2 2/3] xen/arm: add SAF deviation for debugging and logging effects

[Simone Ballarin]

On 28/11/23 09:42, Jan Beulich wrote:
> On 27.11.2023 18:34, Simone Ballarin wrote:
>> On 27/11/23 16:09, Jan Beulich wrote:
>>> On 27.11.2023 15:35, Simone Ballarin wrote:
>>>> On 27/11/23 11:46, Jan Beulich wrote:
>>>>> On 24.11.2023 18:29, Simone Ballarin wrote:
>>>>>> --- a/docs/misra/safe.json
>>>>>> +++ b/docs/misra/safe.json
>>>>>> @@ -28,6 +28,22 @@
>>>>>>             },
>>>>>>             {
>>>>>>                 "id": "SAF-3-safe",
>>>>>> +            "analyser": {
>>>>>> +                "eclair": "MC3R1.R13.1"
>>>>>> +            },
>>>>>> +            "name": "MC3R1.R13.1: effects for debugging and logging",
>>>>>> +            "text": "Effects for debugging and loggings reasons that crash execution or produce logs are allowed in initializer lists. The evaluation order in abnormal conditions is not relevant."
>>>>>> +        },
>>>>>
>>>>> I'm wary of this statement. Order may not matter much anymore _after_ an
>>>>> abnormal condition was encountered, but in the course of determining whether
>>>>> an abnormal condition might have been reached it may very well still matter.
>>>>
>>>> Do you object to the deviation in general? Or just to the wording?
>>>
>>> Primarily the wording. Yet the need to adjust the wording also hints at there
>>> needing to be care when actually making use of this deviation. Which it turn
>>> I'm not convinced is in the spirit of Misra
>>
>> The rule is really strict, but imho the only real dangerous situation is
>> when an entry performs a persistent side effect that can change the
>> behavior of another entry. I.e.:
>>
>> int y = 0;
>> int x[2] =
>> {
>>     y=1, /* first element will be always 1 */
>>     y    /* second element can be either 0 or 1 */
>> };
>>
>> Above we have a dependency between the first entry and the second.
>>
>> Now let's consider logging effects:
>>
>> #define LOG(x) printf("LOG: %s", x);
>>
>> int x[2] =
>> {
>>     ({ LOG("1"); 1; }),
>>     ({ LOG("2"); 2; })
>> };
>>
>>
>> Here the execution can print:
>> "LOG: 1LOG: 2" or
>> "LOG: 2LOG: 1".
>>
>> Do we agree that the evaluation order of prints caused by logging
>> functions/macros do not normally cause dependencies between the
>> entries? The execution is still non-deterministic, but does it really
>> matter?.
>>
>> In the case of function that crash execution, no dependencies can exist
>> since no further entries will be evaluated.
>>
>> In conclusion, I propose the following rewording:
>>
>> "text": "Effects that crash execution or produce logs are allowed in
>> initializer lists. Logging effects do not affect the evaluation of
>> subsequent entries. Crash effects are allowed as they represent the
>> end of the execution.
> 
> Let's assume we have a BUG_ON() (as the "crash effect") the condition of
> which depends on where in the sequence of operations it actually executes,
> i.e. (potentially) dependent upon another part of the initializer. I hope
> we agree that's not something that should be deviated? Yet even the re-
> worded statement would - according to my reading - permit doing so.
> 
> I guess I should try to remember to bring this up on this afternoon's call.

I known you had a conversation about that during the call, but from what
I known, a clear decision for crash effects wasn't taken.

If the community does not want SAF deviations for such cases, we should 
consider moving them to external variables.

> 
>>>>>> --- a/xen/arch/arm/guestcopy.c
>>>>>> +++ b/xen/arch/arm/guestcopy.c
>>>>>> @@ -110,18 +110,21 @@ static unsigned long copy_guest(void *buf, uint64_t addr, unsigned int len,
>>>>>>     unsigned long raw_copy_to_guest(void *to, const void *from, unsigned int len)
>>>>>>     {
>>>>>>         return copy_guest((void *)from, (vaddr_t)to, len,
>>>>>> +                      /* SAF-4-safe No persistent side effects */
>>>>>>                           GVA_INFO(current), COPY_to_guest | COPY_linear);
>>>>>>     }
>>>>>>     
>>>>>>     unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
>>>>>>                                                  unsigned int len)
>>>>>>     {
>>>>>> +    /* SAF-4-safe No persistent side effects */
>>>>>>         return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current),
>>>>>>                           COPY_to_guest | COPY_flush_dcache | COPY_linear);
>>>>>>     }
>>>>>>     
>>>>>>     unsigned long raw_clear_guest(void *to, unsigned int len)
>>>>>>     {
>>>>>> +    /* SAF-4-safe No persistent side effects */
>>>>>>         return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current),
>>>>>>                           COPY_to_guest | COPY_linear);
>>>>>>     }
>>>>>> @@ -129,6 +132,7 @@ unsigned long raw_clear_guest(void *to, unsigned int len)
>>>>>>     unsigned long raw_copy_from_guest(void *to, const void __user *from,
>>>>>>                                       unsigned int len)
>>>>>>     {
>>>>>> +    /* SAF-4-safe No persistent side effects */
>>>>>>         return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current),
>>>>>>                           COPY_from_guest | COPY_linear);
>>>>>>     }
>>>>>
>>>>> I can only guess that in all four of these it's the use of "current" which
>>>>> requires the comment. Yet imo that either needs making explicit, or such a
>>>>> comment shouldn't go on use sites of "current", but on its definition site.
>>>>>
>>>>
>>>> "current" does not contain any violation of R13.1. Its expansion
>>>> produces a side-effect, but this is not a problem in itself. The real
>>>> problem is that GVA_INFO expands it in an initializer list:
>>>> #define GVA_INFO(vcpu) ((copy_info_t) { .gva = { vcpu } })
>>>
>>> But an initializer list doesn't itself constitute a side effect, does it?
>>
>> The side effect should be inside the initializer list. { .gva = 1 } is
>> not a violation.
> 
> I'm afraid I don't see what would be constituting a violation here.
> 
> Jan
> 

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

[XEN PATCH v2 3/3] xen: address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin]

Rule 13.1: Initializer lists shall not contain persistent side effects

The assignment operation in:

.irq = rc = uart->irq,

is a persistent side effect in a struct initializer list.

This patch avoids rc assignment and directly uses uart->irq
in the following if statement.

No functional changes.

Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v2:
- avoid assignment of rc;
- drop changes in vcpu_yield(void).
---
 xen/drivers/char/ns16550.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index ddf2a48be6..644a3192bb 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -445,11 +445,13 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
             struct msi_info msi = {
                 .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
                                  uart->ps_bdf[2]),
-                .irq = rc = uart->irq,
+                .irq = uart->irq,
                 .entry_nr = 1
             };
 
-            if ( rc > 0 )
+            rc = 0;
+
+            if ( uart->irq > 0 )
             {
                 struct msi_desc *msi_desc = NULL;
 
-- 
2.34.1

Re: [XEN PATCH v2 3/3] xen: address violations of MISRA C:2012 Rule 13.1

[Jan Beulich]

On 24.11.2023 18:29, Simone Ballarin wrote:
> Rule 13.1: Initializer lists shall not contain persistent side effects
> 
> The assignment operation in:
> 
> .irq = rc = uart->irq,
> 
> is a persistent side effect in a struct initializer list.
> 
> This patch avoids rc assignment and directly uses uart->irq
> in the following if statement.
> 
> No functional changes.
> 
> Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

Who's the author of this patch? (Either the order of the SoB is wrong, or
there's a From: tag missing.)

> ---
> Changes in v2:
> - avoid assignment of rc;
> - drop changes in vcpu_yield(void).
> ---
>  xen/drivers/char/ns16550.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

This warrants a more specific subject prefix. Also there's only a single
violation being dealt with here.

> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -445,11 +445,13 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
>              struct msi_info msi = {
>                  .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>                                   uart->ps_bdf[2]),
> -                .irq = rc = uart->irq,
> +                .irq = uart->irq,
>                  .entry_nr = 1
>              };
>  
> -            if ( rc > 0 )
> +            rc = 0;
> +
> +            if ( uart->irq > 0 )
>              {
>                  struct msi_desc *msi_desc = NULL;

The fact that there's no functional change here isn't really obvious.
Imo you want to prove that to a reasonable degree in the description.

Jan

Re: [XEN PATCH v2 3/3] xen: address violations of MISRA C:2012 Rule 13.1

[Stefano Stabellini]

On Mon, 27 Nov 2023, Jan Beulich wrote:
> On 24.11.2023 18:29, Simone Ballarin wrote:
> > Rule 13.1: Initializer lists shall not contain persistent side effects
> > 
> > The assignment operation in:
> > 
> > .irq = rc = uart->irq,
> > 
> > is a persistent side effect in a struct initializer list.
> > 
> > This patch avoids rc assignment and directly uses uart->irq
> > in the following if statement.
> > 
> > No functional changes.
> > 
> > Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
> > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> Who's the author of this patch? (Either the order of the SoB is wrong, or
> there's a From: tag missing.)
> 
> > ---
> > Changes in v2:
> > - avoid assignment of rc;
> > - drop changes in vcpu_yield(void).
> > ---
> >  xen/drivers/char/ns16550.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> This warrants a more specific subject prefix. Also there's only a single
> violation being dealt with here.
> 
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -445,11 +445,13 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
> >              struct msi_info msi = {
> >                  .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
> >                                   uart->ps_bdf[2]),
> > -                .irq = rc = uart->irq,
> > +                .irq = uart->irq,
> >                  .entry_nr = 1
> >              };
> >  
> > -            if ( rc > 0 )
> > +            rc = 0;
> > +
> > +            if ( uart->irq > 0 )
> >              {
> >                  struct msi_desc *msi_desc = NULL;
> 
> The fact that there's no functional change here isn't really obvious.
> Imo you want to prove that to a reasonable degree in the description.
 
Agreed. Only reading this chunk, wouldn't it be better to do:

    };

    rc = uart->irq;

    if ( rc > 0 )

at least it would be obvious?

Re: [XEN PATCH v2 3/3] xen: address violations of MISRA C:2012 Rule 13.1

[Jan Beulich]

On 02.12.2023 04:22, Stefano Stabellini wrote:
> On Mon, 27 Nov 2023, Jan Beulich wrote:
>> On 24.11.2023 18:29, Simone Ballarin wrote:
>>> Rule 13.1: Initializer lists shall not contain persistent side effects
>>>
>>> The assignment operation in:
>>>
>>> .irq = rc = uart->irq,
>>>
>>> is a persistent side effect in a struct initializer list.
>>>
>>> This patch avoids rc assignment and directly uses uart->irq
>>> in the following if statement.
>>>
>>> No functional changes.
>>>
>>> Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>
>> Who's the author of this patch? (Either the order of the SoB is wrong, or
>> there's a From: tag missing.)
>>
>>> ---
>>> Changes in v2:
>>> - avoid assignment of rc;
>>> - drop changes in vcpu_yield(void).
>>> ---
>>>  xen/drivers/char/ns16550.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> This warrants a more specific subject prefix. Also there's only a single
>> violation being dealt with here.
>>
>>> --- a/xen/drivers/char/ns16550.c
>>> +++ b/xen/drivers/char/ns16550.c
>>> @@ -445,11 +445,13 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
>>>              struct msi_info msi = {
>>>                  .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>>>                                   uart->ps_bdf[2]),
>>> -                .irq = rc = uart->irq,
>>> +                .irq = uart->irq,
>>>                  .entry_nr = 1
>>>              };
>>>  
>>> -            if ( rc > 0 )
>>> +            rc = 0;
>>> +
>>> +            if ( uart->irq > 0 )
>>>              {
>>>                  struct msi_desc *msi_desc = NULL;
>>
>> The fact that there's no functional change here isn't really obvious.
>> Imo you want to prove that to a reasonable degree in the description.
>  
> Agreed. Only reading this chunk, wouldn't it be better to do:
> 
>     };
> 
>     rc = uart->irq;
> 
>     if ( rc > 0 )
> 
> at least it would be obvious?

Indeed.

Jan