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

[Juergen Gross <jgross@xxxxxxxx>]

On 22/03/17 16:23, Jan Beulich wrote:
>>>> 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.

Hmm, passing the maximum number of entries as a second parameter isn't
really rocket science. :-)


Juergen


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