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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

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

~Andrew

>
>> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Let me know whether this stands even without making the
> requested change.
>
> Jan
>


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