Re: [XEN PATCH v4 1/4] xen: add SAF deviation for debugging and logging effects

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.02.2024 16:16, 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.

I thought I said so before: When talking of just logging, evaluation order
may very well have a impact on correctness. Therefore we shouldn't mix
debugging and logging.

> Function hvm_get_guest_tsc_fixed (indirectly) performs different side effects.
> For example it calls hvm_get_guest_time_fixed that contains an ASSERT and 
> calls
> to spin_lock and spin_unlock.
> 
> These side effects are not dangerous: they can be executed regardless of the
> initializer list evaluation order
> 
> This patch deviates violations using SAF commits caused by debug/logging 
> macros and
> functions.

DYM "comments"?

> --- 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)),

Taking this just as example: I think the comment is too long. Just
saying (leaving aside my comment higher up) "debugging/logging"
would imo be sufficient.

> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -110,26 +110,34 @@ 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,
> -                      GVA_INFO(current), COPY_to_guest | COPY_linear);
> +                      /* SAF-4-safe No persistent side effects */
> +                      GVA_INFO(current),

I _still_ think this leaves ambiguity. The more that you need to look
up GVA_INFO() to recognize what this is about.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -800,6 +800,7 @@ static int cf_check hvm_save_cpu_ctxt(struct vcpu *v, 
> hvm_domain_context_t *h)
>  {
>      struct segment_register seg;
>      struct hvm_hw_cpu ctxt = {
> +        /* SAF-3-safe effects for debugging/logging reasons are safe */
>          .tsc = hvm_get_guest_tsc_fixed(v, v->domain->arch.hvm.sync_tsc),

A prereq for this imo is that the function take const struct vcpu *.
But I'm not sure that'll suffice. The function can change at any time,
rendering the comment here stale perhaps without anyone noticing.

> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -1521,6 +1521,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;
> @@ -1899,6 +1900,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);
> @@ -1916,6 +1918,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);
>  

For all of these iirc the suggestion was to latch current into a local
variable (named "curr" by convention).

Jan