[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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)




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.