From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39236) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W8A4c-0002wy-Jo for qemu-devel@nongnu.org; Tue, 28 Jan 2014 09:55:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W8A4X-00075U-PA for qemu-devel@nongnu.org; Tue, 28 Jan 2014 09:55:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43225) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W8A4X-000720-8x for qemu-devel@nongnu.org; Tue, 28 Jan 2014 09:55:29 -0500 Message-ID: <52E7C4D5.5030600@redhat.com> Date: Tue, 28 Jan 2014 15:55: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> <52E7B28D.60305@redhat.com> <52E7C1DF.2040309@linux.vnet.ibm.com> In-Reply-To: <52E7C1DF.2040309@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 Cc: Christian Borntraeger , Qiao Nuohan , qemu-devel@nongnu.org On 01/28/14 15:42, Ekaterina Tumanova wrote: > On 01/28/2014 05:37 PM, Laszlo Ersek wrote: >> 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 >> > > Thanks you for your reply, Laszlo. > > Just couple of thoughts: > > Firstly, I already mentioned this in previous review cycle, but there > was no answer/changes :( I thought that Qiao had addressed this before (replying to Christian): http://thread.gmane.org/gmane.comp.emulators.qemu/251215/focus=251227 But I agree that no consensus was reached. > Secondly, it feels really wrong to hardcode the specific arch in > the arch-independent file, especially because patch doesn't > prevent this function to be compiled and used on other architectures > (only mentions this in commit message). I do agree with that, but it doesn't regress existing functionality (ie. it's pure improvement on x86 and x86_64, and doesn't break existing code on other arches). > Thirdly, the fix seems pretty simple, so why do not incorporate it > in the first place... If you're implying (as before) cpu_to_uname_machine(), that per se would break the x86_64 case (which is the primary target arch for the feature). Namely: - the dump header needs: x86_64 - the proposed function returns: x86-64 Note the difference between underscore (_) and hyphen (-). Thanks Laszlo