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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxxx>
> 
> ---
> 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