[XEN PATCH 0/4][for-4.19] xen: address violations of Rule 13.1

[XEN PATCH 0/4][for-4.19] xen: address violations of 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

Some expressions with side-effects have been moved outside the
initializer lists, others have been deviated.

Function calls do not necessarily have side-effects, in these cases this
path propose to add GCC pure or const attributes whenever possible.

Function attributes pure and const do not need to be added as GCC
attributes, they can be added using ECLAIR configurations.

Simone Ballarin (4):
  xen/arm: address violations of MISRA C:2012 Rule 13.1
  automation/eclair: add deviations and call properties.
  xen/include: add pure and const attributes
  xen: address violations of MISRA C:2012 Rule 13.1

 .../eclair_analysis/ECLAIR/call_properties.ecl   | 10 ++++++++++
 automation/eclair_analysis/ECLAIR/deviations.ecl | 13 +++++++++++++
 docs/misra/deviations.rst                        | 11 +++++++++++
 xen/arch/arm/device.c                            |  6 +++---
 xen/arch/arm/guestcopy.c                         | 12 ++++++++----
 xen/arch/arm/include/asm/current.h               |  2 +-
 xen/common/sched/core.c                          | 16 ++++++++++++----
 xen/drivers/char/ns16550.c                       |  4 +++-
 xen/include/xen/pdx.h                            |  2 +-
 xen/include/xen/typesafe.h                       |  4 ++--
 10 files changed, 64 insertions(+), 16 deletions(-)

-- 
2.34.1

[XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin]

Rule 13.1: Initializer lists shall not contain persistent side effects

This patch moves expressions with side-effects into new variables before
the initializer lists.

Function calls do not necessarily have side-effects, in these cases the
GCC pure or const attributes are added when possible.

No functional changes.

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

---
Function attributes pure and const do not need to be added as GCC
attributes, they can be added using ECLAIR configurations.
---
 xen/arch/arm/device.c              |  6 +++---
 xen/arch/arm/guestcopy.c           | 12 ++++++++----
 xen/arch/arm/include/asm/current.h |  2 +-
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 1f631d3274..e9be078415 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -319,6 +319,8 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
     int res;
     paddr_t addr, size;
     bool own_device = !dt_device_for_passthrough(dev);
+    bool dev_is_hostbridge = is_pci_passthrough_enabled() &&
+                             device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE;
     /*
      * We want to avoid mapping the MMIO in dom0 for the following cases:
      *   - The device is owned by dom0 (i.e. it has been flagged for
@@ -329,9 +331,7 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
     struct map_range_data mr_data = {
         .d = d,
         .p2mt = p2mt,
-        .skip_mapping = !own_device ||
-                        (is_pci_passthrough_enabled() &&
-                        (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
+        .skip_mapping = !own_device || dev_is_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..3ec6743bf6 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -109,27 +109,31 @@ 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)
 {
+    struct vcpu *current_vcpu = current;
     return copy_guest((void *)from, (vaddr_t)to, len,
-                      GVA_INFO(current), COPY_to_guest | COPY_linear);
+                      GVA_INFO(current_vcpu), COPY_to_guest | COPY_linear);
 }
 
 unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
                                              unsigned int len)
 {
-    return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current),
+    struct vcpu *current_vcpu = current;
+    return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current_vcpu),
                       COPY_to_guest | COPY_flush_dcache | COPY_linear);
 }
 
 unsigned long raw_clear_guest(void *to, unsigned int len)
 {
-    return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current),
+    struct vcpu *current_vcpu = current;
+    return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current_vcpu),
                       COPY_to_guest | COPY_linear);
 }
 
 unsigned long raw_copy_from_guest(void *to, const void __user *from,
                                   unsigned int len)
 {
-    return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current),
+    struct vcpu *current_vcpu = current;
+    return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current_vcpu),
                       COPY_from_guest | COPY_linear);
 }
 
diff --git a/xen/arch/arm/include/asm/current.h b/xen/arch/arm/include/asm/current.h
index 6973eeb1d1..a66e28fefb 100644
--- a/xen/arch/arm/include/asm/current.h
+++ b/xen/arch/arm/include/asm/current.h
@@ -28,7 +28,7 @@ struct cpu_info {
     uint32_t flags;
 };
 
-static inline struct cpu_info *get_cpu_info(void)
+static inline __attribute_pure__ struct cpu_info *get_cpu_info(void)
 {
 #ifdef __clang__
     unsigned long sp;
-- 
2.34.1

[XEN PATCH 2/4] automation/eclair: add deviations and call properties.

[Simone Ballarin]

Deviate violations of MISRA C:2012 Rule 13.1 caused by
functions vcpu_runnable and __bad_atomic_size. These functions
contain side-effects such as debugging logs, assertions and
failures that cannot cause unexpected behaviors.

Add "noeffect" call property to functions read_u.*_atomic and
get_cycles.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 .../eclair_analysis/ECLAIR/call_properties.ecl      | 10 ++++++++++
 automation/eclair_analysis/ECLAIR/deviations.ecl    | 13 +++++++++++++
 docs/misra/deviations.rst                           | 11 +++++++++++
 3 files changed, 34 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/call_properties.ecl b/automation/eclair_analysis/ECLAIR/call_properties.ecl
index 3f7794bf8b..f410a6aa58 100644
--- a/automation/eclair_analysis/ECLAIR/call_properties.ecl
+++ b/automation/eclair_analysis/ECLAIR/call_properties.ecl
@@ -104,3 +104,13 @@ 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."
+-call_properties+={"^read_u(8|16|32|64|int)_atomic.*$", {"noeffect"}}
+-doc_end
+
+-doc_begin="get_cycles does not perform visible side-effects "
+-call_property+={"name(get_cycles)", {"noeffect"}}
+-doc_end
+
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index fa56e5c00a..b80ccea7bc 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -233,6 +233,19 @@ this."
 -config=MC3R1.R10.1,etypes+={safe,
   "stmt(operator(and||or||xor||not||and_assign||or_assign||xor_assign))",
   "any()"}
+#
+# Series 13.
+#
+
+-doc_begin="Function __bad_atomic_size is intended to generate a linkage error
+if invoked. Calling it in intentionally unreachable switch cases is safe even
+in a initializer list."
+-config=MC3R1.R13.1,reports+={safe, "first_area(^.*__bad_atomic_size.*$)"}
+-doc_end
+
+-doc_begin="Function vcpu_runnable contains pragmas and other side-effects for
+debugging purposes, their invocation is safe even in a initializer list."
+-config=MC3R1.R13.1,reports+={safe, "first_area(^.*vcpu_runnable.*$)"}
 -doc_end
 
 -doc_begin="See Section \"4.5 Integers\" of \"GCC_MANUAL\", where it says that
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a18925..2fcdb8da58 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -192,6 +192,17 @@ Deviations related to MISRA C:2012 Rules:
        See automation/eclair_analysis/deviations.ecl for the full explanation.
      - Tagged as `safe` for ECLAIR.
 
+   * - R13.1
+     - Function __bad_atomic_size is intended to generate a linkage error if
+       invoked. Calling it in intentionally unreachable switch cases is
+       safe even in a initializer list.
+     - Tagged as `safe` for ECLAIR.
+
+   * - R13.1
+     - Function vcpu_runnable contains pragmas and other side-effects for
+       debugging purposes, their invocation is safe even in a initializer list.
+     - Tagged as `safe` for ECLAIR.
+
    * - R13.5
      - All developers and reviewers can be safely assumed to be well aware of
        the short-circuit evaluation strategy for logical operators.
-- 
2.34.1

[XEN PATCH 3/4] xen/include: add pure and const attributes

[Simone Ballarin]

Add const and pure attributes to address reports
of MISRA C:2012 Rule 13.1: Initializer lists shall
not contain persistent side effects

Add pure attribute to function pdx_to_pfn.
Add const attribute to functions generated by TYPE_SAFE.

These functions are used in initializer lists: adding
the attributes ensures that no effect will be performed
by them.

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

---
Function attributes pure and const do not need to be added as GCC
attributes, they can be added using ECLAIR configurations.
---
 xen/include/xen/pdx.h      | 2 +-
 xen/include/xen/typesafe.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h
index f3fbc4273a..5d1050967a 100644
--- a/xen/include/xen/pdx.h
+++ b/xen/include/xen/pdx.h
@@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
  * @param pdx Page index
  * @return Obtained pfn after decompressing the pdx
  */
-static inline unsigned long pdx_to_pfn(unsigned long pdx)
+static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
 {
     return (pdx & pfn_pdx_bottom_mask) |
            ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
diff --git a/xen/include/xen/typesafe.h b/xen/include/xen/typesafe.h
index 7ecd3b4a8d..615df6f944 100644
--- a/xen/include/xen/typesafe.h
+++ b/xen/include/xen/typesafe.h
@@ -21,8 +21,8 @@
 
 #define TYPE_SAFE(_type, _name)                                         \
     typedef struct { _type _name; } _name##_t;                          \
-    static inline _name##_t _##_name(_type n) { return (_name##_t) { n }; } \
-    static inline _type _name##_x(_name##_t n) { return n._name; }
+    static inline __attribute_const__ _name##_t _##_name(_type n) { return (_name##_t) { n }; } \
+    static inline __attribute_const__ _type _name##_x(_name##_t n) { return n._name; }
 
 #else
 
-- 
2.34.1

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

[Simone Ballarin]

Rule 13.1: Initializer lists shall not contain persistent side effects

This patch moves expressions with side-effects outside the initializer lists.

No functional changes.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 xen/common/sched/core.c    | 16 ++++++++++++----
 xen/drivers/char/ns16550.c |  4 +++-
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 12deefa745..519884f989 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -1504,6 +1504,8 @@ long vcpu_yield(void)
 {
     struct vcpu * v=current;
     spinlock_t *lock;
+    domid_t domain_id;
+    int vcpu_id;
 
     rcu_read_lock(&sched_res_rculock);
 
@@ -1515,7 +1517,9 @@ long vcpu_yield(void)
 
     SCHED_STAT_CRANK(vcpu_yield);
 
-    TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
+    domain_id = current->domain->domain_id;
+    vcpu_id = current->vcpu_id;
+    TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id);
     raise_softirq(SCHEDULE_SOFTIRQ);
     return 0;
 }
@@ -1888,14 +1892,17 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case SCHEDOP_shutdown:
     {
         struct sched_shutdown sched_shutdown;
+        domid_t domain_id;
+        int vcpu_id;
 
         ret = -EFAULT;
         if ( copy_from_guest(&sched_shutdown, arg, 1) )
             break;
 
+        domain_id = current->domain->domain_id;
+        vcpu_id = current->vcpu_id;
         TRACE_3D(TRC_SCHED_SHUTDOWN,
-                 current->domain->domain_id, current->vcpu_id,
-                 sched_shutdown.reason);
+                 domain_id, vcpu_id, sched_shutdown.reason);
         ret = domain_shutdown(current->domain, (u8)sched_shutdown.reason);
 
         break;
@@ -1905,13 +1912,14 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     {
         struct sched_shutdown sched_shutdown;
         struct domain *d = current->domain;
+        int vcpu_id = current->vcpu_id;
 
         ret = -EFAULT;
         if ( copy_from_guest(&sched_shutdown, arg, 1) )
             break;
 
         TRACE_3D(TRC_SCHED_SHUTDOWN_CODE,
-                 d->domain_id, current->vcpu_id, sched_shutdown.reason);
+                 d->domain_id, vcpu_id, sched_shutdown.reason);
 
         spin_lock(&d->shutdown_lock);
         if ( d->shutdown_code == SHUTDOWN_CODE_INVALID )
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 28ddedd50d..0b3d8b2a30 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -445,10 +445,12 @@ 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
             };
 
+            rc = uart->irq;
+
             if ( rc > 0 )
             {
                 struct msi_desc *msi_desc = NULL;
-- 
2.34.1

Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Julien Grall]

Hi,

On 18/10/2023 15:18, Simone Ballarin wrote:
> Rule 13.1: Initializer lists shall not contain persistent side effects
> 
> This patch moves expressions with side-effects into new variables before
> the initializer lists.

Looking at the code, I feel the commit message should be a bit more 
verbose because they are no apparent side-effects.

> 
> Function calls do not necessarily have side-effects, in these cases the
> GCC pure or const attributes are added when possible.

You are only adding pure in this patch.

> 
> No functional changes.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> ---
> Function attributes pure and const do not need to be added as GCC
> attributes, they can be added using ECLAIR configurations.
> ---
>   xen/arch/arm/device.c              |  6 +++---
>   xen/arch/arm/guestcopy.c           | 12 ++++++++----
>   xen/arch/arm/include/asm/current.h |  2 +-
>   3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> index 1f631d3274..e9be078415 100644
> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -319,6 +319,8 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
>       int res;
>       paddr_t addr, size;
>       bool own_device = !dt_device_for_passthrough(dev);
> +    bool dev_is_hostbridge = is_pci_passthrough_enabled() &&
> +                             device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE;

The commit message suggests that the code is moved because there are 
side-effects. But none of them should have any side-effects.

In fact, if I am not mistaken, both is_pci_passthrough_enabled() and 
device_get_class() could be marked as pure.

>       /*
>        * We want to avoid mapping the MMIO in dom0 for the following cases:
>        *   - The device is owned by dom0 (i.e. it has been flagged for
> @@ -329,9 +331,7 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
>       struct map_range_data mr_data = {
>           .d = d,
>           .p2mt = p2mt,
> -        .skip_mapping = !own_device ||
> -                        (is_pci_passthrough_enabled() &&
> -                        (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
> +        .skip_mapping = !own_device || dev_is_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..3ec6743bf6 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -109,27 +109,31 @@ 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)
>   {
> +    struct vcpu *current_vcpu = current;

It is not clear to me what is the perceived side effect here and the 
others below. Can you clarify?

>       return copy_guest((void *)from, (vaddr_t)to, len,
> -                      GVA_INFO(current), COPY_to_guest | COPY_linear);
> +                      GVA_INFO(current_vcpu), COPY_to_guest | COPY_linear);
>   }
>   
>   unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
>                                                unsigned int len)
>   {
> -    return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current),
> +    struct vcpu *current_vcpu = current;
> +    return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current_vcpu),
>                         COPY_to_guest | COPY_flush_dcache | COPY_linear);
>   }
>   
>   unsigned long raw_clear_guest(void *to, unsigned int len)
>   {
> -    return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current),
> +    struct vcpu *current_vcpu = current;
> +    return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current_vcpu),
>                         COPY_to_guest | COPY_linear);
>   }
>   
>   unsigned long raw_copy_from_guest(void *to, const void __user *from,
>                                     unsigned int len)
>   {
> -    return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current),
> +    struct vcpu *current_vcpu = current;
> +    return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current_vcpu),
>                         COPY_from_guest | COPY_linear);
>   }
>   
> diff --git a/xen/arch/arm/include/asm/current.h b/xen/arch/arm/include/asm/current.h
> index 6973eeb1d1..a66e28fefb 100644
> --- a/xen/arch/arm/include/asm/current.h
> +++ b/xen/arch/arm/include/asm/current.h
> @@ -28,7 +28,7 @@ struct cpu_info {
>       uint32_t flags;
>   };
>   
> -static inline struct cpu_info *get_cpu_info(void)
> +static inline __attribute_pure__ struct cpu_info *get_cpu_info(void)
>   {
>   #ifdef __clang__
>       unsigned long sp;

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 2/4] automation/eclair: add deviations and call properties.

[Stefano Stabellini]

On Wed, 18 Oct 2023, Simone Ballarin wrote:
> Deviate violations of MISRA C:2012 Rule 13.1 caused by
> functions vcpu_runnable and __bad_atomic_size. These functions
> contain side-effects such as debugging logs, assertions and
> failures that cannot cause unexpected behaviors.
> 
> Add "noeffect" call property to functions read_u.*_atomic and
> get_cycles.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

However one comment below


> ---
>  .../eclair_analysis/ECLAIR/call_properties.ecl      | 10 ++++++++++
>  automation/eclair_analysis/ECLAIR/deviations.ecl    | 13 +++++++++++++
>  docs/misra/deviations.rst                           | 11 +++++++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/call_properties.ecl b/automation/eclair_analysis/ECLAIR/call_properties.ecl
> index 3f7794bf8b..f410a6aa58 100644
> --- a/automation/eclair_analysis/ECLAIR/call_properties.ecl
> +++ b/automation/eclair_analysis/ECLAIR/call_properties.ecl
> @@ -104,3 +104,13 @@ 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."
> +-call_properties+={"^read_u(8|16|32|64|int)_atomic.*$", {"noeffect"}}
> +-doc_end
> +
> +-doc_begin="get_cycles does not perform visible side-effects "
> +-call_property+={"name(get_cycles)", {"noeffect"}}
> +-doc_end
> +
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index fa56e5c00a..b80ccea7bc 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -233,6 +233,19 @@ this."
>  -config=MC3R1.R10.1,etypes+={safe,
>    "stmt(operator(and||or||xor||not||and_assign||or_assign||xor_assign))",
>    "any()"}
> +#
> +# Series 13.
> +#
> +
> +-doc_begin="Function __bad_atomic_size is intended to generate a linkage error
> +if invoked. Calling it in intentionally unreachable switch cases is safe even
> +in a initializer list."
> +-config=MC3R1.R13.1,reports+={safe, "first_area(^.*__bad_atomic_size.*$)"}
> +-doc_end
> +
> +-doc_begin="Function vcpu_runnable contains pragmas and other side-effects for
> +debugging purposes, their invocation is safe even in a initializer list."
> +-config=MC3R1.R13.1,reports+={safe, "first_area(^.*vcpu_runnable.*$)"}
>  -doc_end
>  
>  -doc_begin="See Section \"4.5 Integers\" of \"GCC_MANUAL\", where it says that
> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> index 8511a18925..2fcdb8da58 100644
> --- a/docs/misra/deviations.rst
> +++ b/docs/misra/deviations.rst
> @@ -192,6 +192,17 @@ Deviations related to MISRA C:2012 Rules:
>         See automation/eclair_analysis/deviations.ecl for the full explanation.
>       - Tagged as `safe` for ECLAIR.
>  
> +   * - R13.1
> +     - Function __bad_atomic_size is intended to generate a linkage error if
> +       invoked. Calling it in intentionally unreachable switch cases is
> +       safe even in a initializer list.
> +     - Tagged as `safe` for ECLAIR.
> +
> +   * - R13.1
> +     - Function vcpu_runnable contains pragmas and other side-effects for
> +       debugging purposes, their invocation is safe even in a initializer list.
> +     - Tagged as `safe` for ECLAIR.


Would it be possible to use SAF instead? If not, this is fine.

Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Stefano Stabellini]

On Wed, 18 Oct 2023, Julien Grall wrote:
> Hi,
> 
> On 18/10/2023 15:18, Simone Ballarin wrote:
> > Rule 13.1: Initializer lists shall not contain persistent side effects
> > 
> > This patch moves expressions with side-effects into new variables before
> > the initializer lists.
> 
> Looking at the code, I feel the commit message should be a bit more verbose
> because they are no apparent side-effects.
> 
> > 
> > Function calls do not necessarily have side-effects, in these cases the
> > GCC pure or const attributes are added when possible.
> 
> You are only adding pure in this patch.
> 
> > 
> > No functional changes.
> > 
> > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> > 
> > ---
> > Function attributes pure and const do not need to be added as GCC
> > attributes, they can be added using ECLAIR configurations.
> > ---
> >   xen/arch/arm/device.c              |  6 +++---
> >   xen/arch/arm/guestcopy.c           | 12 ++++++++----
> >   xen/arch/arm/include/asm/current.h |  2 +-
> >   3 files changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> > index 1f631d3274..e9be078415 100644
> > --- a/xen/arch/arm/device.c
> > +++ b/xen/arch/arm/device.c
> > @@ -319,6 +319,8 @@ int handle_device(struct domain *d, struct
> > dt_device_node *dev, p2m_type_t p2mt,
> >       int res;
> >       paddr_t addr, size;
> >       bool own_device = !dt_device_for_passthrough(dev);
> > +    bool dev_is_hostbridge = is_pci_passthrough_enabled() &&
> > +                             device_get_class(dev) ==
> > DEVICE_PCI_HOSTBRIDGE;
> 
> The commit message suggests that the code is moved because there are
> side-effects. But none of them should have any side-effects.
> 
> In fact, if I am not mistaken, both is_pci_passthrough_enabled() and
> device_get_class() could be marked as pure.
> 
> >       /*
> >        * We want to avoid mapping the MMIO in dom0 for the following cases:
> >        *   - The device is owned by dom0 (i.e. it has been flagged for
> > @@ -329,9 +331,7 @@ int handle_device(struct domain *d, struct
> > dt_device_node *dev, p2m_type_t p2mt,
> >       struct map_range_data mr_data = {
> >           .d = d,
> >           .p2mt = p2mt,
> > -        .skip_mapping = !own_device ||
> > -                        (is_pci_passthrough_enabled() &&
> > -                        (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
> > +        .skip_mapping = !own_device || dev_is_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..3ec6743bf6 100644
> > --- a/xen/arch/arm/guestcopy.c
> > +++ b/xen/arch/arm/guestcopy.c
> > @@ -109,27 +109,31 @@ 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)
> >   {
> > +    struct vcpu *current_vcpu = current;
> 
> It is not clear to me what is the perceived side effect here and the others
> below. Can you clarify?

I am guessing that's because current is a global variable? But only
reading (not writing) a global variable should be OK?

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Stefano Stabellini]

On Wed, 18 Oct 2023, Simone Ballarin wrote:
> Add const and pure attributes to address reports
> of MISRA C:2012 Rule 13.1: Initializer lists shall
> not contain persistent side effects
> 
> Add pure attribute to function pdx_to_pfn.
> Add const attribute to functions generated by TYPE_SAFE.
> 
> These functions are used in initializer lists: adding
> the attributes ensures that no effect will be performed
> by them.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

However one comment below...

> ---
> Function attributes pure and const do not need to be added as GCC
> attributes, they can be added using ECLAIR configurations.
> ---
>  xen/include/xen/pdx.h      | 2 +-
>  xen/include/xen/typesafe.h | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h
> index f3fbc4273a..5d1050967a 100644
> --- a/xen/include/xen/pdx.h
> +++ b/xen/include/xen/pdx.h
> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
>   * @param pdx Page index
>   * @return Obtained pfn after decompressing the pdx
>   */
> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
>  {
>      return (pdx & pfn_pdx_bottom_mask) |
>             ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
> diff --git a/xen/include/xen/typesafe.h b/xen/include/xen/typesafe.h
> index 7ecd3b4a8d..615df6f944 100644
> --- a/xen/include/xen/typesafe.h
> +++ b/xen/include/xen/typesafe.h
> @@ -21,8 +21,8 @@
>  
>  #define TYPE_SAFE(_type, _name)                                         \
>      typedef struct { _type _name; } _name##_t;                          \
> -    static inline _name##_t _##_name(_type n) { return (_name##_t) { n }; } \
> -    static inline _type _name##_x(_name##_t n) { return n._name; }
> +    static inline __attribute_const__ _name##_t _##_name(_type n) { return (_name##_t) { n }; } \
> +    static inline __attribute_const__ _type _name##_x(_name##_t n) { return n._name; }
>  
>  #else

I think we should also add __attribute_const__ in the NDEBUG definitions
just below.

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

[Stefano Stabellini]

On Wed, 18 Oct 2023, Simone Ballarin wrote:
> Rule 13.1: Initializer lists shall not contain persistent side effects
> 
> This patch moves expressions with side-effects outside the initializer lists.
> 
> No functional changes.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> ---
>  xen/common/sched/core.c    | 16 ++++++++++++----
>  xen/drivers/char/ns16550.c |  4 +++-
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 12deefa745..519884f989 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -1504,6 +1504,8 @@ long vcpu_yield(void)
>  {
>      struct vcpu * v=current;
>      spinlock_t *lock;
> +    domid_t domain_id;
> +    int vcpu_id;
>  
>      rcu_read_lock(&sched_res_rculock);
>  
> @@ -1515,7 +1517,9 @@ long vcpu_yield(void)
>  
>      SCHED_STAT_CRANK(vcpu_yield);
>  
> -    TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
> +    domain_id = current->domain->domain_id;
> +    vcpu_id = current->vcpu_id;
> +    TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id);

Also here it looks like accessing the current pointer is considered a
side effect. Why? This is a just a global variable that is only accessed
for reading.


>      raise_softirq(SCHEDULE_SOFTIRQ);
>      return 0;
>  }
> @@ -1888,14 +1892,17 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>      case SCHEDOP_shutdown:
>      {
>          struct sched_shutdown sched_shutdown;
> +        domid_t domain_id;
> +        int vcpu_id;
>  
>          ret = -EFAULT;
>          if ( copy_from_guest(&sched_shutdown, arg, 1) )
>              break;
>  
> +        domain_id = current->domain->domain_id;
> +        vcpu_id = current->vcpu_id;
>          TRACE_3D(TRC_SCHED_SHUTDOWN,
> -                 current->domain->domain_id, current->vcpu_id,
> -                 sched_shutdown.reason);
> +                 domain_id, vcpu_id, sched_shutdown.reason);
>          ret = domain_shutdown(current->domain, (u8)sched_shutdown.reason);
>  
>          break;
> @@ -1905,13 +1912,14 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>      {
>          struct sched_shutdown sched_shutdown;
>          struct domain *d = current->domain;
> +        int vcpu_id = current->vcpu_id;
>  
>          ret = -EFAULT;
>          if ( copy_from_guest(&sched_shutdown, arg, 1) )
>              break;
>  
>          TRACE_3D(TRC_SCHED_SHUTDOWN_CODE,
> -                 d->domain_id, current->vcpu_id, sched_shutdown.reason);
> +                 d->domain_id, vcpu_id, sched_shutdown.reason);
>  
>          spin_lock(&d->shutdown_lock);
>          if ( d->shutdown_code == SHUTDOWN_CODE_INVALID )
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 28ddedd50d..0b3d8b2a30 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -445,10 +445,12 @@ 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
>              };
>  
> +            rc = uart->irq;

What's the side effect here? If the side effect is rc = uart->irq, why
is it considered a side effect?

Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin]

On 18/10/23 17:03, Julien Grall wrote:
> Hi,
> 
> On 18/10/2023 15:18, Simone Ballarin wrote:
>> Rule 13.1: Initializer lists shall not contain persistent side effects
>>
>> This patch moves expressions with side-effects into new variables before
>> the initializer lists.
> 
> Looking at the code, I feel the commit message should be a bit more 
> verbose because they are no apparent side-effects.
> 
>>
>> Function calls do not necessarily have side-effects, in these cases the
>> GCC pure or const attributes are added when possible.
> 
> You are only adding pure in this patch.
> 
>>
>> No functional changes.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>
>> ---
>> Function attributes pure and const do not need to be added as GCC
>> attributes, they can be added using ECLAIR configurations.
>> ---
>>   xen/arch/arm/device.c              |  6 +++---
>>   xen/arch/arm/guestcopy.c           | 12 ++++++++----
>>   xen/arch/arm/include/asm/current.h |  2 +-
>>   3 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
>> index 1f631d3274..e9be078415 100644
>> --- a/xen/arch/arm/device.c
>> +++ b/xen/arch/arm/device.c
>> @@ -319,6 +319,8 @@ int handle_device(struct domain *d, struct 
>> dt_device_node *dev, p2m_type_t p2mt,
>>       int res;
>>       paddr_t addr, size;
>>       bool own_device = !dt_device_for_passthrough(dev);
>> +    bool dev_is_hostbridge = is_pci_passthrough_enabled() &&
>> +                             device_get_class(dev) == 
>> DEVICE_PCI_HOSTBRIDGE;
> 
> The commit message suggests that the code is moved because there are 
> side-effects. But none of them should have any side-effects.
> 

device_get_class contains an 'ASSERT(dev != NULL)' which is definitely
a side-effect.

I know that this kind of side-effect can be easily deviated, but 
considering the easy fix, I've decided to move the call outside.

By the way, I totally agree with you if you prefer to deviate.

> In fact, if I am not mistaken, both is_pci_passthrough_enabled() and 
> device_get_class() could be marked as pure.
>

Considering the ASSERT, I do not think we can put the attribute pure.

>>       /*
>>        * We want to avoid mapping the MMIO in dom0 for the following 
>> cases:
>>        *   - The device is owned by dom0 (i.e. it has been flagged for
>> @@ -329,9 +331,7 @@ int handle_device(struct domain *d, struct 
>> dt_device_node *dev, p2m_type_t p2mt,
>>       struct map_range_data mr_data = {
>>           .d = d,
>>           .p2mt = p2mt,
>> -        .skip_mapping = !own_device ||
>> -                        (is_pci_passthrough_enabled() &&
>> -                        (device_get_class(dev) == 
>> DEVICE_PCI_HOSTBRIDGE)),
>> +        .skip_mapping = !own_device || dev_is_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..3ec6743bf6 100644
>> --- a/xen/arch/arm/guestcopy.c
>> +++ b/xen/arch/arm/guestcopy.c
>> @@ -109,27 +109,31 @@ 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)
>>   {
>> +    struct vcpu *current_vcpu = current;
> 
> It is not clear to me what is the perceived side effect here and the 
> others below. Can you clarify?
> 

I will use the pure attribute.

>>       return copy_guest((void *)from, (vaddr_t)to, len,
>> -                      GVA_INFO(current), COPY_to_guest | COPY_linear);
>> +                      GVA_INFO(current_vcpu), COPY_to_guest | 
>> COPY_linear);
>>   }
>>   unsigned long raw_copy_to_guest_flush_dcache(void *to, const void 
>> *from,
>>                                                unsigned int len)
>>   {
>> -    return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current),
>> +    struct vcpu *current_vcpu = current;
>> +    return copy_guest((void *)from, (vaddr_t)to, len, 
>> GVA_INFO(current_vcpu),
>>                         COPY_to_guest | COPY_flush_dcache | COPY_linear);
>>   }
>>   unsigned long raw_clear_guest(void *to, unsigned int len)
>>   {
>> -    return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current),
>> +    struct vcpu *current_vcpu = current;
>> +    return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current_vcpu),
>>                         COPY_to_guest | COPY_linear);
>>   }
>>   unsigned long raw_copy_from_guest(void *to, const void __user *from,
>>                                     unsigned int len)
>>   {
>> -    return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current),
>> +    struct vcpu *current_vcpu = current;
>> +    return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current_vcpu),
>>                         COPY_from_guest | COPY_linear);
>>   }
>> diff --git a/xen/arch/arm/include/asm/current.h 
>> b/xen/arch/arm/include/asm/current.h
>> index 6973eeb1d1..a66e28fefb 100644
>> --- a/xen/arch/arm/include/asm/current.h
>> +++ b/xen/arch/arm/include/asm/current.h
>> @@ -28,7 +28,7 @@ struct cpu_info {
>>       uint32_t flags;
>>   };
>> -static inline struct cpu_info *get_cpu_info(void)
>> +static inline __attribute_pure__ struct cpu_info *get_cpu_info(void)
>>   {
>>   #ifdef __clang__
>>       unsigned long sp;
> 
> Cheers,
> 

-- 
Simone Ballarin, M.Sc.

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

Re: [XEN PATCH 2/4] automation/eclair: add deviations and call properties.

[Simone Ballarin]

On 19/10/23 02:57, Stefano Stabellini wrote:
> On Wed, 18 Oct 2023, Simone Ballarin wrote:
>> Deviate violations of MISRA C:2012 Rule 13.1 caused by
>> functions vcpu_runnable and __bad_atomic_size. These functions
>> contain side-effects such as debugging logs, assertions and
>> failures that cannot cause unexpected behaviors.
>>
>> Add "noeffect" call property to functions read_u.*_atomic and
>> get_cycles.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> However one comment below
> 
> 
>> ---
>>   .../eclair_analysis/ECLAIR/call_properties.ecl      | 10 ++++++++++
>>   automation/eclair_analysis/ECLAIR/deviations.ecl    | 13 +++++++++++++
>>   docs/misra/deviations.rst                           | 11 +++++++++++
>>   3 files changed, 34 insertions(+)
>>
>> diff --git a/automation/eclair_analysis/ECLAIR/call_properties.ecl b/automation/eclair_analysis/ECLAIR/call_properties.ecl
>> index 3f7794bf8b..f410a6aa58 100644
>> --- a/automation/eclair_analysis/ECLAIR/call_properties.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/call_properties.ecl
>> @@ -104,3 +104,13 @@ 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."
>> +-call_properties+={"^read_u(8|16|32|64|int)_atomic.*$", {"noeffect"}}
>> +-doc_end
>> +
>> +-doc_begin="get_cycles does not perform visible side-effects "
>> +-call_property+={"name(get_cycles)", {"noeffect"}}
>> +-doc_end
>> +
>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> index fa56e5c00a..b80ccea7bc 100644
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -233,6 +233,19 @@ this."
>>   -config=MC3R1.R10.1,etypes+={safe,
>>     "stmt(operator(and||or||xor||not||and_assign||or_assign||xor_assign))",
>>     "any()"}
>> +#
>> +# Series 13.
>> +#
>> +
>> +-doc_begin="Function __bad_atomic_size is intended to generate a linkage error
>> +if invoked. Calling it in intentionally unreachable switch cases is safe even
>> +in a initializer list."
>> +-config=MC3R1.R13.1,reports+={safe, "first_area(^.*__bad_atomic_size.*$)"}
>> +-doc_end
>> +
>> +-doc_begin="Function vcpu_runnable contains pragmas and other side-effects for
>> +debugging purposes, their invocation is safe even in a initializer list."
>> +-config=MC3R1.R13.1,reports+={safe, "first_area(^.*vcpu_runnable.*$)"}
>>   -doc_end
>>   
>>   -doc_begin="See Section \"4.5 Integers\" of \"GCC_MANUAL\", where it says that
>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>> index 8511a18925..2fcdb8da58 100644
>> --- a/docs/misra/deviations.rst
>> +++ b/docs/misra/deviations.rst
>> @@ -192,6 +192,17 @@ Deviations related to MISRA C:2012 Rules:
>>          See automation/eclair_analysis/deviations.ecl for the full explanation.
>>        - Tagged as `safe` for ECLAIR.
>>   
>> +   * - R13.1
>> +     - Function __bad_atomic_size is intended to generate a linkage error if
>> +       invoked. Calling it in intentionally unreachable switch cases is
>> +       safe even in a initializer list.
>> +     - Tagged as `safe` for ECLAIR.
>> +
>> +   * - R13.1
>> +     - Function vcpu_runnable contains pragmas and other side-effects for
>> +       debugging purposes, their invocation is safe even in a initializer list.
>> +     - Tagged as `safe` for ECLAIR.
> 
> 
> Would it be possible to use SAF instead? If not, this is fine.

Yes, but I do not suggest using SAF comments in these cases.
The SAF should be placed before every non-compliant invocation and not 
only in the declaration, this could cause a lot of SAF comments instead
of a simple deviation.

-- 
Simone Ballarin, M.Sc.

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

Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Julien Grall]

Hi Simone,

On 19/10/2023 08:34, Simone Ballarin wrote:
> On 18/10/23 17:03, Julien Grall wrote:
>> Hi,
>>
>> On 18/10/2023 15:18, Simone Ballarin wrote:
>>> Rule 13.1: Initializer lists shall not contain persistent side effects
>>>
>>> This patch moves expressions with side-effects into new variables before
>>> the initializer lists.
>>
>> Looking at the code, I feel the commit message should be a bit more 
>> verbose because they are no apparent side-effects.
>>
>>>
>>> Function calls do not necessarily have side-effects, in these cases the
>>> GCC pure or const attributes are added when possible.
>>
>> You are only adding pure in this patch.
>>
>>>
>>> No functional changes.
>>>
>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>>
>>> ---
>>> Function attributes pure and const do not need to be added as GCC
>>> attributes, they can be added using ECLAIR configurations.
>>> ---
>>>   xen/arch/arm/device.c              |  6 +++---
>>>   xen/arch/arm/guestcopy.c           | 12 ++++++++----
>>>   xen/arch/arm/include/asm/current.h |  2 +-
>>>   3 files changed, 12 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
>>> index 1f631d3274..e9be078415 100644
>>> --- a/xen/arch/arm/device.c
>>> +++ b/xen/arch/arm/device.c
>>> @@ -319,6 +319,8 @@ int handle_device(struct domain *d, struct 
>>> dt_device_node *dev, p2m_type_t p2mt,
>>>       int res;
>>>       paddr_t addr, size;
>>>       bool own_device = !dt_device_for_passthrough(dev);
>>> +    bool dev_is_hostbridge = is_pci_passthrough_enabled() &&
>>> +                             device_get_class(dev) == 
>>> DEVICE_PCI_HOSTBRIDGE;
>>
>> The commit message suggests that the code is moved because there are 
>> side-effects. But none of them should have any side-effects.
>>
> 
> device_get_class contains an 'ASSERT(dev != NULL)' which is definitely
> a side-effect.

Just to confirm my understanding, the side-effect here would be the fact 
that it traps and then panic(). IOW, if the trap was hypothetically 
replaced by a while (1), then it would not be an issue. is it correct?

I can see two solutions:
   1) Remove the ASSERT(). It is only here to make the NULL dereference 
obvious in debug build. That said, the stack trace for a NULL 
dereference would still be as clear.
   2) Replace the ASSERT with a proper check if ( !dev ) return 
DEVICE_UNKONWN. AFAIU, we would not be able to add a 
ASSERT_UNREACHABLE() because this would be again a perceived side-effect.

The former feels a bit circumventing MISRA as the side effect is 
technically still present. Just hidden. But I do prefer over 2).

>>>       /*
>>>        * We want to avoid mapping the MMIO in dom0 for the following 
>>> cases:
>>>        *   - The device is owned by dom0 (i.e. it has been flagged for
>>> @@ -329,9 +331,7 @@ int handle_device(struct domain *d, struct 
>>> dt_device_node *dev, p2m_type_t p2mt,
>>>       struct map_range_data mr_data = {
>>>           .d = d,
>>>           .p2mt = p2mt,
>>> -        .skip_mapping = !own_device ||
>>> -                        (is_pci_passthrough_enabled() &&
>>> -                        (device_get_class(dev) == 
>>> DEVICE_PCI_HOSTBRIDGE)),
>>> +        .skip_mapping = !own_device || dev_is_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..3ec6743bf6 100644
>>> --- a/xen/arch/arm/guestcopy.c
>>> +++ b/xen/arch/arm/guestcopy.c
>>> @@ -109,27 +109,31 @@ 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)
>>>   {
>>> +    struct vcpu *current_vcpu = current;
>>
>> It is not clear to me what is the perceived side effect here and the 
>> others below. Can you clarify?
>>
> 
> I will use the pure attribute.

So x86 is using a function to define current. But AFAICT this is not the 
case on Arm. So how would you add the pure?

If it is by adding a function, then I would first like to understand 
which part 'current' is currently perceived as a side-effect.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 2/4] automation/eclair: add deviations and call properties.

[Julien Grall]

Hi Simone,

On 19/10/2023 08:44, Simone Ballarin wrote:
> On 19/10/23 02:57, Stefano Stabellini wrote:
>> On Wed, 18 Oct 2023, Simone Ballarin wrote:
>>> Deviate violations of MISRA C:2012 Rule 13.1 caused by
>>> functions vcpu_runnable and __bad_atomic_size. These functions
>>> contain side-effects such as debugging logs, assertions and
>>> failures that cannot cause unexpected behaviors.
>>>
>>> Add "noeffect" call property to functions read_u.*_atomic and
>>> get_cycles.
>>>
>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>
>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> However one comment below
>>
>>
>>> ---
>>>   .../eclair_analysis/ECLAIR/call_properties.ecl      | 10 ++++++++++
>>>   automation/eclair_analysis/ECLAIR/deviations.ecl    | 13 +++++++++++++
>>>   docs/misra/deviations.rst                           | 11 +++++++++++
>>>   3 files changed, 34 insertions(+)
>>>
>>> diff --git a/automation/eclair_analysis/ECLAIR/call_properties.ecl 
>>> b/automation/eclair_analysis/ECLAIR/call_properties.ecl
>>> index 3f7794bf8b..f410a6aa58 100644
>>> --- a/automation/eclair_analysis/ECLAIR/call_properties.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/call_properties.ecl
>>> @@ -104,3 +104,13 @@ 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."
>>> +-call_properties+={"^read_u(8|16|32|64|int)_atomic.*$", {"noeffect"}}
>>> +-doc_end
>>> +
>>> +-doc_begin="get_cycles does not perform visible side-effects "
>>> +-call_property+={"name(get_cycles)", {"noeffect"}}
>>> +-doc_end
>>> +
>>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
>>> b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> index fa56e5c00a..b80ccea7bc 100644
>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> @@ -233,6 +233,19 @@ this."
>>>   -config=MC3R1.R10.1,etypes+={safe,
>>>     
>>> "stmt(operator(and||or||xor||not||and_assign||or_assign||xor_assign))",
>>>     "any()"}
>>> +#
>>> +# Series 13.
>>> +#
>>> +
>>> +-doc_begin="Function __bad_atomic_size is intended to generate a 
>>> linkage error
>>> +if invoked. Calling it in intentionally unreachable switch cases is 
>>> safe even
>>> +in a initializer list."
>>> +-config=MC3R1.R13.1,reports+={safe, 
>>> "first_area(^.*__bad_atomic_size.*$)"}
>>> +-doc_end
>>> +
>>> +-doc_begin="Function vcpu_runnable contains pragmas and other 
>>> side-effects for
>>> +debugging purposes, their invocation is safe even in a initializer 
>>> list."
>>> +-config=MC3R1.R13.1,reports+={safe, "first_area(^.*vcpu_runnable.*$)"}
>>>   -doc_end
>>>   -doc_begin="See Section \"4.5 Integers\" of \"GCC_MANUAL\", where 
>>> it says that
>>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>>> index 8511a18925..2fcdb8da58 100644
>>> --- a/docs/misra/deviations.rst
>>> +++ b/docs/misra/deviations.rst
>>> @@ -192,6 +192,17 @@ Deviations related to MISRA C:2012 Rules:
>>>          See automation/eclair_analysis/deviations.ecl for the full 
>>> explanation.
>>>        - Tagged as `safe` for ECLAIR.
>>> +   * - R13.1
>>> +     - Function __bad_atomic_size is intended to generate a linkage 
>>> error if
>>> +       invoked. Calling it in intentionally unreachable switch cases is
>>> +       safe even in a initializer list.
>>> +     - Tagged as `safe` for ECLAIR.
>>> +
>>> +   * - R13.1
>>> +     - Function vcpu_runnable contains pragmas and other 
>>> side-effects for
>>> +       debugging purposes, their invocation is safe even in a 
>>> initializer list.
>>> +     - Tagged as `safe` for ECLAIR.
>>
>>
>> Would it be possible to use SAF instead? If not, this is fine.
> 
> Yes, but I do not suggest using SAF comments in these cases.

There are not many use of __bad_atomic_size() and I don't expect much 
more. So I think SAF- makes more sense.

For vcpu_runnable(), I don't quite understand the argument. I can't find 
any pragma around the function and I can't find any side-effects in it. 
Can you clarify?

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin]

On 19/10/23 10:19, Julien Grall wrote:
> Hi Simone,
> 
> On 19/10/2023 08:34, Simone Ballarin wrote:
>> On 18/10/23 17:03, Julien Grall wrote:
>>> Hi,
>>>
>>> On 18/10/2023 15:18, Simone Ballarin wrote:
>>>> Rule 13.1: Initializer lists shall not contain persistent side effects
>>>>
>>>> This patch moves expressions with side-effects into new variables 
>>>> before
>>>> the initializer lists.
>>>
>>> Looking at the code, I feel the commit message should be a bit more 
>>> verbose because they are no apparent side-effects.
>>>
>>>>
>>>> Function calls do not necessarily have side-effects, in these cases the
>>>> GCC pure or const attributes are added when possible.
>>>
>>> You are only adding pure in this patch.
>>>
>>>>
>>>> No functional changes.
>>>>
>>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>>>
>>>> ---
>>>> Function attributes pure and const do not need to be added as GCC
>>>> attributes, they can be added using ECLAIR configurations.
>>>> ---
>>>>   xen/arch/arm/device.c              |  6 +++---
>>>>   xen/arch/arm/guestcopy.c           | 12 ++++++++----
>>>>   xen/arch/arm/include/asm/current.h |  2 +-
>>>>   3 files changed, 12 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
>>>> index 1f631d3274..e9be078415 100644
>>>> --- a/xen/arch/arm/device.c
>>>> +++ b/xen/arch/arm/device.c
>>>> @@ -319,6 +319,8 @@ int handle_device(struct domain *d, struct 
>>>> dt_device_node *dev, p2m_type_t p2mt,
>>>>       int res;
>>>>       paddr_t addr, size;
>>>>       bool own_device = !dt_device_for_passthrough(dev);
>>>> +    bool dev_is_hostbridge = is_pci_passthrough_enabled() &&
>>>> +                             device_get_class(dev) == 
>>>> DEVICE_PCI_HOSTBRIDGE;
>>>
>>> The commit message suggests that the code is moved because there are 
>>> side-effects. But none of them should have any side-effects.
>>>
>>
>> device_get_class contains an 'ASSERT(dev != NULL)' which is definitely
>> a side-effect.
> 
> Just to confirm my understanding, the side-effect here would be the fact 
> that it traps and then panic(). IOW, if the trap was hypothetically 
> replaced by a while (1), then it would not be an issue. is it correct? >

No, it isn't. A infinite loop is a side effect.

> I can see two solutions:
>    1) Remove the ASSERT(). It is only here to make the NULL dereference 
> obvious in debug build. That said, the stack trace for a NULL 
> dereference would still be as clear.

Removing the ASSERT just to make MISRA happy does not sound good to me.

>    2) Replace the ASSERT with a proper check if ( !dev ) return 
> DEVICE_UNKONWN. AFAIU, we would not be able to add a 
> ASSERT_UNREACHABLE() because this would be again a perceived side-effect.
> 

Replacing it with a proper check can be a solution, but I still prefer 
to add a deviation or move the invocation outside the initializer list.

> The former feels a bit circumventing MISRA as the side effect is 
> technically still present. Just hidden. But I do prefer over 2).
> 
>>>>       /*
>>>>        * We want to avoid mapping the MMIO in dom0 for the following 
>>>> cases:
>>>>        *   - The device is owned by dom0 (i.e. it has been flagged for
>>>> @@ -329,9 +331,7 @@ int handle_device(struct domain *d, struct 
>>>> dt_device_node *dev, p2m_type_t p2mt,
>>>>       struct map_range_data mr_data = {
>>>>           .d = d,
>>>>           .p2mt = p2mt,
>>>> -        .skip_mapping = !own_device ||
>>>> -                        (is_pci_passthrough_enabled() &&
>>>> -                        (device_get_class(dev) == 
>>>> DEVICE_PCI_HOSTBRIDGE)),
>>>> +        .skip_mapping = !own_device || dev_is_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..3ec6743bf6 100644
>>>> --- a/xen/arch/arm/guestcopy.c
>>>> +++ b/xen/arch/arm/guestcopy.c
>>>> @@ -109,27 +109,31 @@ 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)
>>>>   {
>>>> +    struct vcpu *current_vcpu = current;
>>>
>>> It is not clear to me what is the perceived side effect here and the 
>>> others below. Can you clarify?
>>>
>>
>> I will use the pure attribute.
> So x86 is using a function to define current. But AFAICT this is not the 
> case on Arm. So how would you add the pure?
> 
> If it is by adding a function, then I would first like to understand 
> which part 'current' is currently perceived as a side-effect.
> 

Yes, sorry I was looking to the wrong definition.
In ARM the problem is the presence of a *volatile* ASM.
Please take a look here:

https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set2/latest/PROJECT.ecd;/by_service/MC3R1.R13.1.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":true,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"fingerprint","inputs":[{"enabled":true,"text":"0da7f0c9aea5491eba343618f965c81f5d7aed3c"}]}}}

> Cheers,
> 

-- 
Simone Ballarin, M.Sc.

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

Re: [XEN PATCH 2/4] automation/eclair: add deviations and call properties.

[Simone Ballarin]

On 19/10/23 10:26, Julien Grall wrote:
> Hi Simone,
> 
> On 19/10/2023 08:44, Simone Ballarin wrote:
>> On 19/10/23 02:57, Stefano Stabellini wrote:
>>> On Wed, 18 Oct 2023, Simone Ballarin wrote:
>>>> Deviate violations of MISRA C:2012 Rule 13.1 caused by
>>>> functions vcpu_runnable and __bad_atomic_size. These functions
>>>> contain side-effects such as debugging logs, assertions and
>>>> failures that cannot cause unexpected behaviors.
>>>>
>>>> Add "noeffect" call property to functions read_u.*_atomic and
>>>> get_cycles.
>>>>
>>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>>
>>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>>>
>>> However one comment below
>>>
>>>
>>>> ---
>>>>   .../eclair_analysis/ECLAIR/call_properties.ecl      | 10 ++++++++++
>>>>   automation/eclair_analysis/ECLAIR/deviations.ecl    | 13 
>>>> +++++++++++++
>>>>   docs/misra/deviations.rst                           | 11 +++++++++++
>>>>   3 files changed, 34 insertions(+)
>>>>
>>>> diff --git a/automation/eclair_analysis/ECLAIR/call_properties.ecl 
>>>> b/automation/eclair_analysis/ECLAIR/call_properties.ecl
>>>> index 3f7794bf8b..f410a6aa58 100644
>>>> --- a/automation/eclair_analysis/ECLAIR/call_properties.ecl
>>>> +++ b/automation/eclair_analysis/ECLAIR/call_properties.ecl
>>>> @@ -104,3 +104,13 @@ 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."
>>>> +-call_properties+={"^read_u(8|16|32|64|int)_atomic.*$", {"noeffect"}}
>>>> +-doc_end
>>>> +
>>>> +-doc_begin="get_cycles does not perform visible side-effects "
>>>> +-call_property+={"name(get_cycles)", {"noeffect"}}
>>>> +-doc_end
>>>> +
>>>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
>>>> b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>> index fa56e5c00a..b80ccea7bc 100644
>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>> @@ -233,6 +233,19 @@ this."
>>>>   -config=MC3R1.R10.1,etypes+={safe,
>>>> "stmt(operator(and||or||xor||not||and_assign||or_assign||xor_assign))",
>>>>     "any()"}
>>>> +#
>>>> +# Series 13.
>>>> +#
>>>> +
>>>> +-doc_begin="Function __bad_atomic_size is intended to generate a 
>>>> linkage error
>>>> +if invoked. Calling it in intentionally unreachable switch cases is 
>>>> safe even
>>>> +in a initializer list."
>>>> +-config=MC3R1.R13.1,reports+={safe, 
>>>> "first_area(^.*__bad_atomic_size.*$)"}
>>>> +-doc_end
>>>> +
>>>> +-doc_begin="Function vcpu_runnable contains pragmas and other 
>>>> side-effects for
>>>> +debugging purposes, their invocation is safe even in a initializer 
>>>> list."
>>>> +-config=MC3R1.R13.1,reports+={safe, "first_area(^.*vcpu_runnable.*$)"}
>>>>   -doc_end
>>>>   -doc_begin="See Section \"4.5 Integers\" of \"GCC_MANUAL\", where 
>>>> it says that
>>>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>>>> index 8511a18925..2fcdb8da58 100644
>>>> --- a/docs/misra/deviations.rst
>>>> +++ b/docs/misra/deviations.rst
>>>> @@ -192,6 +192,17 @@ Deviations related to MISRA C:2012 Rules:
>>>>          See automation/eclair_analysis/deviations.ecl for the full 
>>>> explanation.
>>>>        - Tagged as `safe` for ECLAIR.
>>>> +   * - R13.1
>>>> +     - Function __bad_atomic_size is intended to generate a linkage 
>>>> error if
>>>> +       invoked. Calling it in intentionally unreachable switch 
>>>> cases is
>>>> +       safe even in a initializer list.
>>>> +     - Tagged as `safe` for ECLAIR.
>>>> +
>>>> +   * - R13.1
>>>> +     - Function vcpu_runnable contains pragmas and other 
>>>> side-effects for
>>>> +       debugging purposes, their invocation is safe even in a 
>>>> initializer list.
>>>> +     - Tagged as `safe` for ECLAIR.
>>>
>>>
>>> Would it be possible to use SAF instead? If not, this is fine.
>>
>> Yes, but I do not suggest using SAF comments in these cases.
> 
> There are not many use of __bad_atomic_size() and I don't expect much 
> more. So I think SAF- makes more sense.

There are 13 invocations in the preprocessed code: some of them are 
contained in macro expansions. Adding a SAF in a macro means to add 
useless comments if the macro is not expanded in an initializer list.

The correct thing would be adding 13 SAFs. If this is ok, I can do it.

> 
> For vcpu_runnable(), I don't quite understand the argument. I can't find 
> any pragma around the function and I can't find any side-effects in it. 
> Can you clarify?
> 


vcpu_runnable calls atomic_read.

In ARM, atomic_read uses a volatile pointer (that's a side-effect).

In X86 it contains a MACRO (read_atomic) that contains the pragmas
and also __bad_atomic_size.

Maybe the text should discriminate the two cases.

> Cheers,
> 

-- 
Simone Ballarin, M.Sc.

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

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Simone Ballarin]

On 19/10/23 03:02, Stefano Stabellini wrote:
> On Wed, 18 Oct 2023, Simone Ballarin wrote:
>> Add const and pure attributes to address reports
>> of MISRA C:2012 Rule 13.1: Initializer lists shall
>> not contain persistent side effects
>>
>> Add pure attribute to function pdx_to_pfn.
>> Add const attribute to functions generated by TYPE_SAFE.
>>
>> These functions are used in initializer lists: adding
>> the attributes ensures that no effect will be performed
>> by them.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> However one comment below...
> 
>> ---
>> Function attributes pure and const do not need to be added as GCC
>> attributes, they can be added using ECLAIR configurations.
>> ---
>>   xen/include/xen/pdx.h      | 2 +-
>>   xen/include/xen/typesafe.h | 4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h
>> index f3fbc4273a..5d1050967a 100644
>> --- a/xen/include/xen/pdx.h
>> +++ b/xen/include/xen/pdx.h
>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
>>    * @param pdx Page index
>>    * @return Obtained pfn after decompressing the pdx
>>    */
>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
>>   {
>>       return (pdx & pfn_pdx_bottom_mask) |
>>              ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
>> diff --git a/xen/include/xen/typesafe.h b/xen/include/xen/typesafe.h
>> index 7ecd3b4a8d..615df6f944 100644
>> --- a/xen/include/xen/typesafe.h
>> +++ b/xen/include/xen/typesafe.h
>> @@ -21,8 +21,8 @@
>>   
>>   #define TYPE_SAFE(_type, _name)                                         \
>>       typedef struct { _type _name; } _name##_t;                          \
>> -    static inline _name##_t _##_name(_type n) { return (_name##_t) { n }; } \
>> -    static inline _type _name##_x(_name##_t n) { return n._name; }
>> +    static inline __attribute_const__ _name##_t _##_name(_type n) { return (_name##_t) { n }; } \
>> +    static inline __attribute_const__ _type _name##_x(_name##_t n) { return n._name; }
>>   
>>   #else
> 
> I think we should also add __attribute_const__ in the NDEBUG definitions
> just below.

Ok.

-- 
Simone Ballarin, M.Sc.

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

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

[Simone Ballarin]

On 19/10/23 03:06, Stefano Stabellini wrote:
> On Wed, 18 Oct 2023, Simone Ballarin wrote:
>> Rule 13.1: Initializer lists shall not contain persistent side effects
>>
>> This patch moves expressions with side-effects outside the initializer lists.
>>
>> No functional changes.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>> ---
>>   xen/common/sched/core.c    | 16 ++++++++++++----
>>   xen/drivers/char/ns16550.c |  4 +++-
>>   2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
>> index 12deefa745..519884f989 100644
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -1504,6 +1504,8 @@ long vcpu_yield(void)
>>   {
>>       struct vcpu * v=current;
>>       spinlock_t *lock;
>> +    domid_t domain_id;
>> +    int vcpu_id;
>>   
>>       rcu_read_lock(&sched_res_rculock);
>>   
>> @@ -1515,7 +1517,9 @@ long vcpu_yield(void)
>>   
>>       SCHED_STAT_CRANK(vcpu_yield);
>>   
>> -    TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
>> +    domain_id = current->domain->domain_id;
>> +    vcpu_id = current->vcpu_id;
>> +    TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id);
> 
> Also here it looks like accessing the current pointer is considered a
> side effect. Why? This is a just a global variable that is only accessed
> for reading.
> 
> 
>>       raise_softirq(SCHEDULE_SOFTIRQ);
>>       return 0;
>>   }
>> @@ -1888,14 +1892,17 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>       case SCHEDOP_shutdown:
>>       {
>>           struct sched_shutdown sched_shutdown;
>> +        domid_t domain_id;
>> +        int vcpu_id;
>>   
>>           ret = -EFAULT;
>>           if ( copy_from_guest(&sched_shutdown, arg, 1) )
>>               break;
>>   
>> +        domain_id = current->domain->domain_id;
>> +        vcpu_id = current->vcpu_id;
>>           TRACE_3D(TRC_SCHED_SHUTDOWN,
>> -                 current->domain->domain_id, current->vcpu_id,
>> -                 sched_shutdown.reason);
>> +                 domain_id, vcpu_id, sched_shutdown.reason);
>>           ret = domain_shutdown(current->domain, (u8)sched_shutdown.reason);
>>   
>>           break;
>> @@ -1905,13 +1912,14 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>       {
>>           struct sched_shutdown sched_shutdown;
>>           struct domain *d = current->domain;
>> +        int vcpu_id = current->vcpu_id;
>>   
>>           ret = -EFAULT;
>>           if ( copy_from_guest(&sched_shutdown, arg, 1) )
>>               break;
>>   
>>           TRACE_3D(TRC_SCHED_SHUTDOWN_CODE,
>> -                 d->domain_id, current->vcpu_id, sched_shutdown.reason);
>> +                 d->domain_id, vcpu_id, sched_shutdown.reason);
>>   
>>           spin_lock(&d->shutdown_lock);
>>           if ( d->shutdown_code == SHUTDOWN_CODE_INVALID )
>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>> index 28ddedd50d..0b3d8b2a30 100644
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -445,10 +445,12 @@ 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
>>               };
>>   
>> +            rc = uart->irq;
> 
> What's the side effect here? If the side effect is rc = uart->irq, why
> is it considered a side effect?
> 

Yes, rc = uart->irq is the side-effect: it writes a variable
declared outside the context of the expression.

Consider the following example:

int rc;

struct S s = {
   .x = rc = 2,
   .y = rc = 3
};

What's the value of rc?


-- 
Simone Ballarin, M.Sc.

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

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

[Jan Beulich]

On 19.10.2023 03:06, Stefano Stabellini wrote:
> On Wed, 18 Oct 2023, Simone Ballarin wrote:
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -1504,6 +1504,8 @@ long vcpu_yield(void)
>>  {
>>      struct vcpu * v=current;
>>      spinlock_t *lock;
>> +    domid_t domain_id;
>> +    int vcpu_id;
>>  
>>      rcu_read_lock(&sched_res_rculock);
>>  
>> @@ -1515,7 +1517,9 @@ long vcpu_yield(void)
>>  
>>      SCHED_STAT_CRANK(vcpu_yield);
>>  
>> -    TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
>> +    domain_id = current->domain->domain_id;
>> +    vcpu_id = current->vcpu_id;
>> +    TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id);
> 
> Also here it looks like accessing the current pointer is considered a
> side effect. Why? This is a just a global variable that is only accessed
> for reading.

Not exactly. It's a per-CPU variable access on Arm, but involves a
function call on x86. Still that function call has no other side
effects, but I guess Misra wouldn't honor this.

I'm afraid though that the suggested change violates rule 2.2, as
the two new assignments are dead code when !CONFIG_TRACEBUFFER.

Jan

Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Julien Grall]

Hi,

On 19/10/2023 09:43, Simone Ballarin wrote:
> On 19/10/23 10:19, Julien Grall wrote:
>> Hi Simone,
>>
>> On 19/10/2023 08:34, Simone Ballarin wrote:
>>> On 18/10/23 17:03, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 18/10/2023 15:18, Simone Ballarin wrote:
>>>>> Rule 13.1: Initializer lists shall not contain persistent side effects
>>>>>
>>>>> This patch moves expressions with side-effects into new variables 
>>>>> before
>>>>> the initializer lists.
>>>>
>>>> Looking at the code, I feel the commit message should be a bit more 
>>>> verbose because they are no apparent side-effects.
>>>>
>>>>>
>>>>> Function calls do not necessarily have side-effects, in these cases 
>>>>> the
>>>>> GCC pure or const attributes are added when possible.
>>>>
>>>> You are only adding pure in this patch.
>>>>
>>>>>
>>>>> No functional changes.
>>>>>
>>>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>>>>
>>>>> ---
>>>>> Function attributes pure and const do not need to be added as GCC
>>>>> attributes, they can be added using ECLAIR configurations.
>>>>> ---
>>>>>   xen/arch/arm/device.c              |  6 +++---
>>>>>   xen/arch/arm/guestcopy.c           | 12 ++++++++----
>>>>>   xen/arch/arm/include/asm/current.h |  2 +-
>>>>>   3 files changed, 12 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
>>>>> index 1f631d3274..e9be078415 100644
>>>>> --- a/xen/arch/arm/device.c
>>>>> +++ b/xen/arch/arm/device.c
>>>>> @@ -319,6 +319,8 @@ int handle_device(struct domain *d, struct 
>>>>> dt_device_node *dev, p2m_type_t p2mt,
>>>>>       int res;
>>>>>       paddr_t addr, size;
>>>>>       bool own_device = !dt_device_for_passthrough(dev);
>>>>> +    bool dev_is_hostbridge = is_pci_passthrough_enabled() &&
>>>>> +                             device_get_class(dev) == 
>>>>> DEVICE_PCI_HOSTBRIDGE;
>>>>
>>>> The commit message suggests that the code is moved because there are 
>>>> side-effects. But none of them should have any side-effects.
>>>>
>>>
>>> device_get_class contains an 'ASSERT(dev != NULL)' which is definitely
>>> a side-effect.
>>
>> Just to confirm my understanding, the side-effect here would be the 
>> fact that it traps and then panic(). IOW, if the trap was 
>> hypothetically replaced by a while (1), then it would not be an issue. 
>> is it correct? >
> 
> No, it isn't. A infinite loop is a side effect.

I am not sure why. There are no change of state here.

> 
>> I can see two solutions:
>>    1) Remove the ASSERT(). It is only here to make the NULL 
>> dereference obvious in debug build. That said, the stack trace for a 
>> NULL dereference would still be as clear.
> 
> Removing the ASSERT just to make MISRA happy does not sound good to me.

I didn't suggest the ASSERT() just ot make MISRA happy. I suggested it 
because it has no value here (we still have stack track if there are any 
issue).

> 
>>    2) Replace the ASSERT with a proper check if ( !dev ) return 
>> DEVICE_UNKONWN. AFAIU, we would not be able to add a 
>> ASSERT_UNREACHABLE() because this would be again a perceived side-effect.
>>
> 
> Replacing it with a proper check can be a solution, but I still prefer 
> to add a deviation or move the invocation outside the initializer list.

In general, I am not in favor of adding deviation if we can avoid them 
because the code can changed and therefore either moot the deviation or 
hide any other issue.

[...]

> Yes, sorry I was looking to the wrong definition.
> In ARM the problem is the presence of a *volatile* ASM.
> Please take a look here:
> 
> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set2/latest/PROJECT.ecd;/by_service/MC3R1.R13.1.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":true,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"fingerprint","inputs":[{"enabled":true,"text":"0da7f0c9aea5491eba343618f965c81f5d7aed3c"}]}}}

Ok. So the problem is the READ_SYSREG(...). Is there a way we can 
encapsulate the call so we don't need to use your propose trick or 
deviate at every use of 'current'?

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin]

On 19/10/23 12:11, Julien Grall wrote:
> Hi,
> 
> On 19/10/2023 09:43, Simone Ballarin wrote:
>> On 19/10/23 10:19, Julien Grall wrote:
>>> Hi Simone,
>>>
>>> On 19/10/2023 08:34, Simone Ballarin wrote:
>>>> On 18/10/23 17:03, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 18/10/2023 15:18, Simone Ballarin wrote:
>>>>>> Rule 13.1: Initializer lists shall not contain persistent side 
>>>>>> effects
>>>>>>
>>>>>> This patch moves expressions with side-effects into new variables 
>>>>>> before
>>>>>> the initializer lists.
>>>>>
>>>>> Looking at the code, I feel the commit message should be a bit more 
>>>>> verbose because they are no apparent side-effects.
>>>>>
>>>>>>
>>>>>> Function calls do not necessarily have side-effects, in these 
>>>>>> cases the
>>>>>> GCC pure or const attributes are added when possible.
>>>>>
>>>>> You are only adding pure in this patch.
>>>>>
>>>>>>
>>>>>> No functional changes.
>>>>>>
>>>>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>>>>>
>>>>>> ---
>>>>>> Function attributes pure and const do not need to be added as GCC
>>>>>> attributes, they can be added using ECLAIR configurations.
>>>>>> ---
>>>>>>   xen/arch/arm/device.c              |  6 +++---
>>>>>>   xen/arch/arm/guestcopy.c           | 12 ++++++++----
>>>>>>   xen/arch/arm/include/asm/current.h |  2 +-
>>>>>>   3 files changed, 12 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
>>>>>> index 1f631d3274..e9be078415 100644
>>>>>> --- a/xen/arch/arm/device.c
>>>>>> +++ b/xen/arch/arm/device.c
>>>>>> @@ -319,6 +319,8 @@ int handle_device(struct domain *d, struct 
>>>>>> dt_device_node *dev, p2m_type_t p2mt,
>>>>>>       int res;
>>>>>>       paddr_t addr, size;
>>>>>>       bool own_device = !dt_device_for_passthrough(dev);
>>>>>> +    bool dev_is_hostbridge = is_pci_passthrough_enabled() &&
>>>>>> +                             device_get_class(dev) == 
>>>>>> DEVICE_PCI_HOSTBRIDGE;
>>>>>
>>>>> The commit message suggests that the code is moved because there 
>>>>> are side-effects. But none of them should have any side-effects.
>>>>>
>>>>
>>>> device_get_class contains an 'ASSERT(dev != NULL)' which is definitely
>>>> a side-effect.
>>>
>>> Just to confirm my understanding, the side-effect here would be the 
>>> fact that it traps and then panic(). IOW, if the trap was 
>>> hypothetically replaced by a while (1), then it would not be an 
>>> issue. is it correct? >
>>
>> No, it isn't. A infinite loop is a side effect.
> 
> I am not sure why. There are no change of state here.
> 
>>
>>> I can see two solutions:
>>>    1) Remove the ASSERT(). It is only here to make the NULL 
>>> dereference obvious in debug build. That said, the stack trace for a 
>>> NULL dereference would still be as clear.
>>
>> Removing the ASSERT just to make MISRA happy does not sound good to me.
> 
> I didn't suggest the ASSERT() just ot make MISRA happy. I suggested it 
> because it has no value here (we still have stack track if there are any 
> issue).
> 
>>
>>>    2) Replace the ASSERT with a proper check if ( !dev ) return 
>>> DEVICE_UNKONWN. AFAIU, we would not be able to add a 
>>> ASSERT_UNREACHABLE() because this would be again a perceived 
>>> side-effect.
>>>
>>
>> Replacing it with a proper check can be a solution, but I still prefer 
>> to add a deviation or move the invocation outside the initializer list.
> 
> In general, I am not in favor of adding deviation if we can avoid them 
> because the code can changed and therefore either moot the deviation or 
> hide any other issue.
> 

Ok, I will proceed with option 1.

> [...]
> 
>> Yes, sorry I was looking to the wrong definition.
>> In ARM the problem is the presence of a *volatile* ASM.
>> Please take a look here:
>>
>> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set2/latest/PROJECT.ecd;/by_service/MC3R1.R13.1.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":true,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"fingerprint","inputs":[{"enabled":true,"text":"0da7f0c9aea5491eba343618f965c81f5d7aed3c"}]}}}
> 
> Ok. So the problem is the READ_SYSREG(...). Is there a way we can 
> encapsulate the call so we don't need to use your propose trick or 
> deviate at every use of 'current'?
> 

The point is that we need to move "READ_SYSREG(TPIDR_EL2)" outside
the initializer (there are no advantages in wrapping it on a function
if the function cannot be declared pure).

The proposed solution seems to me the cleanest way do to it. I do not 
see any other acceptable solutions.


> Cheers,
> 

-- 
Simone Ballarin, M.Sc.

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

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

[Simone Ballarin]

On 19/10/23 11:35, Jan Beulich wrote:
> On 19.10.2023 03:06, Stefano Stabellini wrote:
>> On Wed, 18 Oct 2023, Simone Ballarin wrote:
>>> --- a/xen/common/sched/core.c
>>> +++ b/xen/common/sched/core.c
>>> @@ -1504,6 +1504,8 @@ long vcpu_yield(void)
>>>   {
>>>       struct vcpu * v=current;
>>>       spinlock_t *lock;
>>> +    domid_t domain_id;
>>> +    int vcpu_id;
>>>   
>>>       rcu_read_lock(&sched_res_rculock);
>>>   
>>> @@ -1515,7 +1517,9 @@ long vcpu_yield(void)
>>>   
>>>       SCHED_STAT_CRANK(vcpu_yield);
>>>   
>>> -    TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
>>> +    domain_id = current->domain->domain_id;
>>> +    vcpu_id = current->vcpu_id;
>>> +    TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id);
>>
>> Also here it looks like accessing the current pointer is considered a
>> side effect. Why? This is a just a global variable that is only accessed
>> for reading.
> 
> Not exactly. It's a per-CPU variable access on Arm, but involves a
> function call on x86. Still that function call has no other side
> effects, but I guess Misra wouldn't honor this.
> 
> I'm afraid though that the suggested change violates rule 2.2, as
> the two new assignments are dead code when !CONFIG_TRACEBUFFER.
> 
> Jan

I confirm that there is no problem in X86: I will simply propose a patch
adding __attribute_pure__ to get_cpu_info.

-- 
Simone Ballarin, M.Sc.

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

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

[Jan Beulich]

On 19.10.2023 13:12, Simone Ballarin wrote:
> On 19/10/23 11:35, Jan Beulich wrote:
>> On 19.10.2023 03:06, Stefano Stabellini wrote:
>>> On Wed, 18 Oct 2023, Simone Ballarin wrote:
>>>> --- a/xen/common/sched/core.c
>>>> +++ b/xen/common/sched/core.c
>>>> @@ -1504,6 +1504,8 @@ long vcpu_yield(void)
>>>>   {
>>>>       struct vcpu * v=current;
>>>>       spinlock_t *lock;
>>>> +    domid_t domain_id;
>>>> +    int vcpu_id;
>>>>   
>>>>       rcu_read_lock(&sched_res_rculock);
>>>>   
>>>> @@ -1515,7 +1517,9 @@ long vcpu_yield(void)
>>>>   
>>>>       SCHED_STAT_CRANK(vcpu_yield);
>>>>   
>>>> -    TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
>>>> +    domain_id = current->domain->domain_id;
>>>> +    vcpu_id = current->vcpu_id;
>>>> +    TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id);
>>>
>>> Also here it looks like accessing the current pointer is considered a
>>> side effect. Why? This is a just a global variable that is only accessed
>>> for reading.
>>
>> Not exactly. It's a per-CPU variable access on Arm, but involves a
>> function call on x86. Still that function call has no other side
>> effects, but I guess Misra wouldn't honor this.
>>
>> I'm afraid though that the suggested change violates rule 2.2, as
>> the two new assignments are dead code when !CONFIG_TRACEBUFFER.
> 
> I confirm that there is no problem in X86: I will simply propose a patch
> adding __attribute_pure__ to get_cpu_info.

I specifically did not suggest that, because I'm afraid the "pure" attribute
may not be used there. Besides this attribute typically being discarded for
inline functions in favor of the compiler's own judgement, it would allow
the compiler to squash multiple invocations. This might even be desirable in
most cases, but would break across a stack pointer change. (In this context
you also need to keep in mind late optimizations done by LTO.)

Jan

Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Julien Grall]

Hi,

On 19/10/2023 12:10, Simone Ballarin wrote:
> On 19/10/23 12:11, Julien Grall wrote:
>> Hi,
>>
>> On 19/10/2023 09:43, Simone Ballarin wrote:
>>> On 19/10/23 10:19, Julien Grall wrote:
>>>> Hi Simone,
>>>>
>>>> On 19/10/2023 08:34, Simone Ballarin wrote:
>>>>> On 18/10/23 17:03, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 18/10/2023 15:18, Simone Ballarin wrote:
>>>>>>> Rule 13.1: Initializer lists shall not contain persistent side 
>>>>>>> effects
>>>>>>>
>>>>>>> This patch moves expressions with side-effects into new variables 
>>>>>>> before
>>>>>>> the initializer lists.
>>>>>>
>>>>>> Looking at the code, I feel the commit message should be a bit 
>>>>>> more verbose because they are no apparent side-effects.
>>>>>>
>>>>>>>
>>>>>>> Function calls do not necessarily have side-effects, in these 
>>>>>>> cases the
>>>>>>> GCC pure or const attributes are added when possible.
>>>>>>
>>>>>> You are only adding pure in this patch.
>>>>>>
>>>>>>>
>>>>>>> No functional changes.
>>>>>>>
>>>>>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>>>>>>
>>>>>>> ---
>>>>>>> Function attributes pure and const do not need to be added as GCC
>>>>>>> attributes, they can be added using ECLAIR configurations.
>>>>>>> ---
>>>>>>>   xen/arch/arm/device.c              |  6 +++---
>>>>>>>   xen/arch/arm/guestcopy.c           | 12 ++++++++----
>>>>>>>   xen/arch/arm/include/asm/current.h |  2 +-
>>>>>>>   3 files changed, 12 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
>>>>>>> index 1f631d3274..e9be078415 100644
>>>>>>> --- a/xen/arch/arm/device.c
>>>>>>> +++ b/xen/arch/arm/device.c
>>>>>>> @@ -319,6 +319,8 @@ int handle_device(struct domain *d, struct 
>>>>>>> dt_device_node *dev, p2m_type_t p2mt,
>>>>>>>       int res;
>>>>>>>       paddr_t addr, size;
>>>>>>>       bool own_device = !dt_device_for_passthrough(dev);
>>>>>>> +    bool dev_is_hostbridge = is_pci_passthrough_enabled() &&
>>>>>>> +                             device_get_class(dev) == 
>>>>>>> DEVICE_PCI_HOSTBRIDGE;
>>>>>>
>>>>>> The commit message suggests that the code is moved because there 
>>>>>> are side-effects. But none of them should have any side-effects.
>>>>>>
>>>>>
>>>>> device_get_class contains an 'ASSERT(dev != NULL)' which is definitely
>>>>> a side-effect.
>>>>
>>>> Just to confirm my understanding, the side-effect here would be the 
>>>> fact that it traps and then panic(). IOW, if the trap was 
>>>> hypothetically replaced by a while (1), then it would not be an 
>>>> issue. is it correct? >
>>>
>>> No, it isn't. A infinite loop is a side effect.
>>
>> I am not sure why. There are no change of state here.
>>
>>>
>>>> I can see two solutions:
>>>>    1) Remove the ASSERT(). It is only here to make the NULL 
>>>> dereference obvious in debug build. That said, the stack trace for a 
>>>> NULL dereference would still be as clear.
>>>
>>> Removing the ASSERT just to make MISRA happy does not sound good to me.
>>
>> I didn't suggest the ASSERT() just ot make MISRA happy. I suggested it 
>> because it has no value here (we still have stack track if there are 
>> any issue).
>>
>>>
>>>>    2) Replace the ASSERT with a proper check if ( !dev ) return 
>>>> DEVICE_UNKONWN. AFAIU, we would not be able to add a 
>>>> ASSERT_UNREACHABLE() because this would be again a perceived 
>>>> side-effect.
>>>>
>>>
>>> Replacing it with a proper check can be a solution, but I still 
>>> prefer to add a deviation or move the invocation outside the 
>>> initializer list.
>>
>> In general, I am not in favor of adding deviation if we can avoid them 
>> because the code can changed and therefore either moot the deviation 
>> or hide any other issue.
>>
> 
> Ok, I will proceed with option 1.
> 
>> [...]
>>
>>> Yes, sorry I was looking to the wrong definition.
>>> In ARM the problem is the presence of a *volatile* ASM.
>>> Please take a look here:
>>>
>>> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set2/latest/PROJECT.ecd;/by_service/MC3R1.R13.1.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":true,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"fingerprint","inputs":[{"enabled":true,"text":"0da7f0c9aea5491eba343618f965c81f5d7aed3c"}]}}}
>>
>> Ok. So the problem is the READ_SYSREG(...). Is there a way we can 
>> encapsulate the call so we don't need to use your propose trick or 
>> deviate at every use of 'current'?
>>
> 
> The point is that we need to move "READ_SYSREG(TPIDR_EL2)" outside
> the initializer (there are no advantages in wrapping it on a function
> if the function cannot be declared pure).

I was thinking that maybe it could help to deviate.

> 
> The proposed solution seems to me the cleanest way do to it. I do not 
> see any other acceptable solutions.

I have some concern with the proposal (they are most likely matter of 
taste).

We usually use this trick when 'current' is used multiple time to save 
process (using 'current' is not cost free). But in this case, this is 
renaming without any apparent benefits.

If we wanted to go down this route, then this would likely want a 
comment. At which point we should just deviate.

I will have a think if there are something else we can do. Could we 
consider to not address it for now?

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin]

On 19/10/23 14:30, Julien Grall wrote:
> Hi,
> 
> On 19/10/2023 12:10, Simone Ballarin wrote:
>> On 19/10/23 12:11, Julien Grall wrote:
>>> Hi,
>>>
>>> On 19/10/2023 09:43, Simone Ballarin wrote:
>>>> On 19/10/23 10:19, Julien Grall wrote:
>>>>> Hi Simone,
>>>>>
>>>>> On 19/10/2023 08:34, Simone Ballarin wrote:
>>>>>> On 18/10/23 17:03, Julien Grall wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 18/10/2023 15:18, Simone Ballarin wrote:
>>>>>>>> Rule 13.1: Initializer lists shall not contain persistent side 
>>>>>>>> effects
>>>>>>>>
>>>>>>>> This patch moves expressions with side-effects into new 
>>>>>>>> variables before
>>>>>>>> the initializer lists.
>>>>>>>
>>>>>>> Looking at the code, I feel the commit message should be a bit 
>>>>>>> more verbose because they are no apparent side-effects.
>>>>>>>
>>>>>>>>
>>>>>>>> Function calls do not necessarily have side-effects, in these 
>>>>>>>> cases the
>>>>>>>> GCC pure or const attributes are added when possible.
>>>>>>>
>>>>>>> You are only adding pure in this patch.
>>>>>>>
>>>>>>>>
>>>>>>>> No functional changes.
>>>>>>>>
>>>>>>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>>>>>>>
>>>>>>>> ---
>>>>>>>> Function attributes pure and const do not need to be added as GCC
>>>>>>>> attributes, they can be added using ECLAIR configurations.
>>>>>>>> ---
>>>>>>>>   xen/arch/arm/device.c              |  6 +++---
>>>>>>>>   xen/arch/arm/guestcopy.c           | 12 ++++++++----
>>>>>>>>   xen/arch/arm/include/asm/current.h |  2 +-
>>>>>>>>   3 files changed, 12 insertions(+), 8 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
>>>>>>>> index 1f631d3274..e9be078415 100644
>>>>>>>> --- a/xen/arch/arm/device.c
>>>>>>>> +++ b/xen/arch/arm/device.c
>>>>>>>> @@ -319,6 +319,8 @@ int handle_device(struct domain *d, struct 
>>>>>>>> dt_device_node *dev, p2m_type_t p2mt,
>>>>>>>>       int res;
>>>>>>>>       paddr_t addr, size;
>>>>>>>>       bool own_device = !dt_device_for_passthrough(dev);
>>>>>>>> +    bool dev_is_hostbridge = is_pci_passthrough_enabled() &&
>>>>>>>> +                             device_get_class(dev) == 
>>>>>>>> DEVICE_PCI_HOSTBRIDGE;
>>>>>>>
>>>>>>> The commit message suggests that the code is moved because there 
>>>>>>> are side-effects. But none of them should have any side-effects.
>>>>>>>
>>>>>>
>>>>>> device_get_class contains an 'ASSERT(dev != NULL)' which is 
>>>>>> definitely
>>>>>> a side-effect.
>>>>>
>>>>> Just to confirm my understanding, the side-effect here would be the 
>>>>> fact that it traps and then panic(). IOW, if the trap was 
>>>>> hypothetically replaced by a while (1), then it would not be an 
>>>>> issue. is it correct? >
>>>>
>>>> No, it isn't. A infinite loop is a side effect.
>>>
>>> I am not sure why. There are no change of state here.
>>>
>>>>
>>>>> I can see two solutions:
>>>>>    1) Remove the ASSERT(). It is only here to make the NULL 
>>>>> dereference obvious in debug build. That said, the stack trace for 
>>>>> a NULL dereference would still be as clear.
>>>>
>>>> Removing the ASSERT just to make MISRA happy does not sound good to me.
>>>
>>> I didn't suggest the ASSERT() just ot make MISRA happy. I suggested 
>>> it because it has no value here (we still have stack track if there 
>>> are any issue).
>>>
>>>>
>>>>>    2) Replace the ASSERT with a proper check if ( !dev ) return 
>>>>> DEVICE_UNKONWN. AFAIU, we would not be able to add a 
>>>>> ASSERT_UNREACHABLE() because this would be again a perceived 
>>>>> side-effect.
>>>>>
>>>>
>>>> Replacing it with a proper check can be a solution, but I still 
>>>> prefer to add a deviation or move the invocation outside the 
>>>> initializer list.
>>>
>>> In general, I am not in favor of adding deviation if we can avoid 
>>> them because the code can changed and therefore either moot the 
>>> deviation or hide any other issue.
>>>
>>
>> Ok, I will proceed with option 1.
>>
>>> [...]
>>>
>>>> Yes, sorry I was looking to the wrong definition.
>>>> In ARM the problem is the presence of a *volatile* ASM.
>>>> Please take a look here:
>>>>
>>>> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set2/latest/PROJECT.ecd;/by_service/MC3R1.R13.1.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":true,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"fingerprint","inputs":[{"enabled":true,"text":"0da7f0c9aea5491eba343618f965c81f5d7aed3c"}]}}}
>>>
>>> Ok. So the problem is the READ_SYSREG(...). Is there a way we can 
>>> encapsulate the call so we don't need to use your propose trick or 
>>> deviate at every use of 'current'?
>>>
>>
>> The point is that we need to move "READ_SYSREG(TPIDR_EL2)" outside
>> the initializer (there are no advantages in wrapping it on a function
>> if the function cannot be declared pure).
> 
> I was thinking that maybe it could help to deviate.
> 
>>
>> The proposed solution seems to me the cleanest way do to it. I do not 
>> see any other acceptable solutions.
> 
> I have some concern with the proposal (they are most likely matter of 
> taste).
> 
> We usually use this trick when 'current' is used multiple time to save 
> process (using 'current' is not cost free). But in this case, this is 
> renaming without any apparent benefits.
> 
> If we wanted to go down this route, then this would likely want a 
> comment. At which point we should just deviate.
> 
> I will have a think if there are something else we can do. Could we 
> consider to not address it for now?
> 

I totally agree.

> Cheers,
> 

-- 
Simone Ballarin, M.Sc.

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

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

[Simone Ballarin]

On 19/10/23 13:19, Jan Beulich wrote:
> On 19.10.2023 13:12, Simone Ballarin wrote:
>> On 19/10/23 11:35, Jan Beulich wrote:
>>> On 19.10.2023 03:06, Stefano Stabellini wrote:
>>>> On Wed, 18 Oct 2023, Simone Ballarin wrote:
>>>>> --- a/xen/common/sched/core.c
>>>>> +++ b/xen/common/sched/core.c
>>>>> @@ -1504,6 +1504,8 @@ long vcpu_yield(void)
>>>>>    {
>>>>>        struct vcpu * v=current;
>>>>>        spinlock_t *lock;
>>>>> +    domid_t domain_id;
>>>>> +    int vcpu_id;
>>>>>    
>>>>>        rcu_read_lock(&sched_res_rculock);
>>>>>    
>>>>> @@ -1515,7 +1517,9 @@ long vcpu_yield(void)
>>>>>    
>>>>>        SCHED_STAT_CRANK(vcpu_yield);
>>>>>    
>>>>> -    TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
>>>>> +    domain_id = current->domain->domain_id;
>>>>> +    vcpu_id = current->vcpu_id;
>>>>> +    TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id);
>>>>
>>>> Also here it looks like accessing the current pointer is considered a
>>>> side effect. Why? This is a just a global variable that is only accessed
>>>> for reading.
>>>
>>> Not exactly. It's a per-CPU variable access on Arm, but involves a
>>> function call on x86. Still that function call has no other side
>>> effects, but I guess Misra wouldn't honor this.
>>>
>>> I'm afraid though that the suggested change violates rule 2.2, as
>>> the two new assignments are dead code when !CONFIG_TRACEBUFFER.
>>
>> I confirm that there is no problem in X86: I will simply propose a patch
>> adding __attribute_pure__ to get_cpu_info.
> 
> I specifically did not suggest that, because I'm afraid the "pure" attribute
> may not be used there. Besides this attribute typically being discarded for
> inline functions in favor of the compiler's own judgement, it would allow
> the compiler to squash multiple invocations. This might even be desirable in
> most cases, but would break across a stack pointer change. (In this context
> you also need to keep in mind late optimizations done by LTO.)
> 
> Jan
> 

Ok, in this case I will just configure ECLAIR to consider it without 
effects.

-- 
Simone Ballarin, M.Sc.

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

Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 6767 bytes --]

On Thu, 19 Oct 2023, Simone Ballarin wrote:
> On 19/10/23 14:30, Julien Grall wrote:
> > Hi,
> > 
> > On 19/10/2023 12:10, Simone Ballarin wrote:
> > > On 19/10/23 12:11, Julien Grall wrote:
> > > > Hi,
> > > > 
> > > > On 19/10/2023 09:43, Simone Ballarin wrote:
> > > > > On 19/10/23 10:19, Julien Grall wrote:
> > > > > > Hi Simone,
> > > > > > 
> > > > > > On 19/10/2023 08:34, Simone Ballarin wrote:
> > > > > > > On 18/10/23 17:03, Julien Grall wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On 18/10/2023 15:18, Simone Ballarin wrote:
> > > > > > > > > Rule 13.1: Initializer lists shall not contain persistent side
> > > > > > > > > effects
> > > > > > > > > 
> > > > > > > > > This patch moves expressions with side-effects into new
> > > > > > > > > variables before
> > > > > > > > > the initializer lists.
> > > > > > > > 
> > > > > > > > Looking at the code, I feel the commit message should be a bit
> > > > > > > > more verbose because they are no apparent side-effects.
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Function calls do not necessarily have side-effects, in these
> > > > > > > > > cases the
> > > > > > > > > GCC pure or const attributes are added when possible.
> > > > > > > > 
> > > > > > > > You are only adding pure in this patch.
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > No functional changes.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> > > > > > > > > 
> > > > > > > > > ---
> > > > > > > > > Function attributes pure and const do not need to be added as
> > > > > > > > > GCC
> > > > > > > > > attributes, they can be added using ECLAIR configurations.
> > > > > > > > > ---
> > > > > > > > >   xen/arch/arm/device.c              |  6 +++---
> > > > > > > > >   xen/arch/arm/guestcopy.c           | 12 ++++++++----
> > > > > > > > >   xen/arch/arm/include/asm/current.h |  2 +-
> > > > > > > > >   3 files changed, 12 insertions(+), 8 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> > > > > > > > > index 1f631d3274..e9be078415 100644
> > > > > > > > > --- a/xen/arch/arm/device.c
> > > > > > > > > +++ b/xen/arch/arm/device.c
> > > > > > > > > @@ -319,6 +319,8 @@ int handle_device(struct domain *d, struct
> > > > > > > > > dt_device_node *dev, p2m_type_t p2mt,
> > > > > > > > >       int res;
> > > > > > > > >       paddr_t addr, size;
> > > > > > > > >       bool own_device = !dt_device_for_passthrough(dev);
> > > > > > > > > +    bool dev_is_hostbridge = is_pci_passthrough_enabled() &&
> > > > > > > > > +                             device_get_class(dev) ==
> > > > > > > > > DEVICE_PCI_HOSTBRIDGE;
> > > > > > > > 
> > > > > > > > The commit message suggests that the code is moved because there
> > > > > > > > are side-effects. But none of them should have any side-effects.
> > > > > > > > 
> > > > > > > 
> > > > > > > device_get_class contains an 'ASSERT(dev != NULL)' which is
> > > > > > > definitely
> > > > > > > a side-effect.
> > > > > > 
> > > > > > Just to confirm my understanding, the side-effect here would be the
> > > > > > fact that it traps and then panic(). IOW, if the trap was
> > > > > > hypothetically replaced by a while (1), then it would not be an
> > > > > > issue. is it correct? >
> > > > > 
> > > > > No, it isn't. A infinite loop is a side effect.
> > > > 
> > > > I am not sure why. There are no change of state here.
> > > > 
> > > > > 
> > > > > > I can see two solutions:
> > > > > >    1) Remove the ASSERT(). It is only here to make the NULL
> > > > > > dereference obvious in debug build. That said, the stack trace for a
> > > > > > NULL dereference would still be as clear.
> > > > > 
> > > > > Removing the ASSERT just to make MISRA happy does not sound good to
> > > > > me.
> > > > 
> > > > I didn't suggest the ASSERT() just ot make MISRA happy. I suggested it
> > > > because it has no value here (we still have stack track if there are any
> > > > issue).
> > > > 
> > > > > 
> > > > > >    2) Replace the ASSERT with a proper check if ( !dev ) return
> > > > > > DEVICE_UNKONWN. AFAIU, we would not be able to add a
> > > > > > ASSERT_UNREACHABLE() because this would be again a perceived
> > > > > > side-effect.
> > > > > > 
> > > > > 
> > > > > Replacing it with a proper check can be a solution, but I still prefer
> > > > > to add a deviation or move the invocation outside the initializer
> > > > > list.
> > > > 
> > > > In general, I am not in favor of adding deviation if we can avoid them
> > > > because the code can changed and therefore either moot the deviation or
> > > > hide any other issue.
> > > > 
> > > 
> > > Ok, I will proceed with option 1.
> > > 
> > > > [...]
> > > > 
> > > > > Yes, sorry I was looking to the wrong definition.
> > > > > In ARM the problem is the presence of a *volatile* ASM.
> > > > > Please take a look here:
> > > > > 
> > > > > https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set2/latest/PROJECT.ecd;/by_service/MC3R1.R13.1.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":true,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"fingerprint","inputs":[{"enabled":true,"text":"0da7f0c9aea5491eba343618f965c81f5d7aed3c"}]}}}
> > > > 
> > > > Ok. So the problem is the READ_SYSREG(...). Is there a way we can
> > > > encapsulate the call so we don't need to use your propose trick or
> > > > deviate at every use of 'current'?
> > > > 
> > > 
> > > The point is that we need to move "READ_SYSREG(TPIDR_EL2)" outside
> > > the initializer (there are no advantages in wrapping it on a function
> > > if the function cannot be declared pure).
> > 
> > I was thinking that maybe it could help to deviate.
> > 
> > > 
> > > The proposed solution seems to me the cleanest way do to it. I do not see
> > > any other acceptable solutions.
> > 
> > I have some concern with the proposal (they are most likely matter of
> > taste).
> > 
> > We usually use this trick when 'current' is used multiple time to save
> > process (using 'current' is not cost free). But in this case, this is
> > renaming without any apparent benefits.
> > 
> > If we wanted to go down this route, then this would likely want a comment.
> > At which point we should just deviate.
> > 
> > I will have a think if there are something else we can do. Could we consider
> > to not address it for now?
> > 
> 
> I totally agree.

I am wondering if we could deviate "current" because even with the
implementation using volatile we know it doesn't have "side effects" in
the sense of changing things for other code outside the function.

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

[Stefano Stabellini]

On Thu, 19 Oct 2023, Simone Ballarin wrote:
> On 19/10/23 13:19, Jan Beulich wrote:
> > On 19.10.2023 13:12, Simone Ballarin wrote:
> > > On 19/10/23 11:35, Jan Beulich wrote:
> > > > On 19.10.2023 03:06, Stefano Stabellini wrote:
> > > > > On Wed, 18 Oct 2023, Simone Ballarin wrote:
> > > > > > --- a/xen/common/sched/core.c
> > > > > > +++ b/xen/common/sched/core.c
> > > > > > @@ -1504,6 +1504,8 @@ long vcpu_yield(void)
> > > > > >    {
> > > > > >        struct vcpu * v=current;
> > > > > >        spinlock_t *lock;
> > > > > > +    domid_t domain_id;
> > > > > > +    int vcpu_id;
> > > > > >           rcu_read_lock(&sched_res_rculock);
> > > > > >    @@ -1515,7 +1517,9 @@ long vcpu_yield(void)
> > > > > >           SCHED_STAT_CRANK(vcpu_yield);
> > > > > >    -    TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id,
> > > > > > current->vcpu_id);
> > > > > > +    domain_id = current->domain->domain_id;
> > > > > > +    vcpu_id = current->vcpu_id;
> > > > > > +    TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id);
> > > > > 
> > > > > Also here it looks like accessing the current pointer is considered a
> > > > > side effect. Why? This is a just a global variable that is only
> > > > > accessed
> > > > > for reading.
> > > > 
> > > > Not exactly. It's a per-CPU variable access on Arm, but involves a
> > > > function call on x86. Still that function call has no other side
> > > > effects, but I guess Misra wouldn't honor this.
> > > > 
> > > > I'm afraid though that the suggested change violates rule 2.2, as
> > > > the two new assignments are dead code when !CONFIG_TRACEBUFFER.
> > > 
> > > I confirm that there is no problem in X86: I will simply propose a patch
> > > adding __attribute_pure__ to get_cpu_info.
> > 
> > I specifically did not suggest that, because I'm afraid the "pure" attribute
> > may not be used there. Besides this attribute typically being discarded for
> > inline functions in favor of the compiler's own judgement, it would allow
> > the compiler to squash multiple invocations. This might even be desirable in
> > most cases, but would break across a stack pointer change. (In this context
> > you also need to keep in mind late optimizations done by LTO.)
> > 
> > Jan
> > 
> 
> Ok, in this case I will just configure ECLAIR to consider it without effects.

I think that's fine, just remember to either use SAF or document under
docs/misra/deviations.rst

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

[Stefano Stabellini]

On Thu, 19 Oct 2023, Simone Ballarin wrote:
> On 19/10/23 03:06, Stefano Stabellini wrote:
> > On Wed, 18 Oct 2023, Simone Ballarin wrote:
> > > Rule 13.1: Initializer lists shall not contain persistent side effects
> > > 
> > > This patch moves expressions with side-effects outside the initializer
> > > lists.
> > > 
> > > No functional changes.
> > > 
> > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> > > ---
> > >   xen/common/sched/core.c    | 16 ++++++++++++----
> > >   xen/drivers/char/ns16550.c |  4 +++-
> > >   2 files changed, 15 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> > > index 12deefa745..519884f989 100644
> > > --- a/xen/common/sched/core.c
> > > +++ b/xen/common/sched/core.c
> > > @@ -1504,6 +1504,8 @@ long vcpu_yield(void)
> > >   {
> > >       struct vcpu * v=current;
> > >       spinlock_t *lock;
> > > +    domid_t domain_id;
> > > +    int vcpu_id;
> > >         rcu_read_lock(&sched_res_rculock);
> > >   @@ -1515,7 +1517,9 @@ long vcpu_yield(void)
> > >         SCHED_STAT_CRANK(vcpu_yield);
> > >   -    TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id,
> > > current->vcpu_id);
> > > +    domain_id = current->domain->domain_id;
> > > +    vcpu_id = current->vcpu_id;
> > > +    TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id);
> > 
> > Also here it looks like accessing the current pointer is considered a
> > side effect. Why? This is a just a global variable that is only accessed
> > for reading.
> > 
> > 
> > >       raise_softirq(SCHEDULE_SOFTIRQ);
> > >       return 0;
> > >   }
> > > @@ -1888,14 +1892,17 @@ ret_t do_sched_op(int cmd,
> > > XEN_GUEST_HANDLE_PARAM(void) arg)
> > >       case SCHEDOP_shutdown:
> > >       {
> > >           struct sched_shutdown sched_shutdown;
> > > +        domid_t domain_id;
> > > +        int vcpu_id;
> > >             ret = -EFAULT;
> > >           if ( copy_from_guest(&sched_shutdown, arg, 1) )
> > >               break;
> > >   +        domain_id = current->domain->domain_id;
> > > +        vcpu_id = current->vcpu_id;
> > >           TRACE_3D(TRC_SCHED_SHUTDOWN,
> > > -                 current->domain->domain_id, current->vcpu_id,
> > > -                 sched_shutdown.reason);
> > > +                 domain_id, vcpu_id, sched_shutdown.reason);
> > >           ret = domain_shutdown(current->domain,
> > > (u8)sched_shutdown.reason);
> > >             break;
> > > @@ -1905,13 +1912,14 @@ ret_t do_sched_op(int cmd,
> > > XEN_GUEST_HANDLE_PARAM(void) arg)
> > >       {
> > >           struct sched_shutdown sched_shutdown;
> > >           struct domain *d = current->domain;
> > > +        int vcpu_id = current->vcpu_id;
> > >             ret = -EFAULT;
> > >           if ( copy_from_guest(&sched_shutdown, arg, 1) )
> > >               break;
> > >             TRACE_3D(TRC_SCHED_SHUTDOWN_CODE,
> > > -                 d->domain_id, current->vcpu_id, sched_shutdown.reason);
> > > +                 d->domain_id, vcpu_id, sched_shutdown.reason);
> > >             spin_lock(&d->shutdown_lock);
> > >           if ( d->shutdown_code == SHUTDOWN_CODE_INVALID )
> > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> > > index 28ddedd50d..0b3d8b2a30 100644
> > > --- a/xen/drivers/char/ns16550.c
> > > +++ b/xen/drivers/char/ns16550.c
> > > @@ -445,10 +445,12 @@ 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
> > >               };
> > >   +            rc = uart->irq;
> > 
> > What's the side effect here? If the side effect is rc = uart->irq, why
> > is it considered a side effect?
> > 
> 
> Yes, rc = uart->irq is the side-effect: it writes a variable
> declared outside the context of the expression.
> 
> Consider the following example:
> 
> int rc;
> 
> struct S s = {
>   .x = rc = 2,
>   .y = rc = 3
> };
> 
> What's the value of rc?

OK thanks for the explanation.

Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Julien Grall]

Hi Stefano,

On 19/10/2023 19:30, Stefano Stabellini wrote:
>>> We usually use this trick when 'current' is used multiple time to save
>>> process (using 'current' is not cost free). But in this case, this is
>>> renaming without any apparent benefits.
>>>
>>> If we wanted to go down this route, then this would likely want a comment.
>>> At which point we should just deviate.
>>>
>>> I will have a think if there are something else we can do. Could we consider
>>> to not address it for now?
>>>
>>
>> I totally agree.
> 
> I am wondering if we could deviate "current" because even with the
> implementation using volatile we know it doesn't have "side effects" in
> the sense of changing things for other code outside the function.

I will let Simone to confirm whether it is possible to do it from a 
technical point of view.

Leaving the technical part aside, is the only violations are the one in 
this patch? If so, I don't think it makes sense to deviate 'current' 
globally. It would be better to have local deviations.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Jan Beulich]

On 18.10.2023 16:18, Simone Ballarin wrote:
> Add const and pure attributes to address reports
> of MISRA C:2012 Rule 13.1: Initializer lists shall
> not contain persistent side effects
> 
> Add pure attribute to function pdx_to_pfn.
> Add const attribute to functions generated by TYPE_SAFE.
> 
> These functions are used in initializer lists: adding
> the attributes ensures that no effect will be performed
> by them.

Adding the attribute does, according to my understanding, ensure nothing.
The compiler may (but isn't required to) diagnose wrong uses of the
attributes, but it may also make use of the attributes (on the
declaration) without regard to the attribute potentially being wrongly
applied. Since further for inline functions the compiler commonly infers
attributes from the expanded code (discarding the attribute), the only
thing achieved here is a documentation aspect, I think.

> --- a/xen/include/xen/pdx.h
> +++ b/xen/include/xen/pdx.h
> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
>   * @param pdx Page index
>   * @return Obtained pfn after decompressing the pdx
>   */
> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
>  {
>      return (pdx & pfn_pdx_bottom_mask) |
>             ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);

Taking this as an example for what I've said above: The compiler can't
know that the globals used by the functions won't change value. Even
within Xen it is only by convention that these variables are assigned
their values during boot, and then aren't changed anymore. Which makes
me wonder: Did you check carefully that around the time the variables
have their values established, no calls to the functions exist (which
might then be subject to folding)?

Additionally - what about the sibling function pfn_to_pdx()?

Jan

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]

On 23/10/2023 2:34 pm, Jan Beulich wrote:
> On 18.10.2023 16:18, Simone Ballarin wrote:
>> --- a/xen/include/xen/pdx.h
>> +++ b/xen/include/xen/pdx.h
>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
>>   * @param pdx Page index
>>   * @return Obtained pfn after decompressing the pdx
>>   */
>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
>>  {
>>      return (pdx & pfn_pdx_bottom_mask) |
>>             ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
> Taking this as an example for what I've said above: The compiler can't
> know that the globals used by the functions won't change value. Even
> within Xen it is only by convention that these variables are assigned
> their values during boot, and then aren't changed anymore. Which makes
> me wonder: Did you check carefully that around the time the variables
> have their values established, no calls to the functions exist (which
> might then be subject to folding)?

I was actually going to point this out, but hadn't found the words.

pdx_to_pfn() is not pure.  It violates the requirements for being
declared pure, in a way that the compiler can see.

Right now, this will cause GCC to ignore the attribute, but who's to say
that future GCCs don't start emitting a diagnostic (in which case we'd
have to delete them to make them compile), or start honouring them (at
which point this logic will start to malfunction around the boot time
modification to the masks).


It is undefined behaviour to intentionally lie to the compiler using
attributes.  This is intentionally introducing undefined behaviour to
placate Eclair.

So why are we bending over backwards to remove UB in other areas, but
deliberately introducing here?  How does that conform with the spirit of
MISRA?

~Andrew

[-- Attachment #2: Type: text/html, Size: 2526 bytes --]

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

[Jan Beulich]

On 18.10.2023 16:18, Simone Ballarin wrote:
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -1504,6 +1504,8 @@ long vcpu_yield(void)
>  {
>      struct vcpu * v=current;
>      spinlock_t *lock;
> +    domid_t domain_id;
> +    int vcpu_id;

While I realize that the field this variable is initialized from is
plain "int", I still think that being wrong means the new variables
here and below want to be "unsigned int".

> @@ -1515,7 +1517,9 @@ long vcpu_yield(void)
>  
>      SCHED_STAT_CRANK(vcpu_yield);
>  
> -    TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
> +    domain_id = current->domain->domain_id;
> +    vcpu_id = current->vcpu_id;
> +    TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id);

If you touch this, I think you ought to also switch to using "v" in
place of "current" (making the supposed side effect go away aiui).

Yet then (for the further changes to this file) - what persistent
side effect does reading "current" have? Clarification of that is
part of the justification for this change, and hence ought to be
spelled out in the description.

> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -445,10 +445,12 @@ 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
>              };
>  
> +            rc = uart->irq;
> +
>              if ( rc > 0 )

If this needs transforming, I think we'd better switch it to

            rc = 0;

            if ( uart->irq > 0 )

thus matching what we have elsewhere in the function.

Jan

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Jan Beulich]

On 23.10.2023 15:51, Andrew Cooper wrote:
> On 23/10/2023 2:34 pm, Jan Beulich wrote:
>> On 18.10.2023 16:18, Simone Ballarin wrote:
>>> --- a/xen/include/xen/pdx.h
>>> +++ b/xen/include/xen/pdx.h
>>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
>>>   * @param pdx Page index
>>>   * @return Obtained pfn after decompressing the pdx
>>>   */
>>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
>>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
>>>  {
>>>      return (pdx & pfn_pdx_bottom_mask) |
>>>             ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
>> Taking this as an example for what I've said above: The compiler can't
>> know that the globals used by the functions won't change value. Even
>> within Xen it is only by convention that these variables are assigned
>> their values during boot, and then aren't changed anymore. Which makes
>> me wonder: Did you check carefully that around the time the variables
>> have their values established, no calls to the functions exist (which
>> might then be subject to folding)?
> 
> I was actually going to point this out, but hadn't found the words.
> 
> pdx_to_pfn() is not pure.  It violates the requirements for being
> declared pure, in a way that the compiler can see.

Hmm, I think you're remembering old wording in the doc. Nowadays it says
"However, functions declared with the pure attribute can safely read any
 nonvolatile objects, and modify the value of objects in a way that does
 not affect their return value or the observable state of the program."

To me this reads as if reads of globals are okay, constrained by the
generally single-threaded view a compiler takes.

Irrespective I agree that without said further checking (and perhaps
even some way of making sure the requirements can't be violated by
future changes), ...

> Right now, this will cause GCC to ignore the attribute, but who's to say
> that future GCCs don't start emitting a diagnostic (in which case we'd
> have to delete them to make them compile), or start honouring them (at
> which point this logic will start to malfunction around the boot time
> modification to the masks).
> 
> 
> It is undefined behaviour to intentionally lie to the compiler using
> attributes.  This is intentionally introducing undefined behaviour to
> placate Eclair.

... we'd effectively be lying to the compiler, putting ourselves at risk.

Jan

> So why are we bending over backwards to remove UB in other areas, but
> deliberately introducing here?  How does that conform with the spirit of
> MISRA?
> 
> ~Andrew

Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin]

On 20/10/23 10:28, Julien Grall wrote:
> Hi Stefano,
> 
> On 19/10/2023 19:30, Stefano Stabellini wrote:
>>>> We usually use this trick when 'current' is used multiple time to save
>>>> process (using 'current' is not cost free). But in this case, this is
>>>> renaming without any apparent benefits.
>>>>
>>>> If we wanted to go down this route, then this would likely want a 
>>>> comment.
>>>> At which point we should just deviate.
>>>>
>>>> I will have a think if there are something else we can do. Could we 
>>>> consider
>>>> to not address it for now?
>>>>
>>>
>>> I totally agree.
>>
>> I am wondering if we could deviate "current" because even with the
>> implementation using volatile we know it doesn't have "side effects" in
>> the sense of changing things for other code outside the function.
> 
> I will let Simone to confirm whether it is possible to do it from a 
> technical point of view.

Yes, it is possible.
> 
> Leaving the technical part aside, is the only violations are the one in 
> this patch? If so, I don't think it makes sense to deviate 'current' 
> globally. It would be better to have local deviations.
> 
> Cheers,
> 
For this specific case I agree in using a local deviation.
In the next version of the patch, I will use a SAF comment.

-- 
Simone Ballarin, M.Sc.

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

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Simone Ballarin]

On 23/10/23 15:34, Jan Beulich wrote:
> On 18.10.2023 16:18, Simone Ballarin wrote:
>> Add const and pure attributes to address reports
>> of MISRA C:2012 Rule 13.1: Initializer lists shall
>> not contain persistent side effects
>>
>> Add pure attribute to function pdx_to_pfn.
>> Add const attribute to functions generated by TYPE_SAFE.
>>
>> These functions are used in initializer lists: adding
>> the attributes ensures that no effect will be performed
>> by them.
> 
> Adding the attribute does, according to my understanding, ensure nothing
> The compiler may (but isn't required to) diagnose wrong uses of the
> attributes, but it may also make use of the attributes (on the
> declaration) without regard to the attribute potentially being wrongly
> applied. Since further for inline functions the compiler commonly infers
> attributes from the expanded code (discarding the attribute), the only
> thing achieved here is a documentation aspect, I think.

Yes, you're right. I will rephrase the commit message.


> 
>> --- a/xen/include/xen/pdx.h
>> +++ b/xen/include/xen/pdx.h
>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
>>    * @param pdx Page index
>>    * @return Obtained pfn after decompressing the pdx
>>    */
>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
>>   {
>>       return (pdx & pfn_pdx_bottom_mask) |
>>              ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
> 
> Taking this as an example for what I've said above: The compiler can't
> know that the globals used by the functions won't change value. Even
> within Xen it is only by convention that these variables are assigned
> their values during boot, and then aren't changed anymore. Which makes
> me wonder: Did you check carefully that around the time the variables
> have their values established, no calls to the functions exist (which
> might then be subject to folding)?

There is no need to check that, the GCC documentation explicitly says:

However, functions declared with the pure attribute *can safely read any 
non-volatile objects*, and modify the value of objects in a way that 
does not affect their return value or the observable state of the program.

https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

> Additionally - what about the sibling function pfn_to_pdx()?
> 
> Jan

I will add the attribute also to the sibling.

-- 
Simone Ballarin, M.Sc.

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

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Jan Beulich]

On 23.10.2023 17:23, Simone Ballarin wrote:
> On 23/10/23 15:34, Jan Beulich wrote:
>> On 18.10.2023 16:18, Simone Ballarin wrote:
>>> --- a/xen/include/xen/pdx.h
>>> +++ b/xen/include/xen/pdx.h
>>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
>>>    * @param pdx Page index
>>>    * @return Obtained pfn after decompressing the pdx
>>>    */
>>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
>>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
>>>   {
>>>       return (pdx & pfn_pdx_bottom_mask) |
>>>              ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
>>
>> Taking this as an example for what I've said above: The compiler can't
>> know that the globals used by the functions won't change value. Even
>> within Xen it is only by convention that these variables are assigned
>> their values during boot, and then aren't changed anymore. Which makes
>> me wonder: Did you check carefully that around the time the variables
>> have their values established, no calls to the functions exist (which
>> might then be subject to folding)?
> 
> There is no need to check that, the GCC documentation explicitly says:
> 
> However, functions declared with the pure attribute *can safely read any 
> non-volatile objects*, and modify the value of objects in a way that 
> does not affect their return value or the observable state of the program.

I did quote this same text in response to what Andrew has said, but I also
did note there that this needs to be taken with a grain of salt: The
compiler generally assumes a single-threaded environment, i.e. no changes
to globals behind the back of the code it is processing.

Jan

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4746 bytes --]

On Mon, 23 Oct 2023, Jan Beulich wrote:
> On 23.10.2023 17:23, Simone Ballarin wrote:
> > On 23/10/23 15:34, Jan Beulich wrote:
> >> On 18.10.2023 16:18, Simone Ballarin wrote:
> >>> --- a/xen/include/xen/pdx.h
> >>> +++ b/xen/include/xen/pdx.h
> >>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
> >>>    * @param pdx Page index
> >>>    * @return Obtained pfn after decompressing the pdx
> >>>    */
> >>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
> >>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
> >>>   {
> >>>       return (pdx & pfn_pdx_bottom_mask) |
> >>>              ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
> >>
> >> Taking this as an example for what I've said above: The compiler can't
> >> know that the globals used by the functions won't change value. Even
> >> within Xen it is only by convention that these variables are assigned
> >> their values during boot, and then aren't changed anymore. Which makes
> >> me wonder: Did you check carefully that around the time the variables
> >> have their values established, no calls to the functions exist (which
> >> might then be subject to folding)?
> > 
> > There is no need to check that, the GCC documentation explicitly says:
> > 
> > However, functions declared with the pure attribute *can safely read any 
> > non-volatile objects*, and modify the value of objects in a way that 
> > does not affect their return value or the observable state of the program.
> 
> I did quote this same text in response to what Andrew has said, but I also
> did note there that this needs to be taken with a grain of salt: The
> compiler generally assumes a single-threaded environment, i.e. no changes
> to globals behind the back of the code it is processing.

Let's start from the beginning. The reason for Simone to add
__attribute_pure__ to pdx_to_pfn and other functions is for
documentation purposes. It is OK if it doesn't serve any purpose other
than documentation.

Andrew, for sure we do not want to lie to the compiler and introduce
undefined behavior. If we think there is a risk of it, we should not do
it.

So, what do we want to document? We want to document that the function
does not have side effects according to MISRA's definition of it, which
might subtly differ from GCC's definition.

Looking at GCC's definition of __attribute_pure__, with the
clarification statement copy/pasted above by both Simone and Jan, it
seems that __attribute_pure__ matches MISRA's definition of a function
without side effects. It also seems that pdx_to_pfn abides to that
definition.

Jan has a point that GCC might be making other assumptions
(single-thread execution) that might not hold true in our case. Given
the way the GCC statement is written I think this is low risk. But maybe
not all GCC versions we want to support in the project might have the
same definition of __attribute_pure__. So we could end up using
__attribute_pure__ correctly for the GCC version used for safety (GCC
12.1, see docs/misra/C-language-toolchain.rst) but it might actually
break an older GCC version.


So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
Clang version might interpret __attribute_pure__ differently and
potentially misbehave.

Option#2 is to avoid this risk, by not using __attribute_pure__.
Instead, we can use SAF-xx-safe or deviations.rst to document that
pdx_to_pfn and other functions like it are without side effects
according to MISRA's definition.


Both options have pros and cons. To me the most important factor is how
many GCC versions come with the statement "pure attribute can safely
read any non-volatile objects, and modify the value of objects in a way
that does not affect their return value or the observable state of the
program".

I checked and these are the results:
- gcc 4.0.2: no statement
- gcc 5.1.0: no statement
- gcc 6.1.0: no statement
- gcc 7.1.0: no statement
- gcc 8.1.0: alternative statement "The pure attribute imposes similar
  but looser restrictions on a function’s definition than the const
  attribute: it allows the function to read global variables."
- gcc 9.1.0: yes statement


So based on the above, __attribute_pure__ comes with its current
definition only from gcc 9 onward. I don't know if as a Xen community we
clearly declare a range of supported compilers, but I would imagine we
would still want to support gcc versions older than 9? (Not to mention
clang, which I haven't checked.)

It doesn't seem to me that __attribute_pure__ could be correctly used on
pdx_to_pfn with GCC 7.1.0 for example.

So in conclusion, I think it is better to avoid __attribute_pure__ and
use SAF-xx-safe or an alternative approach instead.

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Stefano Stabellini]

> It doesn't seem to me that __attribute_pure__ could be correctly used on
> pdx_to_pfn with GCC 7.1.0 for example.
> 
> So in conclusion, I think it is better to avoid __attribute_pure__ and
> use SAF-xx-safe or an alternative approach instead.

I was thinking that another option would be to introduce a macro "pure"
that is defined as __attribute_pure__ for GCC 9 or later and defined to
nothing for GCC 8 or older.

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Jan Beulich]

On 24.10.2023 00:05, Stefano Stabellini wrote:
> On Mon, 23 Oct 2023, Jan Beulich wrote:
>> On 23.10.2023 17:23, Simone Ballarin wrote:
>>> On 23/10/23 15:34, Jan Beulich wrote:
>>>> On 18.10.2023 16:18, Simone Ballarin wrote:
>>>>> --- a/xen/include/xen/pdx.h
>>>>> +++ b/xen/include/xen/pdx.h
>>>>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
>>>>>    * @param pdx Page index
>>>>>    * @return Obtained pfn after decompressing the pdx
>>>>>    */
>>>>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
>>>>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
>>>>>   {
>>>>>       return (pdx & pfn_pdx_bottom_mask) |
>>>>>              ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
>>>>
>>>> Taking this as an example for what I've said above: The compiler can't
>>>> know that the globals used by the functions won't change value. Even
>>>> within Xen it is only by convention that these variables are assigned
>>>> their values during boot, and then aren't changed anymore. Which makes
>>>> me wonder: Did you check carefully that around the time the variables
>>>> have their values established, no calls to the functions exist (which
>>>> might then be subject to folding)?
>>>
>>> There is no need to check that, the GCC documentation explicitly says:
>>>
>>> However, functions declared with the pure attribute *can safely read any 
>>> non-volatile objects*, and modify the value of objects in a way that 
>>> does not affect their return value or the observable state of the program.
>>
>> I did quote this same text in response to what Andrew has said, but I also
>> did note there that this needs to be taken with a grain of salt: The
>> compiler generally assumes a single-threaded environment, i.e. no changes
>> to globals behind the back of the code it is processing.
> 
> Let's start from the beginning. The reason for Simone to add
> __attribute_pure__ to pdx_to_pfn and other functions is for
> documentation purposes. It is OK if it doesn't serve any purpose other
> than documentation.
> 
> Andrew, for sure we do not want to lie to the compiler and introduce
> undefined behavior. If we think there is a risk of it, we should not do
> it.
> 
> So, what do we want to document? We want to document that the function
> does not have side effects according to MISRA's definition of it, which
> might subtly differ from GCC's definition.
> 
> Looking at GCC's definition of __attribute_pure__, with the
> clarification statement copy/pasted above by both Simone and Jan, it
> seems that __attribute_pure__ matches MISRA's definition of a function
> without side effects. It also seems that pdx_to_pfn abides to that
> definition.
> 
> Jan has a point that GCC might be making other assumptions
> (single-thread execution) that might not hold true in our case. Given
> the way the GCC statement is written I think this is low risk. But maybe
> not all GCC versions we want to support in the project might have the
> same definition of __attribute_pure__. So we could end up using
> __attribute_pure__ correctly for the GCC version used for safety (GCC
> 12.1, see docs/misra/C-language-toolchain.rst) but it might actually
> break an older GCC version.
> 
> 
> So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
> Clang version might interpret __attribute_pure__ differently and
> potentially misbehave.
> 
> Option#2 is to avoid this risk, by not using __attribute_pure__.
> Instead, we can use SAF-xx-safe or deviations.rst to document that
> pdx_to_pfn and other functions like it are without side effects
> according to MISRA's definition.
> 
> 
> Both options have pros and cons. To me the most important factor is how
> many GCC versions come with the statement "pure attribute can safely
> read any non-volatile objects, and modify the value of objects in a way
> that does not affect their return value or the observable state of the
> program".
> 
> I checked and these are the results:
> - gcc 4.0.2: no statement
> - gcc 5.1.0: no statement
> - gcc 6.1.0: no statement
> - gcc 7.1.0: no statement
> - gcc 8.1.0: alternative statement "The pure attribute imposes similar
>   but looser restrictions on a function’s definition than the const
>   attribute: it allows the function to read global variables."
> - gcc 9.1.0: yes statement
> 
> 
> So based on the above, __attribute_pure__ comes with its current
> definition only from gcc 9 onward. I don't know if as a Xen community we
> clearly declare a range of supported compilers, but I would imagine we
> would still want to support gcc versions older than 9? (Not to mention
> clang, which I haven't checked.)
> 
> It doesn't seem to me that __attribute_pure__ could be correctly used on
> pdx_to_pfn with GCC 7.1.0 for example.

The absence of documentation doesn't mean the attribute had different
(or even undefined) meaning in earlier versions. Instead it means one
would need to consult other places (source code?) to figure out whether
there was any behavioral difference (I don't think there was).

That said, ...

> So in conclusion, I think it is better to avoid __attribute_pure__ and
> use SAF-xx-safe or an alternative approach instead.

... I agree here. We just don't want to take chances.

Jan

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5760 bytes --]

On Tue, 24 Oct 2023, Jan Beulich wrote:
> On 24.10.2023 00:05, Stefano Stabellini wrote:
> > On Mon, 23 Oct 2023, Jan Beulich wrote:
> >> On 23.10.2023 17:23, Simone Ballarin wrote:
> >>> On 23/10/23 15:34, Jan Beulich wrote:
> >>>> On 18.10.2023 16:18, Simone Ballarin wrote:
> >>>>> --- a/xen/include/xen/pdx.h
> >>>>> +++ b/xen/include/xen/pdx.h
> >>>>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
> >>>>>    * @param pdx Page index
> >>>>>    * @return Obtained pfn after decompressing the pdx
> >>>>>    */
> >>>>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
> >>>>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
> >>>>>   {
> >>>>>       return (pdx & pfn_pdx_bottom_mask) |
> >>>>>              ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
> >>>>
> >>>> Taking this as an example for what I've said above: The compiler can't
> >>>> know that the globals used by the functions won't change value. Even
> >>>> within Xen it is only by convention that these variables are assigned
> >>>> their values during boot, and then aren't changed anymore. Which makes
> >>>> me wonder: Did you check carefully that around the time the variables
> >>>> have their values established, no calls to the functions exist (which
> >>>> might then be subject to folding)?
> >>>
> >>> There is no need to check that, the GCC documentation explicitly says:
> >>>
> >>> However, functions declared with the pure attribute *can safely read any 
> >>> non-volatile objects*, and modify the value of objects in a way that 
> >>> does not affect their return value or the observable state of the program.
> >>
> >> I did quote this same text in response to what Andrew has said, but I also
> >> did note there that this needs to be taken with a grain of salt: The
> >> compiler generally assumes a single-threaded environment, i.e. no changes
> >> to globals behind the back of the code it is processing.
> > 
> > Let's start from the beginning. The reason for Simone to add
> > __attribute_pure__ to pdx_to_pfn and other functions is for
> > documentation purposes. It is OK if it doesn't serve any purpose other
> > than documentation.
> > 
> > Andrew, for sure we do not want to lie to the compiler and introduce
> > undefined behavior. If we think there is a risk of it, we should not do
> > it.
> > 
> > So, what do we want to document? We want to document that the function
> > does not have side effects according to MISRA's definition of it, which
> > might subtly differ from GCC's definition.
> > 
> > Looking at GCC's definition of __attribute_pure__, with the
> > clarification statement copy/pasted above by both Simone and Jan, it
> > seems that __attribute_pure__ matches MISRA's definition of a function
> > without side effects. It also seems that pdx_to_pfn abides to that
> > definition.
> > 
> > Jan has a point that GCC might be making other assumptions
> > (single-thread execution) that might not hold true in our case. Given
> > the way the GCC statement is written I think this is low risk. But maybe
> > not all GCC versions we want to support in the project might have the
> > same definition of __attribute_pure__. So we could end up using
> > __attribute_pure__ correctly for the GCC version used for safety (GCC
> > 12.1, see docs/misra/C-language-toolchain.rst) but it might actually
> > break an older GCC version.
> > 
> > 
> > So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
> > Clang version might interpret __attribute_pure__ differently and
> > potentially misbehave.
> > 
> > Option#2 is to avoid this risk, by not using __attribute_pure__.
> > Instead, we can use SAF-xx-safe or deviations.rst to document that
> > pdx_to_pfn and other functions like it are without side effects
> > according to MISRA's definition.
> > 
> > 
> > Both options have pros and cons. To me the most important factor is how
> > many GCC versions come with the statement "pure attribute can safely
> > read any non-volatile objects, and modify the value of objects in a way
> > that does not affect their return value or the observable state of the
> > program".
> > 
> > I checked and these are the results:
> > - gcc 4.0.2: no statement
> > - gcc 5.1.0: no statement
> > - gcc 6.1.0: no statement
> > - gcc 7.1.0: no statement
> > - gcc 8.1.0: alternative statement "The pure attribute imposes similar
> >   but looser restrictions on a function’s definition than the const
> >   attribute: it allows the function to read global variables."
> > - gcc 9.1.0: yes statement
> > 
> > 
> > So based on the above, __attribute_pure__ comes with its current
> > definition only from gcc 9 onward. I don't know if as a Xen community we
> > clearly declare a range of supported compilers, but I would imagine we
> > would still want to support gcc versions older than 9? (Not to mention
> > clang, which I haven't checked.)
> > 
> > It doesn't seem to me that __attribute_pure__ could be correctly used on
> > pdx_to_pfn with GCC 7.1.0 for example.
> 
> The absence of documentation doesn't mean the attribute had different
> (or even undefined) meaning in earlier versions. Instead it means one
> would need to consult other places (source code?) to figure out whether
> there was any behavioral difference (I don't think there was).
> 
> That said, ...
> 
> > So in conclusion, I think it is better to avoid __attribute_pure__ and
> > use SAF-xx-safe or an alternative approach instead.
> 
> ... I agree here. We just don't want to take chances.

Let me resurrect this thread.

Could we use something like "pure" that we #define as we want?

Depending on the compiler version or other options we could #define pure
to __attribute_pure__ or to nothing.

Opinions?

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Jan Beulich]

On 23.02.2024 02:32, Stefano Stabellini wrote:
> On Tue, 24 Oct 2023, Jan Beulich wrote:
>> On 24.10.2023 00:05, Stefano Stabellini wrote:
>>> On Mon, 23 Oct 2023, Jan Beulich wrote:
>>>> On 23.10.2023 17:23, Simone Ballarin wrote:
>>>>> On 23/10/23 15:34, Jan Beulich wrote:
>>>>>> On 18.10.2023 16:18, Simone Ballarin wrote:
>>>>>>> --- a/xen/include/xen/pdx.h
>>>>>>> +++ b/xen/include/xen/pdx.h
>>>>>>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
>>>>>>>    * @param pdx Page index
>>>>>>>    * @return Obtained pfn after decompressing the pdx
>>>>>>>    */
>>>>>>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
>>>>>>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
>>>>>>>   {
>>>>>>>       return (pdx & pfn_pdx_bottom_mask) |
>>>>>>>              ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
>>>>>>
>>>>>> Taking this as an example for what I've said above: The compiler can't
>>>>>> know that the globals used by the functions won't change value. Even
>>>>>> within Xen it is only by convention that these variables are assigned
>>>>>> their values during boot, and then aren't changed anymore. Which makes
>>>>>> me wonder: Did you check carefully that around the time the variables
>>>>>> have their values established, no calls to the functions exist (which
>>>>>> might then be subject to folding)?
>>>>>
>>>>> There is no need to check that, the GCC documentation explicitly says:
>>>>>
>>>>> However, functions declared with the pure attribute *can safely read any 
>>>>> non-volatile objects*, and modify the value of objects in a way that 
>>>>> does not affect their return value or the observable state of the program.
>>>>
>>>> I did quote this same text in response to what Andrew has said, but I also
>>>> did note there that this needs to be taken with a grain of salt: The
>>>> compiler generally assumes a single-threaded environment, i.e. no changes
>>>> to globals behind the back of the code it is processing.
>>>
>>> Let's start from the beginning. The reason for Simone to add
>>> __attribute_pure__ to pdx_to_pfn and other functions is for
>>> documentation purposes. It is OK if it doesn't serve any purpose other
>>> than documentation.
>>>
>>> Andrew, for sure we do not want to lie to the compiler and introduce
>>> undefined behavior. If we think there is a risk of it, we should not do
>>> it.
>>>
>>> So, what do we want to document? We want to document that the function
>>> does not have side effects according to MISRA's definition of it, which
>>> might subtly differ from GCC's definition.
>>>
>>> Looking at GCC's definition of __attribute_pure__, with the
>>> clarification statement copy/pasted above by both Simone and Jan, it
>>> seems that __attribute_pure__ matches MISRA's definition of a function
>>> without side effects. It also seems that pdx_to_pfn abides to that
>>> definition.
>>>
>>> Jan has a point that GCC might be making other assumptions
>>> (single-thread execution) that might not hold true in our case. Given
>>> the way the GCC statement is written I think this is low risk. But maybe
>>> not all GCC versions we want to support in the project might have the
>>> same definition of __attribute_pure__. So we could end up using
>>> __attribute_pure__ correctly for the GCC version used for safety (GCC
>>> 12.1, see docs/misra/C-language-toolchain.rst) but it might actually
>>> break an older GCC version.
>>>
>>>
>>> So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
>>> Clang version might interpret __attribute_pure__ differently and
>>> potentially misbehave.
>>>
>>> Option#2 is to avoid this risk, by not using __attribute_pure__.
>>> Instead, we can use SAF-xx-safe or deviations.rst to document that
>>> pdx_to_pfn and other functions like it are without side effects
>>> according to MISRA's definition.
>>>
>>>
>>> Both options have pros and cons. To me the most important factor is how
>>> many GCC versions come with the statement "pure attribute can safely
>>> read any non-volatile objects, and modify the value of objects in a way
>>> that does not affect their return value or the observable state of the
>>> program".
>>>
>>> I checked and these are the results:
>>> - gcc 4.0.2: no statement
>>> - gcc 5.1.0: no statement
>>> - gcc 6.1.0: no statement
>>> - gcc 7.1.0: no statement
>>> - gcc 8.1.0: alternative statement "The pure attribute imposes similar
>>>   but looser restrictions on a function’s definition than the const
>>>   attribute: it allows the function to read global variables."
>>> - gcc 9.1.0: yes statement
>>>
>>>
>>> So based on the above, __attribute_pure__ comes with its current
>>> definition only from gcc 9 onward. I don't know if as a Xen community we
>>> clearly declare a range of supported compilers, but I would imagine we
>>> would still want to support gcc versions older than 9? (Not to mention
>>> clang, which I haven't checked.)
>>>
>>> It doesn't seem to me that __attribute_pure__ could be correctly used on
>>> pdx_to_pfn with GCC 7.1.0 for example.
>>
>> The absence of documentation doesn't mean the attribute had different
>> (or even undefined) meaning in earlier versions. Instead it means one
>> would need to consult other places (source code?) to figure out whether
>> there was any behavioral difference (I don't think there was).
>>
>> That said, ...
>>
>>> So in conclusion, I think it is better to avoid __attribute_pure__ and
>>> use SAF-xx-safe or an alternative approach instead.
>>
>> ... I agree here. We just don't want to take chances.
> 
> Let me resurrect this thread.
> 
> Could we use something like "pure" that we #define as we want?
> 
> Depending on the compiler version or other options we could #define pure
> to __attribute_pure__ or to nothing.

While we can do about anything, I don't think it's a good idea to overload
a well known term with something having somewhat different meaning. If a
differently named custom attribute helps, that might be a possible option.

Jan

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 6850 bytes --]

On Fri, 23 Feb 2024, Jan Beulich wrote:
> On 23.02.2024 02:32, Stefano Stabellini wrote:
> > On Tue, 24 Oct 2023, Jan Beulich wrote:
> >> On 24.10.2023 00:05, Stefano Stabellini wrote:
> >>> On Mon, 23 Oct 2023, Jan Beulich wrote:
> >>>> On 23.10.2023 17:23, Simone Ballarin wrote:
> >>>>> On 23/10/23 15:34, Jan Beulich wrote:
> >>>>>> On 18.10.2023 16:18, Simone Ballarin wrote:
> >>>>>>> --- a/xen/include/xen/pdx.h
> >>>>>>> +++ b/xen/include/xen/pdx.h
> >>>>>>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
> >>>>>>>    * @param pdx Page index
> >>>>>>>    * @return Obtained pfn after decompressing the pdx
> >>>>>>>    */
> >>>>>>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
> >>>>>>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
> >>>>>>>   {
> >>>>>>>       return (pdx & pfn_pdx_bottom_mask) |
> >>>>>>>              ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
> >>>>>>
> >>>>>> Taking this as an example for what I've said above: The compiler can't
> >>>>>> know that the globals used by the functions won't change value. Even
> >>>>>> within Xen it is only by convention that these variables are assigned
> >>>>>> their values during boot, and then aren't changed anymore. Which makes
> >>>>>> me wonder: Did you check carefully that around the time the variables
> >>>>>> have their values established, no calls to the functions exist (which
> >>>>>> might then be subject to folding)?
> >>>>>
> >>>>> There is no need to check that, the GCC documentation explicitly says:
> >>>>>
> >>>>> However, functions declared with the pure attribute *can safely read any 
> >>>>> non-volatile objects*, and modify the value of objects in a way that 
> >>>>> does not affect their return value or the observable state of the program.
> >>>>
> >>>> I did quote this same text in response to what Andrew has said, but I also
> >>>> did note there that this needs to be taken with a grain of salt: The
> >>>> compiler generally assumes a single-threaded environment, i.e. no changes
> >>>> to globals behind the back of the code it is processing.
> >>>
> >>> Let's start from the beginning. The reason for Simone to add
> >>> __attribute_pure__ to pdx_to_pfn and other functions is for
> >>> documentation purposes. It is OK if it doesn't serve any purpose other
> >>> than documentation.
> >>>
> >>> Andrew, for sure we do not want to lie to the compiler and introduce
> >>> undefined behavior. If we think there is a risk of it, we should not do
> >>> it.
> >>>
> >>> So, what do we want to document? We want to document that the function
> >>> does not have side effects according to MISRA's definition of it, which
> >>> might subtly differ from GCC's definition.
> >>>
> >>> Looking at GCC's definition of __attribute_pure__, with the
> >>> clarification statement copy/pasted above by both Simone and Jan, it
> >>> seems that __attribute_pure__ matches MISRA's definition of a function
> >>> without side effects. It also seems that pdx_to_pfn abides to that
> >>> definition.
> >>>
> >>> Jan has a point that GCC might be making other assumptions
> >>> (single-thread execution) that might not hold true in our case. Given
> >>> the way the GCC statement is written I think this is low risk. But maybe
> >>> not all GCC versions we want to support in the project might have the
> >>> same definition of __attribute_pure__. So we could end up using
> >>> __attribute_pure__ correctly for the GCC version used for safety (GCC
> >>> 12.1, see docs/misra/C-language-toolchain.rst) but it might actually
> >>> break an older GCC version.
> >>>
> >>>
> >>> So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
> >>> Clang version might interpret __attribute_pure__ differently and
> >>> potentially misbehave.
> >>>
> >>> Option#2 is to avoid this risk, by not using __attribute_pure__.
> >>> Instead, we can use SAF-xx-safe or deviations.rst to document that
> >>> pdx_to_pfn and other functions like it are without side effects
> >>> according to MISRA's definition.
> >>>
> >>>
> >>> Both options have pros and cons. To me the most important factor is how
> >>> many GCC versions come with the statement "pure attribute can safely
> >>> read any non-volatile objects, and modify the value of objects in a way
> >>> that does not affect their return value or the observable state of the
> >>> program".
> >>>
> >>> I checked and these are the results:
> >>> - gcc 4.0.2: no statement
> >>> - gcc 5.1.0: no statement
> >>> - gcc 6.1.0: no statement
> >>> - gcc 7.1.0: no statement
> >>> - gcc 8.1.0: alternative statement "The pure attribute imposes similar
> >>>   but looser restrictions on a function’s definition than the const
> >>>   attribute: it allows the function to read global variables."
> >>> - gcc 9.1.0: yes statement
> >>>
> >>>
> >>> So based on the above, __attribute_pure__ comes with its current
> >>> definition only from gcc 9 onward. I don't know if as a Xen community we
> >>> clearly declare a range of supported compilers, but I would imagine we
> >>> would still want to support gcc versions older than 9? (Not to mention
> >>> clang, which I haven't checked.)
> >>>
> >>> It doesn't seem to me that __attribute_pure__ could be correctly used on
> >>> pdx_to_pfn with GCC 7.1.0 for example.
> >>
> >> The absence of documentation doesn't mean the attribute had different
> >> (or even undefined) meaning in earlier versions. Instead it means one
> >> would need to consult other places (source code?) to figure out whether
> >> there was any behavioral difference (I don't think there was).
> >>
> >> That said, ...
> >>
> >>> So in conclusion, I think it is better to avoid __attribute_pure__ and
> >>> use SAF-xx-safe or an alternative approach instead.
> >>
> >> ... I agree here. We just don't want to take chances.
> > 
> > Let me resurrect this thread.
> > 
> > Could we use something like "pure" that we #define as we want?
> > 
> > Depending on the compiler version or other options we could #define pure
> > to __attribute_pure__ or to nothing.
> 
> While we can do about anything, I don't think it's a good idea to overload
> a well known term with something having somewhat different meaning. If a
> differently named custom attribute helps, that might be a possible option.

It doesn't have a different meaning. If it had a different meaning I'd
agree with you.

The goal is for the #define to have exactly the same meaning as the gcc
definition from gcc 9 onward. However, other versions of gcc or other
compilers could have different semantics. Also we might not want to
allow gcc to perform the optimizations that it might want to do if the
attribute is passed.

So the definition would be clear and 100% aligned with the modern gcc
definition. However we would be able to control the behavior better.

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Jan Beulich]

On 23.02.2024 23:36, Stefano Stabellini wrote:
> On Fri, 23 Feb 2024, Jan Beulich wrote:
>> On 23.02.2024 02:32, Stefano Stabellini wrote:
>>> On Tue, 24 Oct 2023, Jan Beulich wrote:
>>>> On 24.10.2023 00:05, Stefano Stabellini wrote:
>>>>> On Mon, 23 Oct 2023, Jan Beulich wrote:
>>>>>> On 23.10.2023 17:23, Simone Ballarin wrote:
>>>>>>> On 23/10/23 15:34, Jan Beulich wrote:
>>>>>>>> On 18.10.2023 16:18, Simone Ballarin wrote:
>>>>>>>>> --- a/xen/include/xen/pdx.h
>>>>>>>>> +++ b/xen/include/xen/pdx.h
>>>>>>>>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
>>>>>>>>>    * @param pdx Page index
>>>>>>>>>    * @return Obtained pfn after decompressing the pdx
>>>>>>>>>    */
>>>>>>>>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
>>>>>>>>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
>>>>>>>>>   {
>>>>>>>>>       return (pdx & pfn_pdx_bottom_mask) |
>>>>>>>>>              ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
>>>>>>>>
>>>>>>>> Taking this as an example for what I've said above: The compiler can't
>>>>>>>> know that the globals used by the functions won't change value. Even
>>>>>>>> within Xen it is only by convention that these variables are assigned
>>>>>>>> their values during boot, and then aren't changed anymore. Which makes
>>>>>>>> me wonder: Did you check carefully that around the time the variables
>>>>>>>> have their values established, no calls to the functions exist (which
>>>>>>>> might then be subject to folding)?
>>>>>>>
>>>>>>> There is no need to check that, the GCC documentation explicitly says:
>>>>>>>
>>>>>>> However, functions declared with the pure attribute *can safely read any 
>>>>>>> non-volatile objects*, and modify the value of objects in a way that 
>>>>>>> does not affect their return value or the observable state of the program.
>>>>>>
>>>>>> I did quote this same text in response to what Andrew has said, but I also
>>>>>> did note there that this needs to be taken with a grain of salt: The
>>>>>> compiler generally assumes a single-threaded environment, i.e. no changes
>>>>>> to globals behind the back of the code it is processing.
>>>>>
>>>>> Let's start from the beginning. The reason for Simone to add
>>>>> __attribute_pure__ to pdx_to_pfn and other functions is for
>>>>> documentation purposes. It is OK if it doesn't serve any purpose other
>>>>> than documentation.
>>>>>
>>>>> Andrew, for sure we do not want to lie to the compiler and introduce
>>>>> undefined behavior. If we think there is a risk of it, we should not do
>>>>> it.
>>>>>
>>>>> So, what do we want to document? We want to document that the function
>>>>> does not have side effects according to MISRA's definition of it, which
>>>>> might subtly differ from GCC's definition.
>>>>>
>>>>> Looking at GCC's definition of __attribute_pure__, with the
>>>>> clarification statement copy/pasted above by both Simone and Jan, it
>>>>> seems that __attribute_pure__ matches MISRA's definition of a function
>>>>> without side effects. It also seems that pdx_to_pfn abides to that
>>>>> definition.
>>>>>
>>>>> Jan has a point that GCC might be making other assumptions
>>>>> (single-thread execution) that might not hold true in our case. Given
>>>>> the way the GCC statement is written I think this is low risk. But maybe
>>>>> not all GCC versions we want to support in the project might have the
>>>>> same definition of __attribute_pure__. So we could end up using
>>>>> __attribute_pure__ correctly for the GCC version used for safety (GCC
>>>>> 12.1, see docs/misra/C-language-toolchain.rst) but it might actually
>>>>> break an older GCC version.
>>>>>
>>>>>
>>>>> So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
>>>>> Clang version might interpret __attribute_pure__ differently and
>>>>> potentially misbehave.
>>>>>
>>>>> Option#2 is to avoid this risk, by not using __attribute_pure__.
>>>>> Instead, we can use SAF-xx-safe or deviations.rst to document that
>>>>> pdx_to_pfn and other functions like it are without side effects
>>>>> according to MISRA's definition.
>>>>>
>>>>>
>>>>> Both options have pros and cons. To me the most important factor is how
>>>>> many GCC versions come with the statement "pure attribute can safely
>>>>> read any non-volatile objects, and modify the value of objects in a way
>>>>> that does not affect their return value or the observable state of the
>>>>> program".
>>>>>
>>>>> I checked and these are the results:
>>>>> - gcc 4.0.2: no statement
>>>>> - gcc 5.1.0: no statement
>>>>> - gcc 6.1.0: no statement
>>>>> - gcc 7.1.0: no statement
>>>>> - gcc 8.1.0: alternative statement "The pure attribute imposes similar
>>>>>   but looser restrictions on a function’s definition than the const
>>>>>   attribute: it allows the function to read global variables."
>>>>> - gcc 9.1.0: yes statement
>>>>>
>>>>>
>>>>> So based on the above, __attribute_pure__ comes with its current
>>>>> definition only from gcc 9 onward. I don't know if as a Xen community we
>>>>> clearly declare a range of supported compilers, but I would imagine we
>>>>> would still want to support gcc versions older than 9? (Not to mention
>>>>> clang, which I haven't checked.)
>>>>>
>>>>> It doesn't seem to me that __attribute_pure__ could be correctly used on
>>>>> pdx_to_pfn with GCC 7.1.0 for example.
>>>>
>>>> The absence of documentation doesn't mean the attribute had different
>>>> (or even undefined) meaning in earlier versions. Instead it means one
>>>> would need to consult other places (source code?) to figure out whether
>>>> there was any behavioral difference (I don't think there was).
>>>>
>>>> That said, ...
>>>>
>>>>> So in conclusion, I think it is better to avoid __attribute_pure__ and
>>>>> use SAF-xx-safe or an alternative approach instead.
>>>>
>>>> ... I agree here. We just don't want to take chances.
>>>
>>> Let me resurrect this thread.
>>>
>>> Could we use something like "pure" that we #define as we want?
>>>
>>> Depending on the compiler version or other options we could #define pure
>>> to __attribute_pure__ or to nothing.
>>
>> While we can do about anything, I don't think it's a good idea to overload
>> a well known term with something having somewhat different meaning. If a
>> differently named custom attribute helps, that might be a possible option.
> 
> It doesn't have a different meaning. If it had a different meaning I'd
> agree with you.

Then we need to sort this aspect first: If there was no difference in
meaning, we ought to be using the real attribute, not a pseudo
surrogate. Yet the earlier discussion, according to my understanding,
has led to the understanding that for the given example the real
attribute cannot be applied entirely legitimately. Hence why the
thinking of alternatives actually started. What am I missing?

> The goal is for the #define to have exactly the same meaning as the gcc
> definition from gcc 9 onward. However, other versions of gcc or other
> compilers could have different semantics. Also we might not want to
> allow gcc to perform the optimizations that it might want to do if the
> attribute is passed.
> 
> So the definition would be clear and 100% aligned with the modern gcc
> definition. However we would be able to control the behavior better.

If we feared older gcc didn't implement "pure" suitably, we should
simply make __attribute_pure__ expand to nothing there. (Still use of
the attribute then would need limiting to cases where it can validly
be applied.)

Jan

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 9590 bytes --]

On Mon, 26 Feb 2024, Jan Beulich wrote:
> On 23.02.2024 23:36, Stefano Stabellini wrote:
> > On Fri, 23 Feb 2024, Jan Beulich wrote:
> >> On 23.02.2024 02:32, Stefano Stabellini wrote:
> >>> On Tue, 24 Oct 2023, Jan Beulich wrote:
> >>>> On 24.10.2023 00:05, Stefano Stabellini wrote:
> >>>>> On Mon, 23 Oct 2023, Jan Beulich wrote:
> >>>>>> On 23.10.2023 17:23, Simone Ballarin wrote:
> >>>>>>> On 23/10/23 15:34, Jan Beulich wrote:
> >>>>>>>> On 18.10.2023 16:18, Simone Ballarin wrote:
> >>>>>>>>> --- a/xen/include/xen/pdx.h
> >>>>>>>>> +++ b/xen/include/xen/pdx.h
> >>>>>>>>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
> >>>>>>>>>    * @param pdx Page index
> >>>>>>>>>    * @return Obtained pfn after decompressing the pdx
> >>>>>>>>>    */
> >>>>>>>>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
> >>>>>>>>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
> >>>>>>>>>   {
> >>>>>>>>>       return (pdx & pfn_pdx_bottom_mask) |
> >>>>>>>>>              ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
> >>>>>>>>
> >>>>>>>> Taking this as an example for what I've said above: The compiler can't
> >>>>>>>> know that the globals used by the functions won't change value. Even
> >>>>>>>> within Xen it is only by convention that these variables are assigned
> >>>>>>>> their values during boot, and then aren't changed anymore. Which makes
> >>>>>>>> me wonder: Did you check carefully that around the time the variables
> >>>>>>>> have their values established, no calls to the functions exist (which
> >>>>>>>> might then be subject to folding)?
> >>>>>>>
> >>>>>>> There is no need to check that, the GCC documentation explicitly says:
> >>>>>>>
> >>>>>>> However, functions declared with the pure attribute *can safely read any 
> >>>>>>> non-volatile objects*, and modify the value of objects in a way that 
> >>>>>>> does not affect their return value or the observable state of the program.
> >>>>>>
> >>>>>> I did quote this same text in response to what Andrew has said, but I also
> >>>>>> did note there that this needs to be taken with a grain of salt: The
> >>>>>> compiler generally assumes a single-threaded environment, i.e. no changes
> >>>>>> to globals behind the back of the code it is processing.
> >>>>>
> >>>>> Let's start from the beginning. The reason for Simone to add
> >>>>> __attribute_pure__ to pdx_to_pfn and other functions is for
> >>>>> documentation purposes. It is OK if it doesn't serve any purpose other
> >>>>> than documentation.
> >>>>>
> >>>>> Andrew, for sure we do not want to lie to the compiler and introduce
> >>>>> undefined behavior. If we think there is a risk of it, we should not do
> >>>>> it.
> >>>>>
> >>>>> So, what do we want to document? We want to document that the function
> >>>>> does not have side effects according to MISRA's definition of it, which
> >>>>> might subtly differ from GCC's definition.
> >>>>>
> >>>>> Looking at GCC's definition of __attribute_pure__, with the
> >>>>> clarification statement copy/pasted above by both Simone and Jan, it
> >>>>> seems that __attribute_pure__ matches MISRA's definition of a function
> >>>>> without side effects. It also seems that pdx_to_pfn abides to that
> >>>>> definition.
> >>>>>
> >>>>> Jan has a point that GCC might be making other assumptions
> >>>>> (single-thread execution) that might not hold true in our case. Given
> >>>>> the way the GCC statement is written I think this is low risk. But maybe
> >>>>> not all GCC versions we want to support in the project might have the
> >>>>> same definition of __attribute_pure__. So we could end up using
> >>>>> __attribute_pure__ correctly for the GCC version used for safety (GCC
> >>>>> 12.1, see docs/misra/C-language-toolchain.rst) but it might actually
> >>>>> break an older GCC version.
> >>>>>
> >>>>>
> >>>>> So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
> >>>>> Clang version might interpret __attribute_pure__ differently and
> >>>>> potentially misbehave.
> >>>>>
> >>>>> Option#2 is to avoid this risk, by not using __attribute_pure__.
> >>>>> Instead, we can use SAF-xx-safe or deviations.rst to document that
> >>>>> pdx_to_pfn and other functions like it are without side effects
> >>>>> according to MISRA's definition.
> >>>>>
> >>>>>
> >>>>> Both options have pros and cons. To me the most important factor is how
> >>>>> many GCC versions come with the statement "pure attribute can safely
> >>>>> read any non-volatile objects, and modify the value of objects in a way
> >>>>> that does not affect their return value or the observable state of the
> >>>>> program".
> >>>>>
> >>>>> I checked and these are the results:
> >>>>> - gcc 4.0.2: no statement
> >>>>> - gcc 5.1.0: no statement
> >>>>> - gcc 6.1.0: no statement
> >>>>> - gcc 7.1.0: no statement
> >>>>> - gcc 8.1.0: alternative statement "The pure attribute imposes similar
> >>>>>   but looser restrictions on a function’s definition than the const
> >>>>>   attribute: it allows the function to read global variables."
> >>>>> - gcc 9.1.0: yes statement
> >>>>>
> >>>>>
> >>>>> So based on the above, __attribute_pure__ comes with its current
> >>>>> definition only from gcc 9 onward. I don't know if as a Xen community we
> >>>>> clearly declare a range of supported compilers, but I would imagine we
> >>>>> would still want to support gcc versions older than 9? (Not to mention
> >>>>> clang, which I haven't checked.)
> >>>>>
> >>>>> It doesn't seem to me that __attribute_pure__ could be correctly used on
> >>>>> pdx_to_pfn with GCC 7.1.0 for example.
> >>>>
> >>>> The absence of documentation doesn't mean the attribute had different
> >>>> (or even undefined) meaning in earlier versions. Instead it means one
> >>>> would need to consult other places (source code?) to figure out whether
> >>>> there was any behavioral difference (I don't think there was).
> >>>>
> >>>> That said, ...
> >>>>
> >>>>> So in conclusion, I think it is better to avoid __attribute_pure__ and
> >>>>> use SAF-xx-safe or an alternative approach instead.
> >>>>
> >>>> ... I agree here. We just don't want to take chances.
> >>>
> >>> Let me resurrect this thread.
> >>>
> >>> Could we use something like "pure" that we #define as we want?
> >>>
> >>> Depending on the compiler version or other options we could #define pure
> >>> to __attribute_pure__ or to nothing.
> >>
> >> While we can do about anything, I don't think it's a good idea to overload
> >> a well known term with something having somewhat different meaning. If a
> >> differently named custom attribute helps, that might be a possible option.
> > 
> > It doesn't have a different meaning. If it had a different meaning I'd
> > agree with you.
> 
> Then we need to sort this aspect first: If there was no difference in
> meaning, we ought to be using the real attribute, not a pseudo
> surrogate. Yet the earlier discussion, according to my understanding,
> has led to the understanding that for the given example the real
> attribute cannot be applied entirely legitimately. Hence why the
> thinking of alternatives actually started. What am I missing?

There are two different questions:
1) using __attribute_pure__ in general when appropriate
2) using __attribute_pure__ in pdx_to_pfn as this patch does


I was talking about 1): as a general approach it looks like a good idea
to use __attribute_pure__ when possible and appropriate.

Now let's talk about 2). The latest definition of __attribute_pure__ is:

"""
The pure attribute prohibits a function from modifying the state of the program that is observable by means other than inspecting the function’s return value. However, functions declared with the pure attribute can safely read any non-volatile objects, and modify the value of objects in a way that does not affect their return value or the observable state of the program.
"""

So there are two interesting issues:

a) While this documentation explicitly allows for reading global vars,
older versions of the docs are less clear. What do we do about them?

b) Jan wrote that he interprets the statements above to be only valid in
a single-threaded environment


To be honest, I am not convinced by b). Jan, is there a statement in the
GCC docs that says that all the attributes (pure being one of them) only
apply to a single-thread environment? That would be extremely limiting
for something like __attribute_pure__. I think we should take the
documentation of attribute pure at face value. To me, it clearly applies
to pdx_to_pfn. Roberto and the team at Bugseng came to the same
conclusion.

On the other end, I think a) is important. Older version of GCC don't
clarify the behavior toward global variables. From the documentation, I
would use __attribute_pure__ only with GCC 9 or later. Which is why we
need the #define.


> > The goal is for the #define to have exactly the same meaning as the gcc
> > definition from gcc 9 onward. However, other versions of gcc or other
> > compilers could have different semantics. Also we might not want to
> > allow gcc to perform the optimizations that it might want to do if the
> > attribute is passed.
> > 
> > So the definition would be clear and 100% aligned with the modern gcc
> > definition. However we would be able to control the behavior better.
> 
> If we feared older gcc didn't implement "pure" suitably, we should
> simply make __attribute_pure__ expand to nothing there. (Still use of
> the attribute then would need limiting to cases where it can validly
> be applied.)

That's fine by me

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Jan Beulich]

On 27.02.2024 00:48, Stefano Stabellini wrote:
> On Mon, 26 Feb 2024, Jan Beulich wrote:
>> On 23.02.2024 23:36, Stefano Stabellini wrote:
>>> On Fri, 23 Feb 2024, Jan Beulich wrote:
>>>> On 23.02.2024 02:32, Stefano Stabellini wrote:
>>>>> On Tue, 24 Oct 2023, Jan Beulich wrote:
>>>>>> On 24.10.2023 00:05, Stefano Stabellini wrote:
>>>>>>> On Mon, 23 Oct 2023, Jan Beulich wrote:
>>>>>>>> On 23.10.2023 17:23, Simone Ballarin wrote:
>>>>>>>>> On 23/10/23 15:34, Jan Beulich wrote:
>>>>>>>>>> On 18.10.2023 16:18, Simone Ballarin wrote:
>>>>>>>>>>> --- a/xen/include/xen/pdx.h
>>>>>>>>>>> +++ b/xen/include/xen/pdx.h
>>>>>>>>>>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
>>>>>>>>>>>    * @param pdx Page index
>>>>>>>>>>>    * @return Obtained pfn after decompressing the pdx
>>>>>>>>>>>    */
>>>>>>>>>>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
>>>>>>>>>>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
>>>>>>>>>>>   {
>>>>>>>>>>>       return (pdx & pfn_pdx_bottom_mask) |
>>>>>>>>>>>              ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
>>>>>>>>>>
>>>>>>>>>> Taking this as an example for what I've said above: The compiler can't
>>>>>>>>>> know that the globals used by the functions won't change value. Even
>>>>>>>>>> within Xen it is only by convention that these variables are assigned
>>>>>>>>>> their values during boot, and then aren't changed anymore. Which makes
>>>>>>>>>> me wonder: Did you check carefully that around the time the variables
>>>>>>>>>> have their values established, no calls to the functions exist (which
>>>>>>>>>> might then be subject to folding)?
>>>>>>>>>
>>>>>>>>> There is no need to check that, the GCC documentation explicitly says:
>>>>>>>>>
>>>>>>>>> However, functions declared with the pure attribute *can safely read any 
>>>>>>>>> non-volatile objects*, and modify the value of objects in a way that 
>>>>>>>>> does not affect their return value or the observable state of the program.
>>>>>>>>
>>>>>>>> I did quote this same text in response to what Andrew has said, but I also
>>>>>>>> did note there that this needs to be taken with a grain of salt: The
>>>>>>>> compiler generally assumes a single-threaded environment, i.e. no changes
>>>>>>>> to globals behind the back of the code it is processing.
>>>>>>>
>>>>>>> Let's start from the beginning. The reason for Simone to add
>>>>>>> __attribute_pure__ to pdx_to_pfn and other functions is for
>>>>>>> documentation purposes. It is OK if it doesn't serve any purpose other
>>>>>>> than documentation.
>>>>>>>
>>>>>>> Andrew, for sure we do not want to lie to the compiler and introduce
>>>>>>> undefined behavior. If we think there is a risk of it, we should not do
>>>>>>> it.
>>>>>>>
>>>>>>> So, what do we want to document? We want to document that the function
>>>>>>> does not have side effects according to MISRA's definition of it, which
>>>>>>> might subtly differ from GCC's definition.
>>>>>>>
>>>>>>> Looking at GCC's definition of __attribute_pure__, with the
>>>>>>> clarification statement copy/pasted above by both Simone and Jan, it
>>>>>>> seems that __attribute_pure__ matches MISRA's definition of a function
>>>>>>> without side effects. It also seems that pdx_to_pfn abides to that
>>>>>>> definition.
>>>>>>>
>>>>>>> Jan has a point that GCC might be making other assumptions
>>>>>>> (single-thread execution) that might not hold true in our case. Given
>>>>>>> the way the GCC statement is written I think this is low risk. But maybe
>>>>>>> not all GCC versions we want to support in the project might have the
>>>>>>> same definition of __attribute_pure__. So we could end up using
>>>>>>> __attribute_pure__ correctly for the GCC version used for safety (GCC
>>>>>>> 12.1, see docs/misra/C-language-toolchain.rst) but it might actually
>>>>>>> break an older GCC version.
>>>>>>>
>>>>>>>
>>>>>>> So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
>>>>>>> Clang version might interpret __attribute_pure__ differently and
>>>>>>> potentially misbehave.
>>>>>>>
>>>>>>> Option#2 is to avoid this risk, by not using __attribute_pure__.
>>>>>>> Instead, we can use SAF-xx-safe or deviations.rst to document that
>>>>>>> pdx_to_pfn and other functions like it are without side effects
>>>>>>> according to MISRA's definition.
>>>>>>>
>>>>>>>
>>>>>>> Both options have pros and cons. To me the most important factor is how
>>>>>>> many GCC versions come with the statement "pure attribute can safely
>>>>>>> read any non-volatile objects, and modify the value of objects in a way
>>>>>>> that does not affect their return value or the observable state of the
>>>>>>> program".
>>>>>>>
>>>>>>> I checked and these are the results:
>>>>>>> - gcc 4.0.2: no statement
>>>>>>> - gcc 5.1.0: no statement
>>>>>>> - gcc 6.1.0: no statement
>>>>>>> - gcc 7.1.0: no statement
>>>>>>> - gcc 8.1.0: alternative statement "The pure attribute imposes similar
>>>>>>>   but looser restrictions on a function’s definition than the const
>>>>>>>   attribute: it allows the function to read global variables."
>>>>>>> - gcc 9.1.0: yes statement
>>>>>>>
>>>>>>>
>>>>>>> So based on the above, __attribute_pure__ comes with its current
>>>>>>> definition only from gcc 9 onward. I don't know if as a Xen community we
>>>>>>> clearly declare a range of supported compilers, but I would imagine we
>>>>>>> would still want to support gcc versions older than 9? (Not to mention
>>>>>>> clang, which I haven't checked.)
>>>>>>>
>>>>>>> It doesn't seem to me that __attribute_pure__ could be correctly used on
>>>>>>> pdx_to_pfn with GCC 7.1.0 for example.
>>>>>>
>>>>>> The absence of documentation doesn't mean the attribute had different
>>>>>> (or even undefined) meaning in earlier versions. Instead it means one
>>>>>> would need to consult other places (source code?) to figure out whether
>>>>>> there was any behavioral difference (I don't think there was).
>>>>>>
>>>>>> That said, ...
>>>>>>
>>>>>>> So in conclusion, I think it is better to avoid __attribute_pure__ and
>>>>>>> use SAF-xx-safe or an alternative approach instead.
>>>>>>
>>>>>> ... I agree here. We just don't want to take chances.
>>>>>
>>>>> Let me resurrect this thread.
>>>>>
>>>>> Could we use something like "pure" that we #define as we want?
>>>>>
>>>>> Depending on the compiler version or other options we could #define pure
>>>>> to __attribute_pure__ or to nothing.
>>>>
>>>> While we can do about anything, I don't think it's a good idea to overload
>>>> a well known term with something having somewhat different meaning. If a
>>>> differently named custom attribute helps, that might be a possible option.
>>>
>>> It doesn't have a different meaning. If it had a different meaning I'd
>>> agree with you.
>>
>> Then we need to sort this aspect first: If there was no difference in
>> meaning, we ought to be using the real attribute, not a pseudo
>> surrogate. Yet the earlier discussion, according to my understanding,
>> has led to the understanding that for the given example the real
>> attribute cannot be applied entirely legitimately. Hence why the
>> thinking of alternatives actually started. What am I missing?
> 
> There are two different questions:
> 1) using __attribute_pure__ in general when appropriate
> 2) using __attribute_pure__ in pdx_to_pfn as this patch does
> 
> 
> I was talking about 1): as a general approach it looks like a good idea
> to use __attribute_pure__ when possible and appropriate.
> 
> Now let's talk about 2). The latest definition of __attribute_pure__ is:
> 
> """
> The pure attribute prohibits a function from modifying the state of the program that is observable by means other than inspecting the function’s return value. However, functions declared with the pure attribute can safely read any non-volatile objects, and modify the value of objects in a way that does not affect their return value or the observable state of the program.
> """
> 
> So there are two interesting issues:
> 
> a) While this documentation explicitly allows for reading global vars,
> older versions of the docs are less clear. What do we do about them?
> 
> b) Jan wrote that he interprets the statements above to be only valid in
> a single-threaded environment
> 
> 
> To be honest, I am not convinced by b). Jan, is there a statement in the
> GCC docs that says that all the attributes (pure being one of them) only
> apply to a single-thread environment?

It would need to be the other way around, I'm afraid: C99 defines its
abstract machine in a way not even considering possible parallel
execution (except for other external agents, e.g. when "volatile" is
necessary for memory accesses when that memory may also be modified
by such an external agent, e.g. a device on the bus). Hence we'd need
to find an explicit statement in gcc docs which relaxes that globally
or for certain aspects.

> That would be extremely limiting
> for something like __attribute_pure__. I think we should take the
> documentation of attribute pure at face value. To me, it clearly applies
> to pdx_to_pfn. Roberto and the team at Bugseng came to the same
> conclusion.
> 
> On the other end, I think a) is important. Older version of GCC don't
> clarify the behavior toward global variables. From the documentation, I
> would use __attribute_pure__ only with GCC 9 or later. Which is why we
> need the #define.

Right, this is a position we can take. As said, I think we'd then limit
ourselves more than necessary. Otoh the number of people using gcc8 or
older to build up-to-date Xen should be constantly decreasing ...

Jan

>>> The goal is for the #define to have exactly the same meaning as the gcc
>>> definition from gcc 9 onward. However, other versions of gcc or other
>>> compilers could have different semantics. Also we might not want to
>>> allow gcc to perform the optimizations that it might want to do if the
>>> attribute is passed.
>>>
>>> So the definition would be clear and 100% aligned with the modern gcc
>>> definition. However we would be able to control the behavior better.
>>
>> If we feared older gcc didn't implement "pure" suitably, we should
>> simply make __attribute_pure__ expand to nothing there. (Still use of
>> the attribute then would need limiting to cases where it can validly
>> be applied.)
> 
> That's fine by me

Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 10373 bytes --]

On Tue, 27 Feb 2024, Jan Beulich wrote:
> On 27.02.2024 00:48, Stefano Stabellini wrote:
> > On Mon, 26 Feb 2024, Jan Beulich wrote:
> >> On 23.02.2024 23:36, Stefano Stabellini wrote:
> >>> On Fri, 23 Feb 2024, Jan Beulich wrote:
> >>>> On 23.02.2024 02:32, Stefano Stabellini wrote:
> >>>>> On Tue, 24 Oct 2023, Jan Beulich wrote:
> >>>>>> On 24.10.2023 00:05, Stefano Stabellini wrote:
> >>>>>>> On Mon, 23 Oct 2023, Jan Beulich wrote:
> >>>>>>>> On 23.10.2023 17:23, Simone Ballarin wrote:
> >>>>>>>>> On 23/10/23 15:34, Jan Beulich wrote:
> >>>>>>>>>> On 18.10.2023 16:18, Simone Ballarin wrote:
> >>>>>>>>>>> --- a/xen/include/xen/pdx.h
> >>>>>>>>>>> +++ b/xen/include/xen/pdx.h
> >>>>>>>>>>> @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
> >>>>>>>>>>>    * @param pdx Page index
> >>>>>>>>>>>    * @return Obtained pfn after decompressing the pdx
> >>>>>>>>>>>    */
> >>>>>>>>>>> -static inline unsigned long pdx_to_pfn(unsigned long pdx)
> >>>>>>>>>>> +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
> >>>>>>>>>>>   {
> >>>>>>>>>>>       return (pdx & pfn_pdx_bottom_mask) |
> >>>>>>>>>>>              ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
> >>>>>>>>>>
> >>>>>>>>>> Taking this as an example for what I've said above: The compiler can't
> >>>>>>>>>> know that the globals used by the functions won't change value. Even
> >>>>>>>>>> within Xen it is only by convention that these variables are assigned
> >>>>>>>>>> their values during boot, and then aren't changed anymore. Which makes
> >>>>>>>>>> me wonder: Did you check carefully that around the time the variables
> >>>>>>>>>> have their values established, no calls to the functions exist (which
> >>>>>>>>>> might then be subject to folding)?
> >>>>>>>>>
> >>>>>>>>> There is no need to check that, the GCC documentation explicitly says:
> >>>>>>>>>
> >>>>>>>>> However, functions declared with the pure attribute *can safely read any 
> >>>>>>>>> non-volatile objects*, and modify the value of objects in a way that 
> >>>>>>>>> does not affect their return value or the observable state of the program.
> >>>>>>>>
> >>>>>>>> I did quote this same text in response to what Andrew has said, but I also
> >>>>>>>> did note there that this needs to be taken with a grain of salt: The
> >>>>>>>> compiler generally assumes a single-threaded environment, i.e. no changes
> >>>>>>>> to globals behind the back of the code it is processing.
> >>>>>>>
> >>>>>>> Let's start from the beginning. The reason for Simone to add
> >>>>>>> __attribute_pure__ to pdx_to_pfn and other functions is for
> >>>>>>> documentation purposes. It is OK if it doesn't serve any purpose other
> >>>>>>> than documentation.
> >>>>>>>
> >>>>>>> Andrew, for sure we do not want to lie to the compiler and introduce
> >>>>>>> undefined behavior. If we think there is a risk of it, we should not do
> >>>>>>> it.
> >>>>>>>
> >>>>>>> So, what do we want to document? We want to document that the function
> >>>>>>> does not have side effects according to MISRA's definition of it, which
> >>>>>>> might subtly differ from GCC's definition.
> >>>>>>>
> >>>>>>> Looking at GCC's definition of __attribute_pure__, with the
> >>>>>>> clarification statement copy/pasted above by both Simone and Jan, it
> >>>>>>> seems that __attribute_pure__ matches MISRA's definition of a function
> >>>>>>> without side effects. It also seems that pdx_to_pfn abides to that
> >>>>>>> definition.
> >>>>>>>
> >>>>>>> Jan has a point that GCC might be making other assumptions
> >>>>>>> (single-thread execution) that might not hold true in our case. Given
> >>>>>>> the way the GCC statement is written I think this is low risk. But maybe
> >>>>>>> not all GCC versions we want to support in the project might have the
> >>>>>>> same definition of __attribute_pure__. So we could end up using
> >>>>>>> __attribute_pure__ correctly for the GCC version used for safety (GCC
> >>>>>>> 12.1, see docs/misra/C-language-toolchain.rst) but it might actually
> >>>>>>> break an older GCC version.
> >>>>>>>
> >>>>>>>
> >>>>>>> So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
> >>>>>>> Clang version might interpret __attribute_pure__ differently and
> >>>>>>> potentially misbehave.
> >>>>>>>
> >>>>>>> Option#2 is to avoid this risk, by not using __attribute_pure__.
> >>>>>>> Instead, we can use SAF-xx-safe or deviations.rst to document that
> >>>>>>> pdx_to_pfn and other functions like it are without side effects
> >>>>>>> according to MISRA's definition.
> >>>>>>>
> >>>>>>>
> >>>>>>> Both options have pros and cons. To me the most important factor is how
> >>>>>>> many GCC versions come with the statement "pure attribute can safely
> >>>>>>> read any non-volatile objects, and modify the value of objects in a way
> >>>>>>> that does not affect their return value or the observable state of the
> >>>>>>> program".
> >>>>>>>
> >>>>>>> I checked and these are the results:
> >>>>>>> - gcc 4.0.2: no statement
> >>>>>>> - gcc 5.1.0: no statement
> >>>>>>> - gcc 6.1.0: no statement
> >>>>>>> - gcc 7.1.0: no statement
> >>>>>>> - gcc 8.1.0: alternative statement "The pure attribute imposes similar
> >>>>>>>   but looser restrictions on a function’s definition than the const
> >>>>>>>   attribute: it allows the function to read global variables."
> >>>>>>> - gcc 9.1.0: yes statement
> >>>>>>>
> >>>>>>>
> >>>>>>> So based on the above, __attribute_pure__ comes with its current
> >>>>>>> definition only from gcc 9 onward. I don't know if as a Xen community we
> >>>>>>> clearly declare a range of supported compilers, but I would imagine we
> >>>>>>> would still want to support gcc versions older than 9? (Not to mention
> >>>>>>> clang, which I haven't checked.)
> >>>>>>>
> >>>>>>> It doesn't seem to me that __attribute_pure__ could be correctly used on
> >>>>>>> pdx_to_pfn with GCC 7.1.0 for example.
> >>>>>>
> >>>>>> The absence of documentation doesn't mean the attribute had different
> >>>>>> (or even undefined) meaning in earlier versions. Instead it means one
> >>>>>> would need to consult other places (source code?) to figure out whether
> >>>>>> there was any behavioral difference (I don't think there was).
> >>>>>>
> >>>>>> That said, ...
> >>>>>>
> >>>>>>> So in conclusion, I think it is better to avoid __attribute_pure__ and
> >>>>>>> use SAF-xx-safe or an alternative approach instead.
> >>>>>>
> >>>>>> ... I agree here. We just don't want to take chances.
> >>>>>
> >>>>> Let me resurrect this thread.
> >>>>>
> >>>>> Could we use something like "pure" that we #define as we want?
> >>>>>
> >>>>> Depending on the compiler version or other options we could #define pure
> >>>>> to __attribute_pure__ or to nothing.
> >>>>
> >>>> While we can do about anything, I don't think it's a good idea to overload
> >>>> a well known term with something having somewhat different meaning. If a
> >>>> differently named custom attribute helps, that might be a possible option.
> >>>
> >>> It doesn't have a different meaning. If it had a different meaning I'd
> >>> agree with you.
> >>
> >> Then we need to sort this aspect first: If there was no difference in
> >> meaning, we ought to be using the real attribute, not a pseudo
> >> surrogate. Yet the earlier discussion, according to my understanding,
> >> has led to the understanding that for the given example the real
> >> attribute cannot be applied entirely legitimately. Hence why the
> >> thinking of alternatives actually started. What am I missing?
> > 
> > There are two different questions:
> > 1) using __attribute_pure__ in general when appropriate
> > 2) using __attribute_pure__ in pdx_to_pfn as this patch does
> > 
> > 
> > I was talking about 1): as a general approach it looks like a good idea
> > to use __attribute_pure__ when possible and appropriate.
> > 
> > Now let's talk about 2). The latest definition of __attribute_pure__ is:
> > 
> > """
> > The pure attribute prohibits a function from modifying the state of the program that is observable by means other than inspecting the function’s return value. However, functions declared with the pure attribute can safely read any non-volatile objects, and modify the value of objects in a way that does not affect their return value or the observable state of the program.
> > """
> > 
> > So there are two interesting issues:
> > 
> > a) While this documentation explicitly allows for reading global vars,
> > older versions of the docs are less clear. What do we do about them?
> > 
> > b) Jan wrote that he interprets the statements above to be only valid in
> > a single-threaded environment
> > 
> > 
> > To be honest, I am not convinced by b). Jan, is there a statement in the
> > GCC docs that says that all the attributes (pure being one of them) only
> > apply to a single-thread environment?
> 
> It would need to be the other way around, I'm afraid: C99 defines its
> abstract machine in a way not even considering possible parallel
> execution (except for other external agents, e.g. when "volatile" is
> necessary for memory accesses when that memory may also be modified
> by such an external agent, e.g. a device on the bus). Hence we'd need
> to find an explicit statement in gcc docs which relaxes that globally
> or for certain aspects.

I don't think so: the way C99 defines its abstract machine is not
tightly coupled with the way gcc extensions are defined.

Roberto, you have worked with gcc extensions quite a bit, what's your
take on this?

Other maintainers, please express your view.


> > That would be extremely limiting
> > for something like __attribute_pure__. I think we should take the
> > documentation of attribute pure at face value. To me, it clearly applies
> > to pdx_to_pfn. Roberto and the team at Bugseng came to the same
> > conclusion.
> > 
> > On the other end, I think a) is important. Older version of GCC don't
> > clarify the behavior toward global variables. From the documentation, I
> > would use __attribute_pure__ only with GCC 9 or later. Which is why we
> > need the #define.
> 
> Right, this is a position we can take. As said, I think we'd then limit
> ourselves more than necessary. Otoh the number of people using gcc8 or
> older to build up-to-date Xen should be constantly decreasing ...

That is true, but I think it is an OK limitation to have.