Re: [Qemu-devel] [PATCH v8 08/13] dump: add API to write dump header

[Laszlo Ersek <lersek@redhat.com>]

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 <qiaonuohan@cn.fujitsu.com>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>   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