Re: [Crash-utility] [BUG] [compressed kdump / SADUMP] makedumpfile header truncation error

----- Original Message ----- > On 09/17/2013 09:23 PM, Dave Anderson wrote: > > > > > > ----- Original Message ----- > >> On 09/17/2013 03:33 PM, HATAYAMA Daisuke wrote: > >>> (2013/09/17 16:12), Jingbai Ma wrote: > >>>> On 09/17/2013 02:55 PM, HATAYAMA Daisuke wrote: > >>>> > >>>> int32_t, int64_t, uint64_t, etc ... are parts of C99 standard: > >>>> http://en.wikipedia.org/wiki/C_data_types > >>>> All there types have been supported by GCC, so them should work on >all > >>>> the architectures. > >>>> > >>>> Although change these persistent data structure will affect both > >>>> makedumpfile and crash utility, but we will benefit from the > >>>> consistent data structures independent from architectures. We can > >>>> analyze a dumpfile on a OS with different architecture than the > >>>> crashed OS. > >>>> > >>>> > >>> > >>> I know stdint.h things and usefulness if we can use crash and >makedumpfile > >>> for a multiple architectures on single arch. In fact, crash already >supports > >>> cross platform build among some architectures thanks to Dave. > > > > But only if the host and target architectures have the same >endian-ness and > > whose data type sizes match. > > > > If we have a standard for the dump file format, we can handle all the >endian-ness issues. > I know it may affect the dumping speed on the platform that has to > convert the byte order. But at least we can specify the byte order for > dump file header. It won't cost too much. > > > The only problem that has ever been seen with the current header >declarations > > is if an x86 crash binary is built to support the 32-bit ARM >architecture. > > For x86, the 64-bit off_t variables can start on a 4-byte boundary, >but on ARM, > > they have to start on an 8-byte boundary. That being the case, the >off_t > > offset_vmcoreinfo is at offset 20 when built on an x86, and at offset >24 > > when built on ARM: > > This could be addressed through compiler attributes: > off_t offset_vmcoreinfo __atttribute__ ((aligned(8)); > offset_vmcoreinfo will be the same 8-byte boundary on x86 as same as ARM > > > > > struct kdump_sub_header { > > unsigned long phys_base; > > int dump_level; /* header_version 1 and >later */ > > int split; /* header_version 2 and >later */ > > unsigned long start_pfn; /* header_version 2 and >later */ > > unsigned long end_pfn; /* header_version 2 and >later */ > > off_t offset_vmcoreinfo; /* header_version 3 and >later */ > > unsigned long size_vmcoreinfo; /* header_version 3 and >later */ > > off_t offset_note; /* header_version 4 and >later */ > > unsigned long size_note; /* header_version 4 and >later */ > > off_t offset_eraseinfo; /* header_version 5 and >later */ > > unsigned long size_eraseinfo; /* header_version 5 and >later */ > > }; > > > > Do you like this change? > struct kdump_sub_header { > unsigned long phys_base; > int dump_level; > int split; > unsigned long start_pfn; > unsigned long end_pfn; > off_t offset_vmcoreinfo __atttribute__ ((aligned(8)); > unsigned long size_vmcoreinfo; > off_t offset_note __atttribute__ ((aligned(8)); > unsigned long size_note; > off_t offset_eraseinfo __atttribute__ ((aligned(8)); > unsigned long size_eraseinfo; > }; > > Then you can get rid of the padded struct kdump_sub_header_ARM_target in > crash utility. Adding the aligned(8) attribute to the kdump_sub_header would break compatibility with all of the old/current 32-bit x86 dumpfiles that have it aligned on an 4-byte boundary. How do you propose working around that? > > Or we can go further, redefine whole structure and set all fields with > specific bit width. > > struct kdump_sub_header { > uint64_t phys_base; > int32_t dump_level; > int32_t split; > uint64_t start_pfn; > uint64_t end_pfn; > uint64_t offset_vmcoreinfo; > uint64_t size_vmcoreinfo; > uint64_t offset_note; > uint64_t size_note; > uint64_t offset_eraseinfo; > uint64_t size_eraseinfo; > }; > > I have checked the code of crash utility, it shouldn't affect too much, > only in diskdump.c and diskdump.h. > > > So for that anomoly, crash has to support a >kdump_sub_header_ARM_target > > structure that has a pad integer after the end_pfn variable. > > > >>> > >>> My question came from the fact that it looks like you introduced a >single > >>> modified kdump_sub_header structure for all the architectures. They >might > >>> have different combination of length between int and long and maybe > >>> also have other each architecture specific incompatibility. It >wouldn't > >>> work well. > >>> > >>> But from your reply, I think you mean a fully new header for >kdump-compressed > >>> format, right? If so, it must work well. But of course you need to >modify > >>> both of makedumpfile and crash utility to support it. > >>> > >> > >> Yes, I would like to have a new header for kdump-compressed format. >But > >> I'm not sure how much code will be affected in makedumpfile and >crash utility. > >> I'm still under investigating, any ideas would be appreciated. > > > > The challenging part will be the requirement to maintain >backwards-compatibility, > > at least in the crash utility. And backwards-compatibility would >also be required > > in makedumpfile, right? For example, if you want to re-filter an >older compressed > > kdump. > > > > It's not a big deal, we can check the header_version to decide treat it > as traditional format or new format. > We can preserve the current structures as kdump_sub_header_v5 , > kdump_sub_header_v5, etc... in both makedumpfile and crash utility. OK, but I still don't see how to avoid carrying two versions of kdump_sub_header_v5 to handle the current ARM-on-x86 support. Or doing some kind of similar kludge... Supporting both a kdump_sub_header and a kdump_sub_header_v5 is going to make read_dump_header() a bit tricky. I suppose after reading the raw kdump_sub_header block (of either type), if it's v5 or less you could copy the individual fields from the kdump_sub_header_v5 to the new kdump_sub_header before referencing them?

> > But if -- as has been done so far -- an increment of the >header_version in the > > disk_dump_header to signal an additional field in the >kdump_sub_header would be > > trivial to implement. > > Yes, this approach is more simpler, but the drawback is we have to add a > new 64bit max_mapnr_64 to disk_dump_header, then we will have two > max_mapnr* fields, not very nice. And when we add more platforms, we > still have to take care of the bit width and alignment. > Should we fix it in this version or just leave it as it used to be? Note that I suggested above that the 64-bit max_mapnr be added to the kdump_sub_header, so that the disk_dump_header itself can remain backwards-compatible.

Dave