Re: ASSERT in rangeset_remove_range

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.11.2021 12:58, Julien Grall wrote:
> On 08/11/2021 22:45, Stefano Stabellini wrote:
>> I discovered a bug caused by the recent changes to introduce extended
>> regions in make_hypervisor_node (more logs appended):
>>
>>
>> (XEN) d1 BANK[0] 0x00000040000000-0x0000007e800000 (1000MB)
>> (XEN) d1 BANK[1] 0x00000200000000-0x00000200000000 (0MB)
>> (XEN) DEBUG find_unallocated_memory 994 s=40000000 e=7e7fffff
>> (XEN) DEBUG find_unallocated_memory 994 s=200000000 e=1ffffffff
>> (XEN) Assertion 's <= e' failed at rangeset.c:189
>>
>>
>> When a bank of memory is zero in size, then rangeset_remove_range is
>> called with end < start, triggering an ASSERT in rangeset_remove_range.
>>
>> One solution is to avoid creating 0 size banks. The following change
>> does that:
>>
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 49b4eb2b13..3efe542d0f 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -459,9 +459,12 @@ static void __init allocate_memory(struct domain *d, 
>> struct kernel_info *kinfo)
>>           goto fail;
>>   
>>       bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem);
>> -    if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
>> +    if ( bank_size > 0 )
>> +    {
>> +        if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
>>                                  bank_size) )
>> -        goto fail;
>> +            goto fail;
>> +    }
> 
> I would move the size check in allocate_bank_memory().
> 
>>   
>>       if ( kinfo->unassigned_mem )
>>           goto fail;
>>
>>
>>
>> We have a couple of other options too:
>>
>> - remove the ASSERT in rangeset_remove_range
>> There is an argument that we should simply return error
>> fromrangeset_remove_range, rather than a full assert.
> 
> To be honest, this is a developper mistake to call with end < start. If 
> we were going to return an error then we would completely hide (even in 
> developper) it because we would fallback to not exposing extended regions.
> 
> So I am not sure switch from ASSERT() to a plain check is a good idea. 
> Jan, Andrew, Wei, what do you think?

There might be reasons to convert, but I don't think the situation here
warrants doing so.

Jan