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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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)