FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Crash Utility

 
 
LinkBack Thread Tools
 
Old 08-21-2012, 01:29 AM
HATAYAMA Daisuke
 
Default add support to "virsh dump-guest-memory"(qemu memory dump)

From: Dave Anderson <anderson@redhat.com>
Subject: Re: [Crash-utility] [PATCH] add support to "virsh dump-guest-memory"(qemu memory dump)
Date: Mon, 20 Aug 2012 15:47:16 -0400

> ----- Original Message -----
>> At 2012-8-20 16:32, qiaonuohan wrote:
>> >
>> > I have modified the patches, and they are based on crash 6.0.9.
>>
>> I find some mistakes in my former patches, please ignore them and refer
>> to the attachments of this mail.
>>
>> --
>> --
>> Regards
>> Qiao Nuohan
>
> The patch is looking better, but a few issues still remain to be
> cleaned up.
>
> I'm wondering about the correctness of this addition to read_netdump()
> for 32-bit dumpfiles:
>
> int
> read_netdump(int fd, void *bufptr, int cnt, ulong addr, physaddr_t paddr)
> {
> off_t offset;
> struct pt_load_segment *pls;
> int i;
>
> + if (nd->flags & QEMU_MEM_DUMP_KDUMP_BACKUP &&
> + paddr >= nd->backup_src_start &&
> + paddr < nd->backup_src_start + nd->backup_src_size) {
> + ulong orig_paddr;
> +
> + orig_paddr = paddr;
> + paddr += nd->backup_offset - nd->backup_src_start;
> +
> + if (CRASHDEBUG(1))
> + error(INFO,
> + "qemu_mem_dump: kdump backup region: %#lx => %#lx
",
> + orig_paddr, paddr);
> + }
>
> The incoming "paddr" parameter is type physaddr_t, which is declared as:
>
> typedef uint64_t physaddr_t;
>
> I see that you've pretty much copied the "if" statement from read_sadump(),
> but I'm not sure whether SADUMP supports 32-bit dumpfiles?
>

SADUMP supports 32-bit dumpfiles, so this is a bug. Thanks for
pointing out this.

> Since nd->backup_src_start is a physical address, maybe it should be
> declared as a physaddr_t as well? Or if the incoming paddr is 4GB or
> greater, then perhaps it shouldn't be checked against nd->backup_src_start.
> In other words, I'm not sure what happens when you do this:
>
> paddr >= nd->backup_src_start
>
> When running on a 32-bit machine, does paddr get truncated to a 32-bit value,
> or does nd->backup_src_start get promoted to a 64-bit value?
>

At least next test case on RHEL5.8 32-bit machines shows truncation in
32-bit direction.

#include <stdio.h>
#include <stdint.h>

int main(int argc, char **argv)
{
uint64_t u = (1ULL << 32);
unsigned long i = (unsigned long)-1;

if (u >= i)
printf("%#llx >= %#x
", u, i);
else
printf("not %#llx >= %#x
", u, i);

if (u < i)
printf("%#llx < %#x
", u, i);
else
printf("not %#llx < %#x
", u, i);

return 0;
}

[hat@vm-x86 test]$ gcc ./test.c -o test; ./test
not 0x100000000 >= 0xffffffff
0x100000000 < 0xffffffff

Please apply the attached patch.

Thanks.
HATAYAMA, Daisuke
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 08-21-2012, 06:05 AM
qiaonuohan
 
Default add support to "virsh dump-guest-memory"(qemu memory dump)

At 2012-8-21 3:47, Dave Anderson wrote:

The patch is looking better, but a few issues still remain to be
cleaned up.


The patches have been modified as you recommended. And the third patch
also adds some fields to vmcore_data, so "help -n" is modified here
once again.

--
--
Regards
Qiao Nuohan


--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 08-21-2012, 06:32 AM
HATAYAMA Daisuke
 
Default add support to "virsh dump-guest-memory"(qemu memory dump)

From: qiaonuohan <qiaonuohan@cn.fujitsu.com>
Subject: Re: [Crash-utility] [PATCH] add support to "virsh dump-guest-memory"(qemu memory dump)
Date: Tue, 21 Aug 2012 14:05:25 +0800

>>From c2f4d203d1610213ce2af9754d857f7f2192f121 Mon Sep 17 00:00:00 2001
> From: qiaonuohan <qiaonuohan@cn.fujitsu.com>
> Date: Tue, 21 Aug 2012 13:35:54 +0800
> Subject: [PATCH 2/3] support core dump file when kdump is operating
>

> diff --git a/main.c b/main.c
> index 9dced6e..033bfa1 100755
> --- a/main.c
> +++ b/main.c
> @@ -640,6 +640,8 @@ main_loop(void)
> if (!(pc->flags & GDB_INIT)) {
> gdb_session_init();
> show_untrusted_files();
> + if (pc->flags2 & QEMU_MEM_DUMP)
> + qemu_mem_dump_kdump_backup_region_init();
> if (SADUMP_DUMPFILE())
> sadump_kdump_backup_region_init();
> if (XEN_HYPER_MODE()) {

This is different from what Dave suggests. He suggests it's useful if
this conversion is applied also to Xen part. Now the conversion is
done for vmcore with qemu note only. The previous patch sets
QEMU_MEM_DUMP if QEMU note exists. To identify Xen's vmcore, some kind
of the sign that indicates "this vmcore is Xen's" is needed.

I'm now thinking that this backup region processing should be written
commonly among crash dump mechanisms running independently of
kdump. They are now qemu's dump-guest-memory, xen's dump mechanism and
sadump.

Thanks.
HATAYAMA, Daisuke

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 08-21-2012, 01:43 PM
Dave Anderson
 
Default add support to "virsh dump-guest-memory"(qemu memory dump)

----- Original Message -----
> From: qiaonuohan <qiaonuohan@cn.fujitsu.com>
> Subject: Re: [Crash-utility] [PATCH] add support to "virsh
> dump-guest-memory"(qemu memory dump)
> Date: Tue, 21 Aug 2012 14:05:25 +0800
>
> >>From c2f4d203d1610213ce2af9754d857f7f2192f121 Mon Sep 17 00:00:00 2001
> > From: qiaonuohan <qiaonuohan@cn.fujitsu.com>
> > Date: Tue, 21 Aug 2012 13:35:54 +0800
> > Subject: [PATCH 2/3] support core dump file when kdump is operating
> >
>
> > diff --git a/main.c b/main.c
> > index 9dced6e..033bfa1 100755
> > --- a/main.c
> > +++ b/main.c
> > @@ -640,6 +640,8 @@ main_loop(void)
> > if (!(pc->flags & GDB_INIT)) {
> > gdb_session_init();
> > show_untrusted_files();
> > + if (pc->flags2 & QEMU_MEM_DUMP)
> > + qemu_mem_dump_kdump_backup_region_init();
> > if (SADUMP_DUMPFILE())
> > sadump_kdump_backup_region_init();
> > if (XEN_HYPER_MODE()) {
>
> This is different from what Dave suggests. He suggests it's useful if
> this conversion is applied also to Xen part. Now the conversion is
> done for vmcore with qemu note only. The previous patch sets
> QEMU_MEM_DUMP if QEMU note exists. To identify Xen's vmcore, some kind
> of the sign that indicates "this vmcore is Xen's" is needed.
>
> I'm now thinking that this backup region processing should be written
> commonly among crash dump mechanisms running independently of
> kdump. They are now qemu's dump-guest-memory, xen's dump mechanism and
> sadump.

Wait a minute -- I don't understand how "xen's dump mechanism" has anything
to do with this backup region processing?

-> He suggests it's useful if this conversion is applied also to Xen part.

No, when I referred to Xen with respect to the read_kdump() function, I was only
indicating that there is an "if xen" statement there that translates a Xen
psuedo-physical address into a Xen machine address -- where the incoming "paddr"
argument gets modified by xen_kdump_p2m() before being passed to the common
read_netump() function. And given that Qiao's patch is:

(1) also changing the incoming "paddr" argument on the fly, and
(2) it's a kdump clone,

then it makes more sense that his check be done in read_kdump() instead of
in read_netdump().

Anyway, now looking at sadump.c, I see that sadump_kdump_backup_region_init()
and Qiao's qemu_mem_dump_kdump_backup_region_init() function are pretty much
equivalent except for:

(1) qemu_mem_dump_kdump_backup_region_init() supports 32-bit ELF vmcores,
based upon the setting of the nd->flags KDUMP_ELF32/KDUMP_ELF64 bits, and

(2) qemu_mem_dump_kdump_backup_region_init() uses the pre-existing vmcore_data
structure from netdump.c, and sadump.c uses its own sadump_data structure,
where both structures have the same thing at the end of the struct:

...
/* Backup Region, First 640K of System RAM. */
#define KEXEC_BACKUP_SRC_END 0x0009ffff
ulong backup_src_start;
ulong backup_src_size;
ulonglong backup_offset;
};

So I agree that they could be a common function, and we could
have the main_loop() function do something like this:

if (!(pc->flags & GDB_INIT)) {
gdb_session_init();
show_untrusted_files();
+ kdump_backup_region_init()
- if (SADUMP_DUMPFILE())
- sadump_kdump_backup_region_init();
if (XEN_HYPER_MODE()) {

The common kdump_backup_region_init() could be put in netdump.c, where
it can check for SADUMP_DUMPFILE() or (pc->flags2 & QEMU_MEM_DUMP), and
if either is set, do its thing. The main issue would be dealing with the
usage of the two different structures. netdump.c can safely #include
sadump.h, and there would only be a need for a new global function in
sadump.c similar to this one:

struct vmcore_data *get_kdump_vmcore_data(void);

but which passes a pointer to the sadump_data structure back to the
common function in netdump.c.

What do you guys think?

Dave




--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 08-21-2012, 03:09 PM
Dave Anderson
 
Default add support to "virsh dump-guest-memory"(qemu memory dump)

----- Original Message -----
> From: Dave Anderson <anderson@redhat.com>
> Subject: Re: [Crash-utility] [PATCH] add support to "virsh
> dump-guest-memory"(qemu memory dump)
> Date: Mon, 20 Aug 2012 15:47:16 -0400
>
> > ----- Original Message -----
> >> At 2012-8-20 16:32, qiaonuohan wrote:
> >> >
> >> > I have modified the patches, and they are based on crash 6.0.9.
> >>
> >> I find some mistakes in my former patches, please ignore them and refer
> >> to the attachments of this mail.
> >>
> >> --
> >> --
> >> Regards
> >> Qiao Nuohan
> >
> > The patch is looking better, but a few issues still remain to be
> > cleaned up.
> >
> > I'm wondering about the correctness of this addition to read_netdump()
> > for 32-bit dumpfiles:
> >
> > int
> > read_netdump(int fd, void *bufptr, int cnt, ulong addr,
> > physaddr_t paddr)
> > {
> > off_t offset;
> > struct pt_load_segment *pls;
> > int i;
> >
> > + if (nd->flags & QEMU_MEM_DUMP_KDUMP_BACKUP &&
> > + paddr >= nd->backup_src_start &&
> > + paddr < nd->backup_src_start + nd->backup_src_size) {
> > + ulong orig_paddr;
> > +
> > + orig_paddr = paddr;
> > + paddr += nd->backup_offset - nd->backup_src_start;
> > +
> > + if (CRASHDEBUG(1))
> > + error(INFO,
> > + "qemu_mem_dump: kdump backup region: %#lx => %#lx
",
> > + orig_paddr, paddr);
> > + }
> >
> > The incoming "paddr" parameter is type physaddr_t, which is declared as:
> >
> > typedef uint64_t physaddr_t;
> >
> > I see that you've pretty much copied the "if" statement from read_sadump(),
> > but I'm not sure whether SADUMP supports 32-bit dumpfiles?
> >
>
> SADUMP supports 32-bit dumpfiles, so this is a bug. Thanks for
> pointing out this.
>
> > Since nd->backup_src_start is a physical address, maybe it should be
> > declared as a physaddr_t as well? Or if the incoming paddr is 4GB or
> > greater, then perhaps it shouldn't be checked against nd->backup_src_start.
> > In other words, I'm not sure what happens when you do this:
> >
> > paddr >= nd->backup_src_start
> >
> > When running on a 32-bit machine, does paddr get truncated to a 32-bit value,
> > or does nd->backup_src_start get promoted to a 64-bit value?
> >
>
> At least next test case on RHEL5.8 32-bit machines shows truncation in
> 32-bit direction.
>
> #include <stdio.h>
> #include <stdint.h>
>
> int main(int argc, char **argv)
> {
> uint64_t u = (1ULL << 32);
> unsigned long i = (unsigned long)-1;
>
> if (u >= i)
> printf("%#llx >= %#x
", u, i);
> else
> printf("not %#llx >= %#x
", u, i);
>
> if (u < i)
> printf("%#llx < %#x
", u, i);
> else
> printf("not %#llx < %#x
", u, i);
>
> return 0;
> }
>
> [hat@vm-x86 test]$ gcc ./test.c -o test; ./test
> not 0x100000000 >= 0xffffffff
> 0x100000000 < 0xffffffff
>
> Please apply the attached patch.
>
> Thanks.
> HATAYAMA, Daisuke
>
>
> [Text
> Documents:0001-sadump-Fix-invalid-truncation-of-physical-address-to.patch]
>

Thanks Daisuke -- queued for crash-6.1.0.

Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 08-21-2012, 03:18 PM
Dave Anderson
 
Default add support to "virsh dump-guest-memory"(qemu memory dump)

----- Original Message -----
> At 2012-8-21 3:47, Dave Anderson wrote:
> > The patch is looking better, but a few issues still remain to be
> > cleaned up.
>
> The patches have been modified as you recommended. And the third patch
> also adds some fields to vmcore_data, so "help -n" is modified here
> once again.
>
> --
> --
> Regards
> Qiao Nuohan

Thanks for the updated patch. But I am going to defer it until we
first come to an agreement about how to best consolidate the commonality
between the sadump and qemu-mem-dump formats.

I'd like to accept Daisuke's "backup_src_start" bug-fix, but for a new
common init function, it would seem best that these fields should be
the same:

diff --git a/sadump.h b/sadump.h
index 64c2630..29dce06 100644
--- a/sadump.h
+++ b/sadump.h
@@ -204,7 +204,7 @@ struct sadump_data {

/* Backup Region, First 640K of System RAM. */
#define KEXEC_BACKUP_SRC_END 0x0009ffff
- ulong backup_src_start;
+ ulonglong backup_src_start;
ulong backup_src_size;
ulonglong backup_offset;
};
--

whereas your patch has this:

diff --git a/netdump.h b/netdump.h
index 2e296ad..4a6d661 100644
--- a/netdump.h
+++ b/netdump.h
@@ -71,6 +71,11 @@ struct vmcore_data {
struct xen_kdump_data *xen_kdump_data;
void *vmcoreinfo;
uint size_vmcoreinfo;
+/* Backup Region, First 640K of System RAM. */
+#define KEXEC_BACKUP_SRC_END 0x0009ffff
+ ulong backup_src_start;
+ ulong backup_src_size;
+ ulong backup_offset;
};

Can you change your patch so that they are the same?

And would you like to work on creating the new common
kdump_backup_region_init() function that can handle both
vmcore_data and sadump_data structures?

Thanks,
Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 08-22-2012, 08:55 AM
qiaonuohan
 
Default add support to "virsh dump-guest-memory"(qemu memory dump)

At 2012-8-21 23:18, Dave Anderson wrote:

Can you change your patch so that they are the same?

And would you like to work on creating the new common
kdump_backup_region_init() function that can handle both
vmcore_data and sadump_data structures?



Please refer to the attachments. The first two are used to support qemu
memory dump file. And I made the 4th patch to rewrite function
kdump_backup_region_init() based on HATAYAMA's patch(the 3rd patch).


--
--
Regards
Qiao Nuohan


--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 08-22-2012, 08:20 PM
Dave Anderson
 
Default add support to "virsh dump-guest-memory"(qemu memory dump)

----- Original Message -----
> At 2012-8-21 23:18, Dave Anderson wrote:
> > Can you change your patch so that they are the same?
> >
> > And would you like to work on creating the new common
> > kdump_backup_region_init() function that can handle both
> > vmcore_data and sadump_data structures?
> >
>
> Please refer to the attachments. The first two are used to support qemu
> memory dump file. And I made the 4th patch to rewrite function
> kdump_backup_region_init() based on HATAYAMA's patch(the 3rd patch).
>
> --
> --
> Regards
> Qiao Nuohan

Hello Qiao,

Thanks for consolidating the common SADUMP/QEMU_MEM_DUMP code
into kdump_backup_region_init(). The patch works well, at least
on the two sample QEMU_MEM_DUMP dumpfiles I have. Did you also
verify this patch on an SADUMP dumpfile? (I don't have one
available).

I'd like to make a few small changes/additions that don't affect the
functionality of the patch, but are more for aesthetics/maintainability.
You do *not* have to resubmit the patch again!

FYI, here's what I'm changing:

You created new get_vmcore_data() and get_sadump_data() functions
to get pointers to the vmcore_data and sadump_data structures, but
you have them returning a void *:

void *
get_vmcore_data(void)
{
return nd;
}

void *
get_sadump_data(void)
{
return sd;
}

But your get_vmcore_data() is essentially a duplicate of the existing
get_kdump_vmcore_data(), which returns a pointer to the same structure:

struct vmcore_data *
get_kdump_vmcore_data(void)
{
if (!VMCORE_VALID() || !KDUMP_DUMPFILE())
return NULL;

return &vmcore_data;
}

Anyway, in kdump_backup_region_init(), you call the two functions
to set "vd":

void *vd;
...

if (SADUMP_DUMPFILE()) {
vd = get_sadump_data();
...
} else if (pc->flags2 & QEMU_MEM_DUMP) {
vd = get_vmcore_data();

And then you have constructs such as these where you have to cast "vd"
as the relevant structure:

if (SADUMP_DUMPFILE()) {
((struct sadump_data *)vd)->flags |=
SADUMP_KDUMP_BACKUP;
((struct sadump_data *)vd)->backup_src_start =
backup_src_start;
((struct sadump_data *)vd)->backup_src_size =
backup_src_size;
((struct sadump_data *)vd)->backup_offset =
backup_offset;
} else if (pc->flags2 & QEMU_MEM_DUMP) {
((struct vmcore_data *)vd)->flags |=
QEMU_MEM_DUMP_KDUMP_BACKUP;
((struct vmcore_data *)vd)->backup_src_start =
backup_src_start;
((struct vmcore_data *)vd)->backup_src_size =
backup_src_size;
((struct vmcore_data *)vd)->backup_offset =
backup_offset;
}

Kind of ugly, no?

So I'm going to do it like this, so that the constructs above can
be expressed more clearly:

struct vmcore_data *vd;
struct sadump_data *sd;
...

if (SADUMP_DUMPFILE()) {
sd = get_sadump_data();
...
} else if (pc->flags2 & QEMU_MEM_DUMP) {
vd = get_kdump_vmcore_data();

....

if (SADUMP_DUMPFILE()) {
sd->flags |= SADUMP_KDUMP_BACKUP;
sd->backup_src_start = backup_src_start;
sd->backup_src_size = backup_src_size;
sd->backup_offset = backup_offset;
} else if (pc->flags2 & QEMU_MEM_DUMP) {
vd->flags |= QEMU_MEM_DUMP_KDUMP_BACKUP;
vd->backup_src_start = backup_src_start;
vd->backup_src_size = backup_src_size;
vd->backup_offset = backup_offset;
}

Also, I don't like this declaration/naming convention:

int archflag; /* indicate the arch of core file: 1 -> 32bit */

and then all of the subsequent "if (archflag)" checks in the function.
Since it's not really an "arch", but rather whether it's a 32-bit dumpfile,
I've changed its name to "is_32_bit" to clarify the subsequent usage.

I also updated the "help" help page itself, and changed the option from
"help -q" to "help -r". That way, if in the future somebody wants to dump
the registers from some other dumpfile type, it can be done from there.

Anyway, thanks again for all the work you've put into this effort.

Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 08-23-2012, 01:28 AM
qiaonuohan
 
Default add support to "virsh dump-guest-memory"(qemu memory dump)

At 2012-8-23 4:20, Dave Anderson wrote:

I also updated the "help" help page itself, and changed the option from
"help -q" to "help -r". That way, if in the future somebody wants to dump
the registers from some other dumpfile type, it can be done from there.


Add an info here. The information displayed by "help -q" not only
includes registers.

<cut>
+ netdump_print(" version:%08lx size:%08lx
",
+ ptr->version, ptr->size);
<cut>

If you think it is not suitable here, deleting it will be fine.

--
--
Regards
Qiao Nuohan



--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 08-23-2012, 12:45 PM
Dave Anderson
 
Default add support to "virsh dump-guest-memory"(qemu memory dump)

----- Original Message -----
> At 2012-8-23 4:20, Dave Anderson wrote:
> > I also updated the "help" help page itself, and changed the option from
> > "help -q" to "help -r". That way, if in the future somebody wants to dump
> > the registers from some other dumpfile type, it can be done from there.
>
> Add an info here. The information displayed by "help -q" not only
> includes registers.
>
> <cut>
> + netdump_print(" version:%08lx size:%08lx
",
> + ptr->version, ptr->size);
> <cut>
>
> If you think it is not suitable here, deleting it will be fine.

OK, I'll just enclose it with CRASHDEBUG(1) in case there's ever a need
to see it.

BTW, did you or Daisuke get a chance to verify this patch with an SADUMP dumpfile?

Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 

Thread Tools




All times are GMT. The time now is 08:34 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org