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

[Juergen Gross <jgross@xxxxxxxx>]

On 22/03/17 14:12, Jan Beulich wrote:
>>>> 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).

Okay.

> 
>> @@ -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.

You are right. Will remove it.

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

Indeed.

> 
>> @@ -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)?

Yes. Would you prefer a separate patch for the other places I don't
touch yet, or should I fold it into this one?

>> --- 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.

Hmm, maybe a copy function in assembly code? Something like:

e820_raw.nr_map = copy_bios_e820(e820_raw.map);

This would hide struct e820 from assembly and e820map[] and e820nr from
C code.


Juergen

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