Re: [Xen-devel] [PATCH] page_alloc: clear nr_bootmem_regions in end_boot_allocator()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.02.17 at 12:38, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 02/02/17 16:20, Jan Beulich wrote:
>>>>> On 02.02.17 at 16:41, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 02/02/17 15:25, Jan Beulich wrote:
>>>> --- a/xen/common/page_alloc.c
>>>> +++ b/xen/common/page_alloc.c
>>>> @@ -329,13 +329,16 @@ unsigned long __init alloc_boot_pages(
>>>>      unsigned long nr_pfns, unsigned long pfn_align)
>>>>  {
>>>>      unsigned long pg, _e;
>>>> -    int i;
>>>> +    unsigned int i = nr_bootmem_regions;
>>>>  
>>>> -    for ( i = nr_bootmem_regions - 1; i >= 0; i-- )
>>>> +    BOOT_BUG_ON(!nr_bootmem_regions);
>>> Can this just be a plain BUG_ON() to avoid adding further work which
>>> needs to undone for livepatching purposes?
>> Well, for one I don't like adding inconsistency here. And then I'm
>> not convinced switching over to BUG_ON() is a good idea, so I'd
>> rather leave that discussion for when someone indeed wants to
>> make that change. In particular I'm not convinced that during
>> very early boot all the register and stack dumping functions
>> reliably, in which case a simple panic() is more likely to produce
>> at least no confusing output.
> 
> Well - the change is definitely needed.  BOOT_BUG_ON() has an embedded
> __LINE__ which causes problems making livepatches.

There are ways other than converting to BUG_ON() to deal with
that, e.g. allowing a non-ambiguous string to be handed (e.g.
ASSERT()-like) and included in the panic() output.

> The early register/stack functions should work correctly.  I did test
> that when rearranging the x86 IDT handling several releases ago.

What about page walks? And ARM?

> As to consistency, I would prefer if the situation wasn't made worse,
> but if you really insist, then my R-by stands.

Thanks.

Jan


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