From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47007) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W88r6-0002Mm-N5 for qemu-devel@nongnu.org; Tue, 28 Jan 2014 08:37:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W88r0-0007Ir-Tc for qemu-devel@nongnu.org; Tue, 28 Jan 2014 08:37:32 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54328) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W88r0-0007Il-Kp for qemu-devel@nongnu.org; Tue, 28 Jan 2014 08:37:26 -0500 Message-ID: <52E7B28D.60305@redhat.com> Date: Tue, 28 Jan 2014 14:37:17 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1390890126-17377-1-git-send-email-qiaonuohan@cn.fujitsu.com> <1390890126-17377-9-git-send-email-qiaonuohan@cn.fujitsu.com> <52E799BF.4040902@linux.vnet.ibm.com> In-Reply-To: <52E799BF.4040902@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v8 08/13] dump: add API to write dump header List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ekaterina Tumanova , Qiao Nuohan Cc: Christian Borntraeger , qemu-devel@nongnu.org Hello Ekaterina, On 01/28/14 12:51, Ekaterina Tumanova wrote: > On 01/28/2014 10:22 AM, qiaonuohan wrote: >> the functions are used to write header of kdump-compressed format to >> vmcore. >> Header of kdump-compressed format includes: >> 1. common header: DiskDumpHeader32 / DiskDumpHeader64 >> 2. sub header: KdumpSubHeader32 / KdumpSubHeader64 >> 3. extra information: only elf notes here >> >> Signed-off-by: Qiao Nuohan >> Reviewed-by: Laszlo Ersek >> --- >> dump.c | 223 >> +++++++++++++++++++++++++++++++++++++++++++++++++ >> include/sysemu/dump.h | 96 +++++++++++++++++++++ >> 2 files changed, 319 insertions(+), 0 deletions(-) >> >> diff --git a/dump.c b/dump.c >> index 3a1944e..4b2799f 100644 >> --- a/dump.c >> +++ b/dump.c >> @@ -778,6 +778,229 @@ static int buf_write_note(const void *buf, >> size_t size, void *opaque) >> return 0; >> } >> >> +/* write common header, sub header and elf note to vmcore */ >> +static int create_header32(DumpState *s) >> +{ >> + int ret = 0; >> + DiskDumpHeader32 *dh = NULL; >> + KdumpSubHeader32 *kh = NULL; >> + size_t size; >> + int endian = s->dump_info.d_endian; >> + uint32_t block_size; >> + uint32_t sub_hdr_size; >> + uint32_t bitmap_blocks; >> + uint32_t status = 0; >> + uint64_t offset_note; >> + >> + /* write common header, the version of kdump-compressed format is >> 6th */ >> + size = sizeof(DiskDumpHeader32); >> + dh = g_malloc0(size); >> + >> + strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE)); >> + dh->header_version = cpu_convert_to_target32(6, endian); >> + block_size = s->page_size; >> + dh->block_size = cpu_convert_to_target32(block_size, endian); >> + sub_hdr_size = sizeof(struct KdumpSubHeader32) + s->note_size; >> + sub_hdr_size = DIV_ROUND_UP(sub_hdr_size, block_size); >> + dh->sub_hdr_size = cpu_convert_to_target32(sub_hdr_size, endian); >> + /* dh->max_mapnr may be truncated, full 64bit is in >> kh.max_mapnr_64 */ >> + dh->max_mapnr = cpu_convert_to_target32(MIN(s->max_mapnr, UINT_MAX), >> + endian); >> + dh->nr_cpus = cpu_convert_to_target32(s->nr_cpus, endian); >> + bitmap_blocks = DIV_ROUND_UP(s->len_dump_bitmap, block_size) * 2; >> + dh->bitmap_blocks = cpu_convert_to_target32(bitmap_blocks, endian); >> + memcpy(&(dh->utsname.machine), "i686", 4); >> + > > In my opinion, it's not a right thing to hardcode the architecture in > arch-independent module dump.c > > you hardcode it here in create_header32 routine by > > memcpy(&(dh->utsname.machine), "i686", 4); > > and in create_header64 routine by > > memcpy(&(dh->utsname.machine), "x86_64", 6); > > Besides that you code actually can work on s390x arch. IMHO, target > arch should be coded here. > > Maybe you could make use of this function: cpu_to_uname_machine. I seem to recall that Qiao Nuohan stated that he (*) couldn't test this feature on any other architecture than i686/x86_64. So my proposal is, let's not block the series based on just this one point. Let's review it and allow it to be merged (if there are no other problems). Then you and/or Christian could post a small patch that allows the feature to work on s390x too, checking also that your patch doesn't regress it on x86_64. Perhaps Qiao Nuohan could re-test at that time for regressions (on x86_64), and follow up with his (*) Tested-by. Does it sound acceptable? (*) Qiao Nuohan, could you please state if you'd like to be referred to as "he" or "she" in third person; also, as "Qiao" or "Nuohan" when using just one name? I apoligize but I can't figure it out :( Thanks, Laszlo