9:11 a.m.
----- Original Message -----
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.
No, I would leave the SADUMP implementation as it is. First, I don't maintain it,
and secondly, apparently the hardware that it runs on always has a non-zero
phys_base? I really don't know. The sadump maintainers will be ACK'ing this
patchset as well, so I will leave it up to them.
But diskdump_phys_base() and kdump_phys_base() -- as long as the dumpfile type is
verified -- should return whatever is there, including zero, regardless whether
it is the initialization value or set legitimately. The subsequent call to
x86_64_virt_phys_base() will either verify it, or if lucky, calculate it += 16MB.
Make sense?
> > 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.
Yeah, you're right -- looking at the current sadump implementation, it looks
like even though its get_sadump_smram_cpu_state_any() function can return FALSE,
sadump_calc_kaslr_offset() doesn't bother to check it.
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?
Looks OK -- but since your kdump_get_idtr() is identical to diskdump_get_idtr(),
and kdump_get_cr3() is identical to diskdump_get_cr3(), why can't the 4 functions
be merged into a single function, and put in the new kaslr_helper.c? I understand
why the two get_qemucpustate() functions exist.
> > } 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?
I would keep the QEMU_MEM_DUMP_NO_VMCOREINFO() section where it is, because it
still potentially calls your new kdump_phys_base().
Thanks,
Dave