Re: [XEN PATCH v2 1/5] x86: address MISRA C:2012 Rule 5.3

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 08/08/2023 15:46, Jan Beulich wrote:
> On 08.08.2023 13:08, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/e820.c
>> +++ b/xen/arch/x86/e820.c
>> @@ -543,27 +543,27 @@ static void __init 
>> machine_specific_memory_setup(struct e820map *raw)
>>          clip_to_limit(top_of_ram, "MTRRs do not cover all of 
>> memory.");
>>  }
>> 
>> -/* This function relies on the passed in e820->map[] being sorted. */
>> -int __init e820_add_range(
>> -    struct e820map *e820, uint64_t s, uint64_t e, uint32_t type)
>> +/* This function relies on the global e820->map[] being sorted. */
>> +int __init e820_add_range(uint64_t s, uint64_t e, uint32_t type)
>>  {
>>      unsigned int i;
>> +    struct e820entry *ei = e820.map;
>> 
>> -    for ( i = 0; i < e820->nr_map; ++i )
>> +    for ( i = 0; i < e820.nr_map; ++i )
>>      {
>> -        uint64_t rs = e820->map[i].addr;
>> -        uint64_t re = rs + e820->map[i].size;
>> +        uint64_t rs = ei[i].addr;
>> +        uint64_t re = rs + ei[i].size;
>> 
>> -        if ( rs == e && e820->map[i].type == type )
>> +        if ( rs == e && ei[i].type == type )
>>          {
>> -            e820->map[i].addr = s;
>> +            ei[i].addr = s;
>>              return 1;
>>          }
>> 
>> -        if ( re == s && e820->map[i].type == type &&
>> -             (i + 1 == e820->nr_map || e820->map[i + 1].addr >= e) )
>> +        if ( re == s && ei[i].type == type &&
>> +             (i + 1 == e820.nr_map || ei[i + 1].addr >= e) )
>>          {
>> -            e820->map[i].size += e - s;
>> +            ei[i].size += e - s;
>>              return 1;
>>          }
>> 
>> @@ -574,20 +574,20 @@ int __init e820_add_range(
>>              return 0;
>>      }
>> 
>> -    if ( e820->nr_map >= ARRAY_SIZE(e820->map) )
>> +    if ( e820.nr_map >= ARRAY_SIZE(e820.map) )
>>      {
>>          printk(XENLOG_WARNING "E820: overflow while adding region"
>>                 " %"PRIx64"-%"PRIx64"\n", s, e);
>>          return 0;
>>      }
>> 
>> -    memmove(e820->map + i + 1, e820->map + i,
>> -            (e820->nr_map - i) * sizeof(*e820->map));
>> +    memmove(ei + i + 1, ei + i,
>> +            (e820.nr_map - i) * sizeof(*e820.map));
>> 
>> -    e820->nr_map++;
>> -    e820->map[i].addr = s;
>> -    e820->map[i].size = e - s;
>> -    e820->map[i].type = type;
>> +    e820.nr_map++;
>> +    ei[i].addr = s;
>> +    ei[i].size = e - s;
>> +    ei[i].type = type;
>> 
>>      return 1;
>>  }
> 
> To be honest this isn't quite what I was hoping for; the many ei[i]. 
> are
> (imo) quite a bit harder to read than ei-> would have been (hence my
> earlier suggestion to also update that pointer in the for() loop 
> header).
> Then again I see there is one use of ei[i + 1], which would likely look
> less neat as ei[1].addr when everywhere else we have ei->. So I guess 
> up
> to you whether you adjust further; I'll ack either form.
> 

I'll leave it as is.

>> --- a/xen/arch/x86/guest/hypervisor.c
>> +++ b/xen/arch/x86/guest/hypervisor.c
>> @@ -63,7 +63,7 @@ void hypervisor_resume(void)
>>  void __init hypervisor_e820_fixup(struct e820map *e820)
> 
> What about this one? The function parameter ...
> 
>>  {
>>      if ( ops.e820_fixup )
>> -        ops.e820_fixup(e820);
>> +        ops.e820_fixup();
>>  }
> 
> ... isn't used anymore, and the sole call site passes &e820.
> 

It remained there by accident.

>> --- a/xen/arch/x86/include/asm/e820.h
>> +++ b/xen/arch/x86/include/asm/e820.h
>> @@ -29,8 +29,7 @@ extern int reserve_e820_ram(struct e820map *e820, 
>> uint64_t s, uint64_t e);
>>  extern int e820_change_range_type(
>>      struct e820map *e820, uint64_t s, uint64_t e,
>>      uint32_t orig_type, uint32_t new_type);
> 
> And what about this one? None of the other subjects in the series 
> suggest
> this is then taken care of in a separate patch (as per the earlier
> discussion it indeed doesn't want dealing with right here).
> 

I'll mention this detail. While I work on other rules I'll think of a 
good way to rename.

>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -686,7 +686,7 @@ static void __init parse_video_info(void)
>>  #endif
>>  }
>> 
>> -static void __init kexec_reserve_area(struct e820map *e820)
>> +static void __init kexec_reserve_area(void)
>>  {
>>  #ifdef CONFIG_KEXEC
>>      unsigned long kdump_start = kexec_crash_area.start;
>> @@ -700,7 +700,7 @@ static void __init kexec_reserve_area(struct 
>> e820map *e820)
>> 
>>      is_reserved = true;
>> 
>> -    if ( !reserve_e820_ram(e820, kdump_start, kdump_start + 
>> kdump_size) )
>> +    if ( !reserve_e820_ram(&boot_e820, kdump_start, kdump_start + 
>> kdump_size) )
>>      {
>>          printk("Kdump: DISABLED (failed to reserve %luMB (%lukB) at 
>> %#lx)"
>>                 "\n", kdump_size >> 20, kdump_size >> 10, 
>> kdump_start);
>> @@ -1308,7 +1308,7 @@ void __init noreturn __start_xen(unsigned long 
>> mbi_p)
>>          if ( e820.map[i].type == E820_RAM )
>>              nr_pages += e820.map[i].size >> PAGE_SHIFT;
>>      set_kexec_crash_area_size((u64)nr_pages << PAGE_SHIFT);
>> -    kexec_reserve_area(&boot_e820);
>> +    kexec_reserve_area();
>> 
>>      initial_images = mod;
>>      nr_initial_images = mbi->mods_count;
>> @@ -1495,7 +1495,7 @@ void __init noreturn __start_xen(unsigned long 
>> mbi_p)
>>          reserve_e820_ram(&boot_e820, __pa(_stext), 
>> __pa(__2M_rwdata_end));
>> 
>>      /* Late kexec reservation (dynamic start address). */
>> -    kexec_reserve_area(&boot_e820);
>> +    kexec_reserve_area();
>> 
>>      setup_max_pdx(raw_max_page);
>>      if ( highmem_start )
> 
> Seeing all the knock-on effects for the add_range() change, I think 
> this
> separate adjustment would better have been an independent patch.
> 
> Jan

I can submit it standalone and put together x86/vmsi and delay

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)