Rule 18.3: "The relational operators >, >=, < and <=
shall not be applied to objects of pointer type
except where they point into the same object".

Update relational comparison to cast `text_start`
(void pointer) to `unsigned long`. This ensures the
comparison occurs between two values of the same integral type.

Update relational comparison between `lock1` and `lock2` to cast
pointers to `uintptr_t`. This ensures MISRA C compliance and avoids
undefined behavior when comparing pointer values directly.

Update for-loop condition to cast pointers `p` and `params->checksum`
to `uintptr_t` for the relational `<` operator. This resolves a MISRA C 18.3
violation by ensuring that relational operations are not performed directly
on pointers of different objects (which is undefined behavior).

Add Rule 18.3 deviations.

Add Rule 18.3 to the monitored set.

No functional changes to program behavior.

Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
---
This patch eliminates MISRA C Rule 18.3 violations for both ARM and X86.

Test CI: https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/1943306650
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 8 ++++++++
 automation/eclair_analysis/ECLAIR/monitored.ecl  | 1 +
 xen/arch/x86/efi/efi-boot.h                      | 3 ++-
 xen/common/sched/core.c                          | 2 +-
 xen/common/virtual_region.c                      | 4 ++--
 5 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index 483507e7b9..d89834a49b 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -568,6 +568,14 @@ C99 Undefined Behaviour 45: Pointers that do not point into, or just beyond, the
 -config=MC3A2.R18.2,reports+={safe, "any_area(any_loc(any_exp(macro(^page_to_mfn$))))"}
 -doc_end
 
+-doc_begin="Consider relational pointer comparisons in kernel-related sections as safe and compliant."
+-config=MC3R1.R18.3,reports+={safe, "any_area(any_loc(any_exp(macro(name(is_kernel||is_kernel_text||is_kernel_rodata||is_kernel_inittext)))))"}
+-doc_end
+
+-doc_begin="Allow deviations for pointer comparisons in BUG_ON and ASSERT macros, treating them as safe for debugging and validation."
+-config=MC3R1.R18.3,reports+={safe, "any_area(any_loc(any_exp(macro(name(BUG_ON||ASSERT)))))"}
+-doc_end
+
 -doc_begin="Flexible array members are deliberately used and XEN developers are aware of the dangers related to them:
 unexpected result when the structure is given as argument to a sizeof() operator and the truncation in assignment between structures."
 -config=MC3A2.R18.7,reports+={deliberate, "any()"}
diff --git a/automation/eclair_analysis/ECLAIR/monitored.ecl b/automation/eclair_analysis/ECLAIR/monitored.ecl
index 00bff9edbe..b8fb297e73 100644
--- a/automation/eclair_analysis/ECLAIR/monitored.ecl
+++ b/automation/eclair_analysis/ECLAIR/monitored.ecl
@@ -68,6 +68,7 @@
 -enable=MC3A2.R17.6
 -enable=MC3A2.R18.1
 -enable=MC3A2.R18.2
+-enable=MC3A2.R18.3
 -enable=MC3A2.R18.6
 -enable=MC3A2.R18.8
 -enable=MC3A2.R19.1
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 0194720003..170c569eb4 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -461,7 +461,8 @@ static void __init efi_arch_edd(void)
                 params->device_path_info_length =
                     sizeof(struct edd_device_params) -
                     offsetof(struct edd_device_params, key);
-                for ( p = (const u8 *)&params->key; p < &params->checksum; ++p )
+                for ( p = (const u8 *)&params->key;
+                      (uintptr_t)p < (uintptr_t)&params->checksum; ++p )
                     params->checksum -= *p;
                 break;
             case MEDIA_DEVICE_PATH:
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index ea95dea65a..c3c101c142 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -360,7 +360,7 @@ static always_inline void sched_spin_lock_double(
     {
         *flags = _spin_lock_irqsave(lock1);
     }
-    else if ( lock1 < lock2 )
+    else if ( (uintptr_t)lock1 < (uintptr_t)lock2 )
     {
         *flags = _spin_lock_irqsave(lock1);
         _spin_lock(lock2);
diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
index 1dc2e9f592..515751b6c3 100644
--- a/xen/common/virtual_region.c
+++ b/xen/common/virtual_region.c
@@ -83,8 +83,8 @@ const struct virtual_region *find_text_region(unsigned long addr)
     rcu_read_lock(&rcu_virtual_region_lock);
     list_for_each_entry_rcu ( iter, &virtual_region_list, list )
     {
-        if ( (void *)addr >= iter->text_start &&
-             (void *)addr <  iter->text_end )
+        if ( addr >= (unsigned long)iter->text_start &&
+             addr <  (unsigned long)iter->text_end )
         {
             region = iter;
             break;
-- 
2.43.0

On 2025-07-23 12:12, Dmytro Prokopchuk1 wrote:
> Rule 18.3: "The relational operators >, >=, < and <=
> shall not be applied to objects of pointer type
> except where they point into the same object".
> 
> Update relational comparison to cast `text_start`
> (void pointer) to `unsigned long`. This ensures the
> comparison occurs between two values of the same integral type.
> 
> Update relational comparison between `lock1` and `lock2` to cast
> pointers to `uintptr_t`. This ensures MISRA C compliance and avoids
> undefined behavior when comparing pointer values directly.
> 
> Update for-loop condition to cast pointers `p` and `params->checksum`
> to `uintptr_t` for the relational `<` operator. This resolves a MISRA C 
> 18.3
> violation by ensuring that relational operations are not performed 
> directly
> on pointers of different objects (which is undefined behavior).
> 
> Add Rule 18.3 deviations.
> 
> Add Rule 18.3 to the monitored set.
> 
> No functional changes to program behavior.
> 
> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
> ---
> This patch eliminates MISRA C Rule 18.3 violations for both ARM and 
> X86.
> 
> Test CI: 
> https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/1943306650
> ---
>  automation/eclair_analysis/ECLAIR/deviations.ecl | 8 ++++++++
>  automation/eclair_analysis/ECLAIR/monitored.ecl  | 1 +
>  xen/arch/x86/efi/efi-boot.h                      | 3 ++-
>  xen/common/sched/core.c                          | 2 +-
>  xen/common/virtual_region.c                      | 4 ++--
>  5 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index 483507e7b9..d89834a49b 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -568,6 +568,14 @@ C99 Undefined Behaviour 45: Pointers that do not 
> point into, or just beyond, the
>  -config=MC3A2.R18.2,reports+={safe, 
> "any_area(any_loc(any_exp(macro(^page_to_mfn$))))"}
>  -doc_end
> 
> +-doc_begin="Consider relational pointer comparisons in kernel-related 
> sections as safe and compliant."

This would need some evidence that they don't compare pointers to 
different arrays, wouldn't it?

> +-config=MC3R1.R18.3,reports+={safe, 
> "any_area(any_loc(any_exp(macro(name(is_kernel||is_kernel_text||is_kernel_rodata||is_kernel_inittext)))))"}
> +-doc_end
> +
> +-doc_begin="Allow deviations for pointer comparisons in BUG_ON and 
> ASSERT macros, treating them as safe for debugging and validation."
> +-config=MC3R1.R18.3,reports+={safe, 
> "any_area(any_loc(any_exp(macro(name(BUG_ON||ASSERT)))))"}
> +-doc_end
> +

same here: no matter that they are used for debugging, if you mark 
something as "safe", then the argument needs to be there (in the .rst 
file, but still).

>  -doc_begin="Flexible array members are deliberately used and XEN 
> developers are aware of the dangers related to them:
>  unexpected result when the structure is given as argument to a 
> sizeof() operator and the truncation in assignment between structures."
>  -config=MC3A2.R18.7,reports+={deliberate, "any()"}
> diff --git a/automation/eclair_analysis/ECLAIR/monitored.ecl 
> b/automation/eclair_analysis/ECLAIR/monitored.ecl
> index 00bff9edbe..b8fb297e73 100644
> --- a/automation/eclair_analysis/ECLAIR/monitored.ecl
> +++ b/automation/eclair_analysis/ECLAIR/monitored.ecl
> @@ -68,6 +68,7 @@
>  -enable=MC3A2.R17.6
>  -enable=MC3A2.R18.1
>  -enable=MC3A2.R18.2
> +-enable=MC3A2.R18.3
>  -enable=MC3A2.R18.6
>  -enable=MC3A2.R18.8
>  -enable=MC3A2.R19.1
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 0194720003..170c569eb4 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -461,7 +461,8 @@ static void __init efi_arch_edd(void)
>                  params->device_path_info_length =
>                      sizeof(struct edd_device_params) -
>                      offsetof(struct edd_device_params, key);
> -                for ( p = (const u8 *)&params->key; p < 
> &params->checksum; ++p )
> +                for ( p = (const u8 *)&params->key;
> +                      (uintptr_t)p < (uintptr_t)&params->checksum; ++p 
> )
>                      params->checksum -= *p;
>                  break;
>              case MEDIA_DEVICE_PATH:
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index ea95dea65a..c3c101c142 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -360,7 +360,7 @@ static always_inline void sched_spin_lock_double(
>      {
>          *flags = _spin_lock_irqsave(lock1);
>      }
> -    else if ( lock1 < lock2 )
> +    else if ( (uintptr_t)lock1 < (uintptr_t)lock2 )
>      {
>          *flags = _spin_lock_irqsave(lock1);
>          _spin_lock(lock2);
> diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
> index 1dc2e9f592..515751b6c3 100644
> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -83,8 +83,8 @@ const struct virtual_region 
> *find_text_region(unsigned long addr)
>      rcu_read_lock(&rcu_virtual_region_lock);
>      list_for_each_entry_rcu ( iter, &virtual_region_list, list )
>      {
> -        if ( (void *)addr >= iter->text_start &&
> -             (void *)addr <  iter->text_end )
> +        if ( addr >= (unsigned long)iter->text_start &&
> +             addr <  (unsigned long)iter->text_end )
>          {
>              region = iter;
>              break;

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

On 23.07.2025 12:12, Dmytro Prokopchuk1 wrote:
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -568,6 +568,14 @@ C99 Undefined Behaviour 45: Pointers that do not point into, or just beyond, the
>  -config=MC3A2.R18.2,reports+={safe, "any_area(any_loc(any_exp(macro(^page_to_mfn$))))"}
>  -doc_end
>  
> +-doc_begin="Consider relational pointer comparisons in kernel-related sections as safe and compliant."
> +-config=MC3R1.R18.3,reports+={safe, "any_area(any_loc(any_exp(macro(name(is_kernel||is_kernel_text||is_kernel_rodata||is_kernel_inittext)))))"}
> +-doc_end
> +
> +-doc_begin="Allow deviations for pointer comparisons in BUG_ON and ASSERT macros, treating them as safe for debugging and validation."
> +-config=MC3R1.R18.3,reports+={safe, "any_area(any_loc(any_exp(macro(name(BUG_ON||ASSERT)))))"}
> +-doc_end

Nit: Drop "deviations for" from the verbal description?

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -461,7 +461,8 @@ static void __init efi_arch_edd(void)
>                  params->device_path_info_length =
>                      sizeof(struct edd_device_params) -
>                      offsetof(struct edd_device_params, key);
> -                for ( p = (const u8 *)&params->key; p < &params->checksum; ++p )
> +                for ( p = (const u8 *)&params->key;
> +                      (uintptr_t)p < (uintptr_t)&params->checksum; ++p )

There mere addition of such casts makes code more fragile imo. What about the
alternative of using != instead of < here? I certainly prefer < in such situations,
but functionally != ought to be equivalent (and less constrained by C and Misra).

As mentioned several times when discussing these rules, it's also not easy to see
how "pointers of different objects" could be involved here: It's all within
*params, isn't it?

Finally, when touching such code it would be nice if u<N> was converted to
uint<N>_t.

> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -360,7 +360,7 @@ static always_inline void sched_spin_lock_double(
>      {
>          *flags = _spin_lock_irqsave(lock1);
>      }
> -    else if ( lock1 < lock2 )
> +    else if ( (uintptr_t)lock1 < (uintptr_t)lock2 )

Similarly, no matter what C or Misra may have to say here, imo such casts are
simply dangerous. Especially when open-coded.

> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -83,8 +83,8 @@ const struct virtual_region *find_text_region(unsigned long addr)
>      rcu_read_lock(&rcu_virtual_region_lock);
>      list_for_each_entry_rcu ( iter, &virtual_region_list, list )
>      {
> -        if ( (void *)addr >= iter->text_start &&
> -             (void *)addr <  iter->text_end )
> +        if ( addr >= (unsigned long)iter->text_start &&
> +             addr <  (unsigned long)iter->text_end )

Considering further casts to unsigned long of the same struct fields, was it
considered to alter the type of the struct fields instead?

Jan

On 7/23/25 16:58, Jan Beulich wrote:
> On 23.07.2025 12:12, Dmytro Prokopchuk1 wrote:
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -568,6 +568,14 @@ C99 Undefined Behaviour 45: Pointers that do not point into, or just beyond, the
>>   -config=MC3A2.R18.2,reports+={safe, "any_area(any_loc(any_exp(macro(^page_to_mfn$))))"}
>>   -doc_end
>>   
>> +-doc_begin="Consider relational pointer comparisons in kernel-related sections as safe and compliant."
>> +-config=MC3R1.R18.3,reports+={safe, "any_area(any_loc(any_exp(macro(name(is_kernel||is_kernel_text||is_kernel_rodata||is_kernel_inittext)))))"}
>> +-doc_end
>> +
>> +-doc_begin="Allow deviations for pointer comparisons in BUG_ON and ASSERT macros, treating them as safe for debugging and validation."
>> +-config=MC3R1.R18.3,reports+={safe, "any_area(any_loc(any_exp(macro(name(BUG_ON||ASSERT)))))"}
>> +-doc_end
> 
> Nit: Drop "deviations for" from the verbal description?
Ok.

> 
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -461,7 +461,8 @@ static void __init efi_arch_edd(void)
>>                   params->device_path_info_length =
>>                       sizeof(struct edd_device_params) -
>>                       offsetof(struct edd_device_params, key);
>> -                for ( p = (const u8 *)&params->key; p < &params->checksum; ++p )
>> +                for ( p = (const u8 *)&params->key;
>> +                      (uintptr_t)p < (uintptr_t)&params->checksum; ++p )
> 
> There mere addition of such casts makes code more fragile imo. What about the
> alternative of using != instead of < here? I certainly prefer < in such situations,
> but functionally != ought to be equivalent (and less constrained by C and Misra).
> 
> As mentioned several times when discussing these rules, it's also not easy to see
> how "pointers of different objects" could be involved here: It's all within
> *params, isn't it?
Hard to say something. Let's hold this so far.

> 
> Finally, when touching such code it would be nice if u<N> was converted to
> uint<N>_t.
> 
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -360,7 +360,7 @@ static always_inline void sched_spin_lock_double(
>>       {
>>           *flags = _spin_lock_irqsave(lock1);
>>       }
>> -    else if ( lock1 < lock2 )
>> +    else if ( (uintptr_t)lock1 < (uintptr_t)lock2 )
> 
> Similarly, no matter what C or Misra may have to say here, imo such casts are
> simply dangerous. Especially when open-coded.
Well, this function 'sched_spin_lock_double' has the following description:
"If locks are different, take the one with the lower address first."

I think it's save to compare the integer representations of 'lock1' and 
'lock2' addresses explicitly (casting the pointers values to an integer 
type). We need to find the 'lower address'.
Any risks here?

Dmytro
> 
>> --- a/xen/common/virtual_region.c
>> +++ b/xen/common/virtual_region.c
>> @@ -83,8 +83,8 @@ const struct virtual_region *find_text_region(unsigned long addr)
>>       rcu_read_lock(&rcu_virtual_region_lock);
>>       list_for_each_entry_rcu ( iter, &virtual_region_list, list )
>>       {
>> -        if ( (void *)addr >= iter->text_start &&
>> -             (void *)addr <  iter->text_end )
>> +        if ( addr >= (unsigned long)iter->text_start &&
>> +             addr <  (unsigned long)iter->text_end )
> 
> Considering further casts to unsigned long of the same struct fields, was it
> considered to alter the type of the struct fields instead?
There are present casts of struct fields 'text_start' and 'text_end' in 
the file 'xen/common/virtual_region.c'.

         modify_xen_mappings_lite((unsigned long)region->text_start,
                                  (unsigned long)region->text_end,
                                  PAGE_HYPERVISOR_RWX);

Changing fields type to 'unsigned long' will give the opportunity to 
remove casts from source code (mentioned before),
and remove '(void*)' casts from here:

         if ( (void *)addr >= iter->text_start &&
              (void *)addr <  iter->text_end )

Unfortunately there are places where these fields are initialized, so 
cast to the 'unsigned long' should be there.
Example:
     .text_start = _sinittext,
     .text_end = _einittext,
and
     .text_start = _sinittext,
     .text_end = _einittext,

where
     extern char _sinittext[], _einittext[];
     extern char _stext[], _etext[];

So, my proposition is to add cast to 'unsigned long' as I proposed in 
this patch.

Dmytro
> 
> Jan

On Fri, 25 Jul 2025, Dmytro Prokopchuk1 wrote:
> On 7/23/25 16:58, Jan Beulich wrote:
> > On 23.07.2025 12:12, Dmytro Prokopchuk1 wrote:
> >> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> >> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> >> @@ -568,6 +568,14 @@ C99 Undefined Behaviour 45: Pointers that do not point into, or just beyond, the
> >>   -config=MC3A2.R18.2,reports+={safe, "any_area(any_loc(any_exp(macro(^page_to_mfn$))))"}
> >>   -doc_end
> >>   
> >> +-doc_begin="Consider relational pointer comparisons in kernel-related sections as safe and compliant."
> >> +-config=MC3R1.R18.3,reports+={safe, "any_area(any_loc(any_exp(macro(name(is_kernel||is_kernel_text||is_kernel_rodata||is_kernel_inittext)))))"}
> >> +-doc_end
> >> +
> >> +-doc_begin="Allow deviations for pointer comparisons in BUG_ON and ASSERT macros, treating them as safe for debugging and validation."
> >> +-config=MC3R1.R18.3,reports+={safe, "any_area(any_loc(any_exp(macro(name(BUG_ON||ASSERT)))))"}
> >> +-doc_end
> > 
> > Nit: Drop "deviations for" from the verbal description?
> Ok.
> 
> > 
> >> --- a/xen/arch/x86/efi/efi-boot.h
> >> +++ b/xen/arch/x86/efi/efi-boot.h
> >> @@ -461,7 +461,8 @@ static void __init efi_arch_edd(void)
> >>                   params->device_path_info_length =
> >>                       sizeof(struct edd_device_params) -
> >>                       offsetof(struct edd_device_params, key);
> >> -                for ( p = (const u8 *)&params->key; p < &params->checksum; ++p )
> >> +                for ( p = (const u8 *)&params->key;
> >> +                      (uintptr_t)p < (uintptr_t)&params->checksum; ++p )
> > 
> > There mere addition of such casts makes code more fragile imo. What about the
> > alternative of using != instead of < here? I certainly prefer < in such situations,
> > but functionally != ought to be equivalent (and less constrained by C and Misra).
> > 
> > As mentioned several times when discussing these rules, it's also not easy to see
> > how "pointers of different objects" could be involved here: It's all within
> > *params, isn't it?
> Hard to say something. Let's hold this so far.
> 
> > 
> > Finally, when touching such code it would be nice if u<N> was converted to
> > uint<N>_t.
> > 
> >> --- a/xen/common/sched/core.c
> >> +++ b/xen/common/sched/core.c
> >> @@ -360,7 +360,7 @@ static always_inline void sched_spin_lock_double(
> >>       {
> >>           *flags = _spin_lock_irqsave(lock1);
> >>       }
> >> -    else if ( lock1 < lock2 )
> >> +    else if ( (uintptr_t)lock1 < (uintptr_t)lock2 )
> > 
> > Similarly, no matter what C or Misra may have to say here, imo such casts are
> > simply dangerous. Especially when open-coded.
> Well, this function 'sched_spin_lock_double' has the following description:
> "If locks are different, take the one with the lower address first."
> 
> I think it's save to compare the integer representations of 'lock1' and 
> 'lock2' addresses explicitly (casting the pointers values to an integer 
> type). We need to find the 'lower address'.
> Any risks here?
> 
> Dmytro
> > 
> >> --- a/xen/common/virtual_region.c
> >> +++ b/xen/common/virtual_region.c
> >> @@ -83,8 +83,8 @@ const struct virtual_region *find_text_region(unsigned long addr)
> >>       rcu_read_lock(&rcu_virtual_region_lock);
> >>       list_for_each_entry_rcu ( iter, &virtual_region_list, list )
> >>       {
> >> -        if ( (void *)addr >= iter->text_start &&
> >> -             (void *)addr <  iter->text_end )
> >> +        if ( addr >= (unsigned long)iter->text_start &&
> >> +             addr <  (unsigned long)iter->text_end )
> > 
> > Considering further casts to unsigned long of the same struct fields, was it
> > considered to alter the type of the struct fields instead?
> There are present casts of struct fields 'text_start' and 'text_end' in 
> the file 'xen/common/virtual_region.c'.
> 
>          modify_xen_mappings_lite((unsigned long)region->text_start,
>                                   (unsigned long)region->text_end,
>                                   PAGE_HYPERVISOR_RWX);
> 
> Changing fields type to 'unsigned long' will give the opportunity to 
> remove casts from source code (mentioned before),
> and remove '(void*)' casts from here:
> 
>          if ( (void *)addr >= iter->text_start &&
>               (void *)addr <  iter->text_end )
> 
> Unfortunately there are places where these fields are initialized, so 
> cast to the 'unsigned long' should be there.
> Example:
>      .text_start = _sinittext,
>      .text_end = _einittext,
> and
>      .text_start = _sinittext,
>      .text_end = _einittext,
> 
> where
>      extern char _sinittext[], _einittext[];
>      extern char _stext[], _etext[];
> 

Everything related to stext/etext, sinittext/einittext, etc should be
deviated as those are not even pointers: they are linker symbols. Also,
they do refer to the same "object": the Xen text.

On 7/30/25 01:09, Stefano Stabellini wrote:
> On Fri, 25 Jul 2025, Dmytro Prokopchuk1 wrote:
>> On 7/23/25 16:58, Jan Beulich wrote:
>>> On 23.07.2025 12:12, Dmytro Prokopchuk1 wrote:
>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>> @@ -568,6 +568,14 @@ C99 Undefined Behaviour 45: Pointers that do not point into, or just beyond, the
>>>>    -config=MC3A2.R18.2,reports+={safe, "any_area(any_loc(any_exp(macro(^page_to_mfn$))))"}
>>>>    -doc_end
>>>>    
>>>> +-doc_begin="Consider relational pointer comparisons in kernel-related sections as safe and compliant."
>>>> +-config=MC3R1.R18.3,reports+={safe, "any_area(any_loc(any_exp(macro(name(is_kernel||is_kernel_text||is_kernel_rodata||is_kernel_inittext)))))"}
>>>> +-doc_end
>>>> +
>>>> +-doc_begin="Allow deviations for pointer comparisons in BUG_ON and ASSERT macros, treating them as safe for debugging and validation."
>>>> +-config=MC3R1.R18.3,reports+={safe, "any_area(any_loc(any_exp(macro(name(BUG_ON||ASSERT)))))"}
>>>> +-doc_end
>>>
>>> Nit: Drop "deviations for" from the verbal description?
>> Ok.
>>
>>>
>>>> --- a/xen/arch/x86/efi/efi-boot.h
>>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>>> @@ -461,7 +461,8 @@ static void __init efi_arch_edd(void)
>>>>                    params->device_path_info_length =
>>>>                        sizeof(struct edd_device_params) -
>>>>                        offsetof(struct edd_device_params, key);
>>>> -                for ( p = (const u8 *)&params->key; p < &params->checksum; ++p )
>>>> +                for ( p = (const u8 *)&params->key;
>>>> +                      (uintptr_t)p < (uintptr_t)&params->checksum; ++p )
>>>
>>> There mere addition of such casts makes code more fragile imo. What about the
>>> alternative of using != instead of < here? I certainly prefer < in such situations,
>>> but functionally != ought to be equivalent (and less constrained by C and Misra).
>>>
>>> As mentioned several times when discussing these rules, it's also not easy to see
>>> how "pointers of different objects" could be involved here: It's all within
>>> *params, isn't it?
>> Hard to say something. Let's hold this so far.
>>
>>>
>>> Finally, when touching such code it would be nice if u<N> was converted to
>>> uint<N>_t.
>>>
>>>> --- a/xen/common/sched/core.c
>>>> +++ b/xen/common/sched/core.c
>>>> @@ -360,7 +360,7 @@ static always_inline void sched_spin_lock_double(
>>>>        {
>>>>            *flags = _spin_lock_irqsave(lock1);
>>>>        }
>>>> -    else if ( lock1 < lock2 )
>>>> +    else if ( (uintptr_t)lock1 < (uintptr_t)lock2 )

Could we assume that these 'lock1' and 'lock2' pointers belong to the 
same allocation region - 'sched_resource' ?

Dmytro.

>>>
>>> Similarly, no matter what C or Misra may have to say here, imo such casts are
>>> simply dangerous. Especially when open-coded.
>> Well, this function 'sched_spin_lock_double' has the following description:
>> "If locks are different, take the one with the lower address first."
>>
>> I think it's save to compare the integer representations of 'lock1' and
>> 'lock2' addresses explicitly (casting the pointers values to an integer
>> type). We need to find the 'lower address'.
>> Any risks here?
>>
>> Dmytro
>>>
>>>> --- a/xen/common/virtual_region.c
>>>> +++ b/xen/common/virtual_region.c
>>>> @@ -83,8 +83,8 @@ const struct virtual_region *find_text_region(unsigned long addr)
>>>>        rcu_read_lock(&rcu_virtual_region_lock);
>>>>        list_for_each_entry_rcu ( iter, &virtual_region_list, list )
>>>>        {
>>>> -        if ( (void *)addr >= iter->text_start &&
>>>> -             (void *)addr <  iter->text_end )
>>>> +        if ( addr >= (unsigned long)iter->text_start &&
>>>> +             addr <  (unsigned long)iter->text_end )
>>>
>>> Considering further casts to unsigned long of the same struct fields, was it
>>> considered to alter the type of the struct fields instead?
>> There are present casts of struct fields 'text_start' and 'text_end' in
>> the file 'xen/common/virtual_region.c'.
>>
>>           modify_xen_mappings_lite((unsigned long)region->text_start,
>>                                    (unsigned long)region->text_end,
>>                                    PAGE_HYPERVISOR_RWX);
>>
>> Changing fields type to 'unsigned long' will give the opportunity to
>> remove casts from source code (mentioned before),
>> and remove '(void*)' casts from here:
>>
>>           if ( (void *)addr >= iter->text_start &&
>>                (void *)addr <  iter->text_end )
>>
>> Unfortunately there are places where these fields are initialized, so
>> cast to the 'unsigned long' should be there.
>> Example:
>>       .text_start = _sinittext,
>>       .text_end = _einittext,
>> and
>>       .text_start = _sinittext,
>>       .text_end = _einittext,
>>
>> where
>>       extern char _sinittext[], _einittext[];
>>       extern char _stext[], _etext[];
>>
> 
> Everything related to stext/etext, sinittext/einittext, etc should be
> deviated as those are not even pointers: they are linker symbols. Also,
> they do refer to the same "object": the Xen text.

On 20.08.25 11:18, Dmytro Prokopchuk1 wrote:
> 
> 
> On 7/30/25 01:09, Stefano Stabellini wrote:
>> On Fri, 25 Jul 2025, Dmytro Prokopchuk1 wrote:
>>> On 7/23/25 16:58, Jan Beulich wrote:
>>>> On 23.07.2025 12:12, Dmytro Prokopchuk1 wrote:
>>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>> @@ -568,6 +568,14 @@ C99 Undefined Behaviour 45: Pointers that do not point into, or just beyond, the
>>>>>     -config=MC3A2.R18.2,reports+={safe, "any_area(any_loc(any_exp(macro(^page_to_mfn$))))"}
>>>>>     -doc_end
>>>>>     
>>>>> +-doc_begin="Consider relational pointer comparisons in kernel-related sections as safe and compliant."
>>>>> +-config=MC3R1.R18.3,reports+={safe, "any_area(any_loc(any_exp(macro(name(is_kernel||is_kernel_text||is_kernel_rodata||is_kernel_inittext)))))"}
>>>>> +-doc_end
>>>>> +
>>>>> +-doc_begin="Allow deviations for pointer comparisons in BUG_ON and ASSERT macros, treating them as safe for debugging and validation."
>>>>> +-config=MC3R1.R18.3,reports+={safe, "any_area(any_loc(any_exp(macro(name(BUG_ON||ASSERT)))))"}
>>>>> +-doc_end
>>>>
>>>> Nit: Drop "deviations for" from the verbal description?
>>> Ok.
>>>
>>>>
>>>>> --- a/xen/arch/x86/efi/efi-boot.h
>>>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>>>> @@ -461,7 +461,8 @@ static void __init efi_arch_edd(void)
>>>>>                     params->device_path_info_length =
>>>>>                         sizeof(struct edd_device_params) -
>>>>>                         offsetof(struct edd_device_params, key);
>>>>> -                for ( p = (const u8 *)&params->key; p < &params->checksum; ++p )
>>>>> +                for ( p = (const u8 *)&params->key;
>>>>> +                      (uintptr_t)p < (uintptr_t)&params->checksum; ++p )
>>>>
>>>> There mere addition of such casts makes code more fragile imo. What about the
>>>> alternative of using != instead of < here? I certainly prefer < in such situations,
>>>> but functionally != ought to be equivalent (and less constrained by C and Misra).
>>>>
>>>> As mentioned several times when discussing these rules, it's also not easy to see
>>>> how "pointers of different objects" could be involved here: It's all within
>>>> *params, isn't it?
>>> Hard to say something. Let's hold this so far.
>>>
>>>>
>>>> Finally, when touching such code it would be nice if u<N> was converted to
>>>> uint<N>_t.
>>>>
>>>>> --- a/xen/common/sched/core.c
>>>>> +++ b/xen/common/sched/core.c
>>>>> @@ -360,7 +360,7 @@ static always_inline void sched_spin_lock_double(
>>>>>         {
>>>>>             *flags = _spin_lock_irqsave(lock1);
>>>>>         }
>>>>> -    else if ( lock1 < lock2 )
>>>>> +    else if ( (uintptr_t)lock1 < (uintptr_t)lock2 )
> 
> Could we assume that these 'lock1' and 'lock2' pointers belong to the
> same allocation region - 'sched_resource' ?

No, they can be either in sched_resource, in a per-scheduler private memory
area, or even in the .data section of the hypervisor.


Juergen

On 25.07.2025 23:34, Dmytro Prokopchuk1 wrote:
> On 7/23/25 16:58, Jan Beulich wrote:
>> On 23.07.2025 12:12, Dmytro Prokopchuk1 wrote:
>>> --- a/xen/common/sched/core.c
>>> +++ b/xen/common/sched/core.c
>>> @@ -360,7 +360,7 @@ static always_inline void sched_spin_lock_double(
>>>       {
>>>           *flags = _spin_lock_irqsave(lock1);
>>>       }
>>> -    else if ( lock1 < lock2 )
>>> +    else if ( (uintptr_t)lock1 < (uintptr_t)lock2 )
>>
>> Similarly, no matter what C or Misra may have to say here, imo such casts are
>> simply dangerous. Especially when open-coded.
> Well, this function 'sched_spin_lock_double' has the following description:
> "If locks are different, take the one with the lower address first."
> 
> I think it's save to compare the integer representations of 'lock1' and 
> 'lock2' addresses explicitly (casting the pointers values to an integer 
> type). We need to find the 'lower address'.
> Any risks here?

These instances of casts are of comparably little risk, yes, but almost every
cast comes with some risk. If only to set a bad precedent that someone the
more or less blindly copies.

But in the end it'll be the scheduler maintainers to judge here.

>>> --- a/xen/common/virtual_region.c
>>> +++ b/xen/common/virtual_region.c
>>> @@ -83,8 +83,8 @@ const struct virtual_region *find_text_region(unsigned long addr)
>>>       rcu_read_lock(&rcu_virtual_region_lock);
>>>       list_for_each_entry_rcu ( iter, &virtual_region_list, list )
>>>       {
>>> -        if ( (void *)addr >= iter->text_start &&
>>> -             (void *)addr <  iter->text_end )
>>> +        if ( addr >= (unsigned long)iter->text_start &&
>>> +             addr <  (unsigned long)iter->text_end )
>>
>> Considering further casts to unsigned long of the same struct fields, was it
>> considered to alter the type of the struct fields instead?
> There are present casts of struct fields 'text_start' and 'text_end' in 
> the file 'xen/common/virtual_region.c'.
> 
>          modify_xen_mappings_lite((unsigned long)region->text_start,
>                                   (unsigned long)region->text_end,
>                                   PAGE_HYPERVISOR_RWX);
> 
> Changing fields type to 'unsigned long' will give the opportunity to 
> remove casts from source code (mentioned before),
> and remove '(void*)' casts from here:
> 
>          if ( (void *)addr >= iter->text_start &&
>               (void *)addr <  iter->text_end )
> 
> Unfortunately there are places where these fields are initialized, so 
> cast to the 'unsigned long' should be there.
> Example:
>      .text_start = _sinittext,
>      .text_end = _einittext,
> and
>      .text_start = _sinittext,
>      .text_end = _einittext,
> 
> where
>      extern char _sinittext[], _einittext[];
>      extern char _stext[], _etext[];
> 
> So, my proposition is to add cast to 'unsigned long' as I proposed in 
> this patch.

My take is that the solution with, ultimately, fewer casts overall wants using.
Plus my personal view is that casts in initializers are a little less "bad".

Jan

On 7/28/25 11:06, Jan Beulich wrote:
> On 25.07.2025 23:34, Dmytro Prokopchuk1 wrote:
>> On 7/23/25 16:58, Jan Beulich wrote:
>>> On 23.07.2025 12:12, Dmytro Prokopchuk1 wrote:
>>>> --- a/xen/common/sched/core.c
>>>> +++ b/xen/common/sched/core.c
>>>> @@ -360,7 +360,7 @@ static always_inline void sched_spin_lock_double(
>>>>        {
>>>>            *flags = _spin_lock_irqsave(lock1);
>>>>        }
>>>> -    else if ( lock1 < lock2 )
>>>> +    else if ( (uintptr_t)lock1 < (uintptr_t)lock2 )
>>>
>>> Similarly, no matter what C or Misra may have to say here, imo such casts are
>>> simply dangerous. Especially when open-coded.
>> Well, this function 'sched_spin_lock_double' has the following description:
>> "If locks are different, take the one with the lower address first."
>>
>> I think it's save to compare the integer representations of 'lock1' and
>> 'lock2' addresses explicitly (casting the pointers values to an integer
>> type). We need to find the 'lower address'.
>> Any risks here?
> 
> These instances of casts are of comparably little risk, yes, but almost every
> cast comes with some risk. If only to set a bad precedent that someone the
> more or less blindly copies.
> 
> But in the end it'll be the scheduler maintainers to judge here.
> 
>>>> --- a/xen/common/virtual_region.c
>>>> +++ b/xen/common/virtual_region.c
>>>> @@ -83,8 +83,8 @@ const struct virtual_region *find_text_region(unsigned long addr)
>>>>        rcu_read_lock(&rcu_virtual_region_lock);
>>>>        list_for_each_entry_rcu ( iter, &virtual_region_list, list )
>>>>        {
>>>> -        if ( (void *)addr >= iter->text_start &&
>>>> -             (void *)addr <  iter->text_end )
>>>> +        if ( addr >= (unsigned long)iter->text_start &&
>>>> +             addr <  (unsigned long)iter->text_end )
>>>
>>> Considering further casts to unsigned long of the same struct fields, was it
>>> considered to alter the type of the struct fields instead?
>> There are present casts of struct fields 'text_start' and 'text_end' in
>> the file 'xen/common/virtual_region.c'.
>>
>>           modify_xen_mappings_lite((unsigned long)region->text_start,
>>                                    (unsigned long)region->text_end,
>>                                    PAGE_HYPERVISOR_RWX);
>>
>> Changing fields type to 'unsigned long' will give the opportunity to
>> remove casts from source code (mentioned before),
>> and remove '(void*)' casts from here:
>>
>>           if ( (void *)addr >= iter->text_start &&
>>                (void *)addr <  iter->text_end )
>>
>> Unfortunately there are places where these fields are initialized, so
>> cast to the 'unsigned long' should be there.
>> Example:
>>       .text_start = _sinittext,
>>       .text_end = _einittext,
>> and
>>       .text_start = _sinittext,
>>       .text_end = _einittext,
>>
>> where
>>       extern char _sinittext[], _einittext[];
>>       extern char _stext[], _etext[];
>>
>> So, my proposition is to add cast to 'unsigned long' as I proposed in
>> this patch.
> 
> My take is that the solution with, ultimately, fewer casts overall wants using.
> Plus my personal view is that casts in initializers are a little less "bad".
> 
> Jan

As for me the following changes look not good (if change struct item 
type to 'unsigned long').
Just code snips.

-    const void *text_start;
-    const void *text_end;
+    unsigned long    text_start;
+    unsigned long    text_end;

-    const void *rodata_start;
-    const void *rodata_end;
+    unsigned long    rodata_start;
+    unsigned long    rodata_end;


-    region->text_start = payload->text_addr;
-    region->text_end = payload->text_addr + payload->text_size;
+    region->text_start = (unsigned long)payload->text_addr;
+    region->text_end = (unsigned long)(payload->text_addr + 
payload->text_size);


-        region->rodata_start = payload->ro_addr;
-        region->rodata_end = payload->ro_addr + payload->ro_size;
+        region->rodata_start = (unsigned long)payload->ro_addr;
+        region->rodata_end = (unsigned long)(payload->ro_addr + 
payload->ro_size);


-    .text_start = _stext,
-    .text_end = _etext,
-    .rodata_start = _srodata,
-    .rodata_end = _erodata,
+    .text_start = (unsigned long)_stext,
+    .text_end = (unsigned long)_etext,
+    .rodata_start = (unsigned long)_srodata,
+    .rodata_end = (unsigned long)_erodata,


-    .text_start = _sinittext,
-    .text_end = _einittext,
+    .text_start = (unsigned long)_sinittext,
+    .text_end = (unsigned long)_einittext,


-        if ( (void *)addr >= iter->text_start &&   <<<<< Actually the 
violation was only here
-             (void *)addr <  iter->text_end )    <<<<<< and here
+        if ( addr >= iter->text_start &&
+             addr <  iter->text_end )


-        modify_xen_mappings_lite((unsigned long)region->text_start,
-                                 (unsigned long)region->text_end,
+        modify_xen_mappings_lite(region->text_start,
+                                 region->text_end,



          if ( region->rodata_start )
-            modify_xen_mappings_lite((unsigned long)region->rodata_start,
-                                     (unsigned long)region->rodata_end,
+            modify_xen_mappings_lite(region->rodata_start,
+                                     region->rodata_end,


-        modify_xen_mappings_lite((unsigned long)region->text_start,
-                                 (unsigned long)region->text_end,
+        modify_xen_mappings_lite(region->text_start,
+                                 region->text_end,


          if ( region->rodata_start )
-            modify_xen_mappings_lite((unsigned long)region->rodata_start,
-                                     (unsigned long)region->rodata_end,
+            modify_xen_mappings_lite(region->rodata_start,
+                                     region->rodata_end,


Here are run-time casts, and especially I don't like this 'if' statements:
  'if ( region->rodata_start )' and 'if ( region->rodata_start )'.

It intended to be NULL ptr check, but now it's not.
Probably it will work as expected, assuming these integer values are 
zero, but I'm not sure at all.

Dmytro.