Hi Lianbo,
Thanks for the reviews.
On Tue, Sep 26, 2023 at 01:55:32PM +0800, lijiang wrote:
> Hi, Aditya
> Thank you for the fix.
> On Fri, Sep 22, 2023 at 7:03 AM <crash-utility-request@xxxxxxxxxx> wrote:
>
> > Date: Thu, 21 Sep 2023 12:29:05 +0530
> > From: Aditya Gupta <adityag@xxxxxxxxxxxxx>
> > To: crash-utility@xxxxxxxxxx
> > Cc: Hari Bathini <hbathini@xxxxxxxxxxxxx>, Mahesh J Salgaonkar
> > <mahesh@xxxxxxxxxxxxx>, Sourabh Jain <sourabhjain@xxxxxxxxxxxxx>,
> > d.hatayama@xxxxxxxxxxx
> > Subject: [PATCH v1] diskdump: add hook for additional
> > checks on prstatus notes validity
> > Message-ID: <20230921065905.1020839-1-adityag@xxxxxxxxxxxxx>
> > Content-Type: text/plain; charset="US-ASCII"; x-default=true
> >
> > Upstream crash reports these warnings on PowerPC64:
> >
> > WARNING: cpu 0 invalid NT_PRSTATUS note (n_type != NT_PRSTATUS)
> > ...
> >
> > Apart from these warnings, register values are also invalid.
> >
> > This warning was found in the commit:
> >
> > commit db8c030857b4 ("diskdump/netdump: fix segmentation fault
> > caused by failure of stopping CPUs")
> >
> > With above commit, crash checks whether 'crash_notes' is initialised,
> > before mapping PRSTATUS notes.
> >
> > But some architectures such as PowerPC64, in fadump case
> > (firmware-assisted dump), don't populate 'crash_notes' since the
> > registers are already stored in the cpu notes in the vmcore.
> >
> > Instead of checking 'crash_notes' for all architectures, introduce
> > a machdep hook ('is_cpu_prstatus_valid'), for architectures to
> > decide validity checks for PRSTATUS notes
> >
> > A default hook ('diskdump_is_cpu_prstatus_valid') has also been provided
> > for all architectures other than PowerPC64, which checks if 'crash_notes'
> > for a given cpu is valid, maintaining the current behaviour
> >
> > PowerPC64 doesn't utilise 'crash_notes' to get register values, so no
> > additional checks are required
> >
> > ...
> >
> > ---
> > ---
> > defs.h | 1 +
> > diskdump.c | 15 ++++++++++++---
> > ppc64.c | 10 ++++++++++
> > 3 files changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/defs.h b/defs.h
> > index 96a7a2a31471..f7f56947e5ac 100644
> > --- a/defs.h
> > +++ b/defs.h
> > @@ -1073,6 +1073,7 @@ struct machdep_table {
> > int (*verify_line_number)(ulong, ulong, ulong);
> > void (*get_irq_affinity)(int);
> > void (*show_interrupts)(int, ulong *);
> > + int (*is_cpu_prstatus_valid)(int cpu);
> >
>
> I would suggest putting it at the end of this table. Although it may not
> break the compatibility of the extension module, just like the
> offset_table/size_table, I get used to doing that if there is no special
> reason.
>
Sure, will move it to the end of the table. I did not think of the compatibility
issue, thanks.
>
> > int (*is_page_ptr)(ulong, physaddr_t *);
> > int (*get_cpu_reg)(int, int, const char *, int, void *);
> > };
> > diff --git a/diskdump.c b/diskdump.c
> > index 2c284ff3f97f..ad9a00b08ce1 100644
> > --- a/diskdump.c
> > +++ b/diskdump.c
> > @@ -142,13 +142,22 @@ int have_crash_notes(int cpu)
> > return TRUE;
> > }
> >
> > +int diskdump_is_cpu_prstatus_valid(int cpu)
> > +{
> > + static int crash_notes_exists = -1;
> > +
> > + if (crash_notes_exists == -1)
> > + crash_notes_exists = kernel_symbol_exists("crash_notes");
> > +
> > + return (!crash_notes_exists || have_crash_notes(cpu));
> > +}
> > +
> >
>
> Got a warning as below:
>
> cc -c -g -DX86_64 -DLZO -DGDB_10_2 diskdump.c -Wall -O2
> -Wstrict-prototypes -Wmissing-prototypes -fstack-protector
> -Wformat-security
> diskdump.c:145:5: warning: no previous prototype for
> ‘diskdump_is_cpu_prstatus_valid’ [-Wmissing-prototypes]
> 145 | int diskdump_is_cpu_prstatus_valid(int cpu)
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
Will fix in V2. Will add a declaration in defs.h, just above 'have_crash_notes'
declaration, or should I add the declaration at the end of all diskdump.c
declarations in defs.h ?
>
>
> > void
> > map_cpus_to_prstatus_kdump_cmprs(void)
> > {
> > void **nt_ptr;
> > int online, i, j, nrcpus;
> > size_t size;
> > - int crash_notes_exists;
> >
> > if (pc->flags2 & QEMU_MEM_DUMP_COMPRESSED) /* notes exist for all
> > cpus */
> > goto resize_note_pointers;
> > @@ -171,10 +180,9 @@ map_cpus_to_prstatus_kdump_cmprs(void)
> > * Re-populate the array with the notes mapping to online cpus
> > */
> > nrcpus = (kt->kernel_NR_CPUS ? kt->kernel_NR_CPUS : NR_CPUS);
> > - crash_notes_exists = kernel_symbol_exists("crash_notes");
> >
> > for (i = 0, j = 0; i < nrcpus; i++) {
> > - if (in_cpu_map(ONLINE_MAP, i) && (!crash_notes_exists ||
> > have_crash_notes(i))) {
> > + if (in_cpu_map(ONLINE_MAP, i) &&
> > machdep->is_cpu_prstatus_valid(i)) {
> > dd->nt_prstatus_percpu[i] = nt_ptr[j++];
> > dd->num_prstatus_notes =
> > MAX(dd->num_prstatus_notes, i+1);
> > @@ -1076,6 +1084,7 @@ diskdump_init(char *unused, FILE *fptr)
> > if (!DISKDUMP_VALID() && !KDUMP_CMPRS_VALID())
> > return FALSE;
> >
> > + machdep->is_cpu_prstatus_valid = diskdump_is_cpu_prstatus_valid;
> > dd->ofp = fptr;
> > return TRUE;
> > }
> > diff --git a/ppc64.c b/ppc64.c
> > index fc34006f4863..1159b8c3a8e7 100644
> > --- a/ppc64.c
> > +++ b/ppc64.c
> > @@ -298,6 +298,15 @@ struct machine_specific book3e_machine_specific = {
> > .is_vmaddr = book3e_is_vmaddr,
> > };
> >
> > +/**
> > + * No additional checks are required on PPC64, for checking if PRSTATUS
> > notes
> > + * is valid
> > + */
> > +int ppc64_is_cpu_prstatus_valid(int cpu)
> > +{
> > + return TRUE;
> > +}
> > +
> > #define SKIBOOT_BASE 0x30000000
> >
> > /*
> > @@ -418,6 +427,7 @@ ppc64_init(int when)
> > break;
> >
> > case POST_GDB:
> > + machdep->is_cpu_prstatus_valid =
> > ppc64_is_cpu_prstatus_valid;
> >
>
> The hook is set in the stage of POST_GDB, I'm wondering if the current
> warning is still shown in the crash minimal mode(with option --minimal).
> Can you help to confirm this one?
Sure, will check this. Just looked at it, seems the warning might still be there,
if it is minimal mode.
Basically what I wanted is, this machdep->is_cpu_prstatus_valid to be
overwritten, after diskdump_init has run (which sets machdep->is_cpu_prstatus_valid
to a default), and before 'map_cpus_to_prstatus_kdump_cmprs' (where it is used),
will see if the warning comes, will try to understand the flow and move the
code accordingly.
>
> And other changes are fine to me.
Thanks,
- Aditya Gupta
>
> Thanks.
> Lianbo
>
> ms = machdep->machspec;
> >
> > if (!(machdep->flags & BOOK3E)) {
> > --
> > 2.41.0
> >
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
