On Thu, Sep 21, 2023 at 01:51:27PM +0800, lijiang wrote:
> On Mon, Sep 18, 2023 at 7:34 PM lijiang <lijiang@xxxxxxxxxx> wrote:
>
> >>
> >> ---
> >> ---
> >> ppc64.c | 47 +++++++++++++++++++++++++++++++++--------------
> >> 1 file changed, 33 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/ppc64.c b/ppc64.c
> >> index fc34006f4863..1e84c5f56773 100644
> >> --- a/ppc64.c
> >> +++ b/ppc64.c
> >> @@ -1280,8 +1280,30 @@ void ppc64_vmemmap_init(void)
> >> long backing_size, virt_addr_offset, phys_offset, list_offset;
> >> ulong *vmemmap_list;
> >> char *vmemmap_buf;
> >> - struct machine_specific *ms;
> >> -
> >> + struct machine_specific *ms = machdep->machspec;
> >> +
> >> + ld = &list_data;
> >> + BZERO(ld, sizeof(struct list_data));
> >> +
> >> + /*
> >> + * vmemmap_list is missing or set to 0 in the kernel would imply
> >> + * vmemmap region is mapped in the kernel pagetable. So, read
> >> vmemmap_list
> >> + * anyway and use the translation method accordingly.
> >> + */
> >> + readmem(symbol_value("vmemmap_list"), KVADDR, &ld->start,
> >> sizeof(void *),
> >> + "vmemmap_list", RETURN_ON_ERROR);
> >> + if (!ld->start) {
> >> + /*
> >> + * vmemmap_list is set to 0 or missing. Do kernel
> >> pagetable walk
> >> + * for vmemmamp address translation.
> >> + */
> >> + ms->vmemmap_list = NULL;
> >> + ms->vmemmap_cnt = 0;
> >> +
> >> + machdep->flags |= VMEMMAP_AWARE;
> >> + return;
> >> + }
> >> +
> >>
> >
> I would suggest putting the 'readmem(symbol_value("vmemmap_list"),...)'
> after the 'kernel_symbol_exists("vmemmap_list")', and also check the
> returned value of readmem().
>
Yes, I considered that, but the if condition checking for
'kernel_symbol_exists("vmemmap_list")' returns from the function if the symbol
is not found, same with the readmem return value check earlier, we wanted to
initialise the ms->vmemmap_list and machdep->flags before that.
The readmem used to be like this, which returns from function if readmem failed:
- if (!readmem(symbol_value("vmemmap_list"),
- KVADDR, &ld->start, sizeof(void *), "vmemmap_list",
- RETURN_ON_ERROR))
- return;
So, intentionally I put it above the if and removed the return value check,
since we don't want to depend on vmemmap_list symbol being there for newer
kernels anymore.
And to be future proof in case the address mapping moves to page table for
Hash MMU case also, and since vmemmap_list is PowerPC specific, the symbol
might also be removed.
But, if it's a coding guidelines requirement to handle the return values, I can
think of how to handle the return value of readmem, any suggestions ?
> And other changes are fine to me.
>
Thank you for your reviews Lianbo :)
Thanks,
- Aditya Gupta
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
