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

[Simone Ballarin <simone.ballarin@xxxxxxxxxxx>]

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@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.
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)