Re: [Xen-devel] [PATCH 2/3] xen/x86: use trampoline e820 buffer for BIOS interface only

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.03.17 at 14:10, <jgross@xxxxxxxx> wrote:
> @@ -133,7 +134,8 @@ static struct change_member *change_point[2*E820MAX] 
> __initdata;
>  static struct e820entry *overlap_list[E820MAX] __initdata;
>  static struct e820entry new_bios[E820MAX] __initdata;
>  
> -static int __init sanitize_e820_map(struct e820entry * biosmap, char * 
> pnr_map)
> +static int __init sanitize_e820_map(struct e820entry * biosmap,
> +                                    unsigned int * pnr_map)

Please correct style violations in code you touch anyway (here:
stray blanks after the *s).

> @@ -508,17 +510,16 @@ static void __init reserve_dmi_region(void)
>      }
>  }
>  
> -static void __init machine_specific_memory_setup(
> -    struct e820entry *raw, unsigned int *raw_nr)
> +static void __init machine_specific_memory_setup(struct e820map *raw)
>  {
>      unsigned long mpt_limit, ro_mpt_limit;
>      uint64_t top_of_ram, size;
>      int i;
>  
> -    char nr = (char)*raw_nr;
> -    sanitize_e820_map(raw, &nr);
> -    *raw_nr = nr;
> -    (void)copy_e820_map(raw, nr);
> +    unsigned int nr = raw->nr_map;
> +    sanitize_e820_map(raw->map, &nr);

And here: Missing blank line between declarations and
statements (in fact the blank line is there but misplaced). Or wait:
The variable "nr" doesn't appear to be needed at all:

    sanitize_e820_map(raw->map, &raw->nr_map);
    copy_e820_map(raw->map, raw->nr_map);

I guess it was there merely because field type and parameter
type didn't match up.

> +    raw->nr_map = nr;
> +    (void)copy_e820_map(raw->map, nr);

Stray cast.

> @@ -194,10 +194,10 @@ static void __init 
> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>              type = E820_NVS;
>              break;
>          }
> -        if ( e820nr && type == e->type &&
> +        if ( e820_raw.nr_map && type == e->type &&
>               desc->PhysicalStart == e->addr + e->size )
>              e->size += len;
> -        else if ( !len || e820nr >= E820MAX )
> +        else if ( !len || e820_raw.nr_map >= E820MAX )

ARRAY_SIZE() (also elsewhere)?

> --- a/xen/include/asm-x86/e820.h
> +++ b/xen/include/asm-x86/e820.h
> @@ -30,15 +30,16 @@ extern int e820_change_range_type(
>      uint32_t orig_type, uint32_t new_type);
>  extern int e820_add_range(
>      struct e820map *, uint64_t s, uint64_t e, uint32_t type);
> -extern unsigned long init_e820(const char *, struct e820entry *, unsigned 
> int *);
> +extern unsigned long init_e820(const char *, struct e820map *);
>  extern struct e820map e820;
> +extern struct e820map e820_raw;
>  
>  /* These symbols live in the boot trampoline. */
>  extern struct e820entry e820map[];
>  extern unsigned int e820nr;

It would be nice to restrict the visibility of these two (to be certain
there are no stray references), but that may be difficult to achieve.
One option might be to have an accessor function at the bottom of
e820.c, placing their declarations right ahead of that function. That
way only that function can validly use them. Another option would
be to drop the declarations altogether, moving the copying to
e820_raw into assembly code.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel