| Message ID | c4af21ba365767f88a2516719dd0f3b70777b549.1468970114.git.geoff@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, Am Dienstag, 19 Juli 2016, 23:28:13 schrieb Geoff Levand: > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > The end address of "reg" attribute in device tree's memory should be > inclusive. Actually, there's a bug/inconsistency in kexec-tools right now. crashdump-arm.c expect usablemem_rgns.ranges[i].end to be the last byte in the range, but crashdump-powerpc.c, crashdump-ppc64.c and fs2dt.c expect it to be the first byte after the range.
On Wed, Jul 27, 2016 at 07:45:13PM -0300, Thiago Jung Bauermann wrote: > Hello, > > Am Dienstag, 19 Juli 2016, 23:28:13 schrieb Geoff Levand: > > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > The end address of "reg" attribute in device tree's memory should be > > inclusive. > > Actually, there's a bug/inconsistency in kexec-tools right now. > > crashdump-arm.c expect usablemem_rgns.ranges[i].end to be the last byte in > the range, but crashdump-powerpc.c, crashdump-ppc64.c and fs2dt.c expect it > to be the first byte after the range. Well, ARM (and the generic code I introduced for mem_ranges) follows what i386, ia64, mips, s390, and sh all do with struct memory_range when used for crashdump. It is extremely bad for a project to have a single structure used inconsistently like this - even with generic helpers, you can't be sure that the right helpers are used on the right structures, and it will lead to off-by-one errors all over the place. Just don't pull crap like this, it's asking for trouble - settle on one way and stick to it. Given that the majority of architectures treat .end as inclusive, I think ppc* and fs2dt need to conform to the convention establised by the other architectures for this structure. It's also interesting to note that crashdump-powerpc has this: /* On powerpc memory ranges in device-tree is denoted as start * and size rather than start and end, as is the case with * other architectures like i386 . Because of this when loading * the memory ranges in crashdump-elf.c the filesz calculation * [ end - start + 1 ] goes for a toss. * * To be in sync with other archs adjust the end value for * every crash memory range before calling the generic function */ for (i = 0; i < nr_ranges; i++) { end = crash_memory_range[i].end - 1; crash_memory_range[i].end = end; } to convert from powerpc end-exclusive to end-inclusive mode - something that could be killed if it were to conform throughout to the established convention. Same goes for ppc64 too: /* On ppc64 memory ranges in device-tree is denoted as start * and size rather than start and end, as is the case with * other architectures like i386 . Because of this when loading * the memory ranges in crashdump-elf.c the filesz calculation * [ end - start + 1 ] goes for a toss. * * To be in sync with other archs adjust the end value for * every crash memory range before calling the generic function */ for (i = 0; i < nr_ranges; i++) { end = crash_memory_range[i].end - 1; crash_memory_range[i].end = end; }
Am Donnerstag, 28 Juli 2016, 00:23:31 schrieb Russell King - ARM Linux: > On Wed, Jul 27, 2016 at 07:45:13PM -0300, Thiago Jung Bauermann wrote: > > Hello, > > > > Am Dienstag, 19 Juli 2016, 23:28:13 schrieb Geoff Levand: > > > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > The end address of "reg" attribute in device tree's memory should be > > > inclusive. > > > > Actually, there's a bug/inconsistency in kexec-tools right now. > > > > crashdump-arm.c expect usablemem_rgns.ranges[i].end to be the last byte > > in the range, but crashdump-powerpc.c, crashdump-ppc64.c and fs2dt.c > > expect it to be the first byte after the range. > > Well, ARM (and the generic code I introduced for mem_ranges) follows > what i386, ia64, mips, s390, and sh all do with struct memory_range > when used for crashdump. > > It is extremely bad for a project to have a single structure used > inconsistently like this - even with generic helpers, you can't be > sure that the right helpers are used on the right structures, and > it will lead to off-by-one errors all over the place. Just don't > pull crap like this, it's asking for trouble - settle on one way > and stick to it. Agreed. Personally, I prefer base address and size because it's unambiguous. But as long as just one convention is used and the structure and helpers make it clear which one they expect, it doesn't matter that much. > Given that the majority of architectures treat .end as inclusive, I > think ppc* and fs2dt need to conform to the convention establised by > the other architectures for this structure. So do valid_memory_range and find_memory_range in kexec/kexec.c, which assume struct memory_range is end-exclusive too. I'm not sure about locate_hole, it seems to assume end-inclusive but it does have a line saying "size = end - start".
On Thu, Jul 28, 2016 at 08:54:55PM -0300, Thiago Jung Bauermann wrote: > Am Donnerstag, 28 Juli 2016, 00:23:31 schrieb Russell King - ARM Linux: > > Well, ARM (and the generic code I introduced for mem_ranges) follows > > what i386, ia64, mips, s390, and sh all do with struct memory_range > > when used for crashdump. > > > > It is extremely bad for a project to have a single structure used > > inconsistently like this - even with generic helpers, you can't be > > sure that the right helpers are used on the right structures, and > > it will lead to off-by-one errors all over the place. Just don't > > pull crap like this, it's asking for trouble - settle on one way > > and stick to it. > > Agreed. Personally, I prefer base address and size because it's unambiguous. > But as long as just one convention is used and the structure and helpers > make it clear which one they expect, it doesn't matter that much. Indeed. > > Given that the majority of architectures treat .end as inclusive, I > > think ppc* and fs2dt need to conform to the convention establised by > > the other architectures for this structure. > > So do valid_memory_range and find_memory_range in kexec/kexec.c, which > assume struct memory_range is end-exclusive too. I'm not sure about > locate_hole, it seems to assume end-inclusive but it does have a line saying > "size = end - start". Unfortunately, valid_memory_range() is a mess of doing this one way and the other: send = sstart + segment->memsz - 1; return valid_memory_range(info, sstart, send); ... last = base + memsz -1; if (!valid_memory_range(info, base, last)) { So, callers of valid_memory_range pass a start and inclusive end address to valid_memory_range(), and the end address becomes "send" in this function. /* Check to see if we are fully contained */ if ((mstart <= sstart) && (mend >= send)) { So, this also points to an inclusive end address for mend, but the preceding line has: && mend == info->memory_range[i+1].start which doesn't, so this is buggy because it inconsistently treats the end address as inclusive vs exclusive. find_memory_range() looks like end-exclusive. locate_hole() in one place treats it as end-inclusive while doing the merge, and end-exclusive while looking for a hole. So, these functions are a mess and need fixing.
On Fri, 2016-07-29 at 09:27 +0100, Russell King - ARM Linux wrote:
> So, these functions are a mess and need fixing.
Since this change isn't really related to arm64 support, I'll
drop this patch from my series.
-Geoff
On Fri, Jul 29, 2016 at 10:12:26AM -0700, Geoff Levand wrote: > On Fri, 2016-07-29 at 09:27 +0100, Russell King - ARM Linux wrote: > > > So, these functions are a mess and need fixing. > > Since this change isn't really related to arm64 support, I'll > drop this patch from my series. Do you have a case which triggers bugs in this code?
On Fri, Jul 29, 2016 at 06:23:56PM +0100, Russell King - ARM Linux wrote: > On Fri, Jul 29, 2016 at 10:12:26AM -0700, Geoff Levand wrote: > > On Fri, 2016-07-29 at 09:27 +0100, Russell King - ARM Linux wrote: > > > > > So, these functions are a mess and need fixing. > > > > Since this change isn't really related to arm64 support, I'll > > drop this patch from my series. > > Do you have a case which triggers bugs in this code? Actually, this patch was necessary when my kdump used "usable-memory" properties in "memory" nodes, as on ppc64, to limit the usable memory regions that can be used by crash dump kernel. Since then, I've moved to the approach of using "mem=" kernel parameter, then introducing a new property, "linux,usable-memory-range," under /chosen and now we don't need this patch any more. So, we can drop it but I still believe that it is buggy. Thanks, -Takahiro AKASHI > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
Simon, What is your opinion on this issue? On Mon, Aug 01, 2016 at 01:52:40PM +0900, AKASHI Takahiro wrote: > On Fri, Jul 29, 2016 at 06:23:56PM +0100, Russell King - ARM Linux wrote: > > On Fri, Jul 29, 2016 at 10:12:26AM -0700, Geoff Levand wrote: > > > On Fri, 2016-07-29 at 09:27 +0100, Russell King - ARM Linux wrote: > > > > > > > So, these functions are a mess and need fixing. > > > > > > Since this change isn't really related to arm64 support, I'll > > > drop this patch from my series. > > > > Do you have a case which triggers bugs in this code? > > Actually, this patch was necessary when my kdump used "usable-memory" > properties in "memory" nodes, as on ppc64, to limit the usable memory > regions that can be used by crash dump kernel. > Since then, I've moved to the approach of using "mem=" kernel parameter, > then introducing a new property, "linux,usable-memory-range," under /chosen > and now we don't need this patch any more. > > So, we can drop it but I still believe that it is buggy. Due to the discussions[1], I may want to re-enable "usable-memory" property on arm64. In addition, I would like to add a function, dtb_add_usable_memory_properties(), a variant of add_usable_memory_properties(), to kexec/dt-ops.c. So this issue is quite crucial now. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-August/452685.html -Takahiro AKASHI > Thanks, > -Takahiro AKASHI > > > -- > > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > > according to speedtest.net.
diff --git a/kexec/fs2dt.c b/kexec/fs2dt.c index 6ed2399..f00fd98 100644 --- a/kexec/fs2dt.c +++ b/kexec/fs2dt.c @@ -236,7 +236,8 @@ static void add_dyn_reconf_usable_mem_property__(int fd) ranges_size*8); } ranges[rlen++] = cpu_to_be64(loc_base); - ranges[rlen++] = cpu_to_be64(loc_end - loc_base); + ranges[rlen++] = cpu_to_be64(loc_end + - loc_base + 1); rngs_cnt++; } } @@ -350,7 +351,7 @@ static void add_usable_mem_property(int fd, size_t len) ranges_size*sizeof(*ranges)); } ranges[rlen++] = cpu_to_be64(loc_base); - ranges[rlen++] = cpu_to_be64(loc_end - loc_base); + ranges[rlen++] = cpu_to_be64(loc_end - loc_base + 1); } }