[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: ASSERT in rangeset_remove_range
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |