Re: [PATCH] misra: address MISRA C Rule 18.3 compliance

[Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>]

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.