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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.03.17 at 16:01, <jgross@xxxxxxxx> wrote:
> On 22/03/17 14:12, Jan Beulich wrote:
>>>>> On 21.03.17 at 14:10, <jgross@xxxxxxxx> wrote:
>>> @@ -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?

Either way should be fine.

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

Good idea, go ahead, albeit the hiding from assembly part is not
entirely true: Assembly code, with the single argument passed
above, would make the implication that the passed in array is no
smaller than the BIOS one.

Jan


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