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

[Laszlo Ersek <lersek@redhat.com>]

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 <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
>>
> 
> 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