7:50 a.m.
On Fri, Mar 16, 2018 at 04:33:41PM -0400, Dave Anderson wrote:
Hi Sergio,
A few initial comments/questions/concerns about this patch...
> diff --git a/diskdump.c b/diskdump.c
> index b08a46c..1ec4bcf 100644
> --- a/diskdump.c
> +++ b/diskdump.c
> @@ -56,6 +56,7 @@ struct diskdump_data {
> void **nt_prstatus_percpu;
> uint num_prstatus_notes;
> void **nt_qemu_percpu;
> + void **nt_qemucs_percpu;
> uint num_qemu_notes;
>
> /* page cache */
> @@ -72,6 +73,7 @@ struct diskdump_data {
> ulong *valid_pages;
> ulong accesses;
> ulong snapshot_task;
> + ulong kaslr_phys_base;
> };
Generally speaking, there already is an sd->phys_base, and you've
added an nd->phys_base, but I don't understand why you also added
a new dd->kaslr_phys_base member and new diskdump_kaslr_phys_base()
function? Is there any reason that you can't continue to use the
currently-existing dd->sub_header_kdump->phys_base member and the
diskdump_phys_base() function? I just find the concept of a
"kaslr_phys_base" confusing, i.e., as if there are two different
types of phys_base in the kernel. Can you please try to utilize
the existing member and function?
Also related, your diskdump_kaslr_phys_base() and kdump_phys_base() functions
don't account for (return FALSE) with a legitimate phys_base value of 0.
In fact I have a sample RHEL7 ELF vmcore generated by virsh dump which
has a phys_base of 0. More on that below...
Both of these are the consequence of trying to avoid changing the current
SADUMP's implementation while keeping NETDUMP's and DISKDUMP's as similar as
possible.
But if there's no problem in changing SADUMP's, I would change the setters to
succeed unconditionally, and remove the "if (base->phys_base)" check from
the
getters. This would make them semantically equivalent to current's
"diskdump_phys_base", which is the main reason I've added kaslr_phys_base,
allowing me to use the existing dd->sub_header_kdump->phys_base.
> diff --git a/kaslr_helper.c b/kaslr_helper.c
> index 1079863..5b71e3e 100644
> --- a/kaslr_helper.c
> +++ b/kaslr_helper.c
> @@ -386,6 +386,9 @@ calc_kaslr_offset(ulong *kaslr_offset, ulong *phys_base)
> if (KDUMP_DUMPFILE()) {
> idtr = kdump_get_idtr();
> cr3 = kdump_get_cr3();
> + } else if (DISKDUMP_DUMPFILE()) {
> + idtr = diskdump_get_idtr();
> + cr3 = diskdump_get_cr3();
All 4 of these new functions above can fail and return 0. Probably
unlikely, but shouldn't there be a FALSE return if either one is 0,
rather than continuing and using them?
Again, I was trying to keep in sync with current SADUMP's implementation.
Otherwise, I'd prefer implementing a single function like this:
int [diskdump|kdump|sadump]_get_idtr_cr3(uint64_t *idtr, uint64_t *cr3);
That would allow me to write some like:
if ((KDUMP_DUMPFILE() && !kdump_get_idtr_cr3(&idtr, &cr3)) ||
(DISKDUMP_DUMPFILE() && !diskdump_get_idtr_cr3(&idtr, &cr3))) {
return FALSE;
}
What do you think?
> } else {
> return FALSE;
> }
> diff --git a/x86_64.c b/x86_64.c
> index ed5985a..3c492e4 100644
> --- a/x86_64.c
> +++ b/x86_64.c
> @@ -6632,8 +6632,15 @@ x86_64_calc_phys_base(void)
> * Get relocation value from whatever dumpfile format is being used.
> */
>
> - if (QEMU_MEM_DUMP_NO_VMCOREINFO() && KDUMP_DUMPFILE()) {
> - if (kdump_phys_base(&phys_base)) {
> + if (QEMU_MEM_DUMP_NO_VMCOREINFO()) {
> + int ret;
> +
> + if (KDUMP_DUMPFILE())
> + ret = kdump_phys_base(&phys_base);
> + else if (DISKDUMP_DUMPFILE())
> + ret = diskdump_kaslr_phys_base(&phys_base);
> +
> + if (ret) {
> machdep->machspec->phys_base = phys_base;
> if (CRASHDEBUG(1))
> fprintf(fp, "kdump-novmci: phys base: %lx\n",
> --
> 2.14.3
This is where the "0 phys_base" issue comes into play. I think the section
above should do the same thing as the following "if (DISKDUMP_DUMPFILE())"
does, where diskump_phys_base() is only concerned if the dumpfile itself
is valid. It may return 0 as a phys_base, and that's OK, because it
unconditionally calls x86_64_virt_phys_base() -- which serves a dual
purpose, either to:
(1) verify it, or
(2) if it's bogus, it checks whether plus-or-minus 16MB works.
After switching to using dd->sub_header_kdump->phys_base, do you think we can
just leave QEMU_MEM_DUMP_COMPRESSED to be dealt by the DISKDUMP_DUMPFILE
section, and add another equivalent one for QEMU_MEM_DUMP_ELF && !VMCOREINFO?
Sergio.