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
