Re: [Crash-utility] [PATCH v2 2/3] [ppc] Support for platform based Virtual address translation
Nakayama-San,
> This patch adds infrastructure for defining Virtual address
translation bits
> for each platform and use the specific definition for the platform depending on
> the 'powerpc_base_platform' variable. If a matching platform is not found,
> fallbacks to the default definition.
>
> Each platform can define the PGDIRSHIT, PTRS_PER_PTE and the size of a
> physical address.
>
> I have not modified the size of the machdep->pgd allocation. Instead,
> we always read a PAGESIZE() which contains the entry.
This way is very effectual in several angles, I like it
in addtion to your first integration work.
> Signed-off-by: Suzuki K. Poulose<suzuki(a)in.ibm.com>
> --- a/ppc.c
> +++ b/ppc.c
> @@ -67,10 +67,59 @@ static void ppc_display_machine_stats(void);
> static void ppc_dump_line_number(ulong);
> static struct line_number_hook ppc_line_number_hooks[];
>
> +struct platform {
> + char *name;
const char *name is better?
Yes, will do that.
> + int pgdir_shift;
> + int ptrs_per_pgd;
> + int ptrs_per_pte;
> + int pte_size;
> +} ppc_boards[] = {
> + {
> + /* Always keep the default as the first entry */
> + .name = "default",
> + .pgdir_shift = DEFAULT_PGDIR_SHIFT,
> + .ptrs_per_pgd = DEFAULT_PTRS_PER_PGD,
> + .ptrs_per_pte = DEFAULT_PTRS_PER_PTE,
> + .pte_size = DEFAULT_PTE_SIZE,
> + },
> + {
> + /* Keep this at the end */
> + .name = NULL,
> + }
> +};
> +
> +struct platform *base_platform;
> +static struct platform *ppc_get_base_platform(void);
> +
> /* Defined in diskdump.c */
> extern void process_elf32_notes(void *, ulong);
>
> /*
> + * Find the platform of the crashing system and set the
> + * base_platform accordingly.
> + */
> +struct platform*
> +ppc_get_base_platform(void)
> +{
> + char platform[32];
> + struct platform *tmp =&ppc_boards[1]; /* start at 1 */
> + ulong ppc_platform;
> +
> + if(!try_get_symbol_data("powerpc_base_platform",
sizeof(ulong),&ppc_platform))
> + return&ppc_boards[0];
> +
> + if (read_string(ppc_platform, platform, 31) == 0)
> + return&ppc_boards[0];
> +
> + for (; tmp->name!= NULL; tmp++)
> + if (!strcmp(tmp->name, platform))
> + return tmp;
Is there any reasons to distinct "ppc440gp"?
No specific reasons, apart from just the name difference.
Since I see that there are no "ppc440" prefix platform
other than PPC40
in latest cpu_specs[], and all of ppc440gp's values in ppc_boards[] are the
same as ppc440.
Can it be summarized by using STRNEQ("ppc440") or strncmp("ppc440")?
The problem is, this is generic code. And there could be a different platforms
which start with the same prefix and having different settings for the page
bits, (e.g, freescale processors). So generalizing this would be a problem.
One option I could think of is defining a function rather than a 'name' for each
platform to identify the 'running platform' as its variant.
i.e,
struct platform {
int (*check_platform) (char *s);
const char *name;
...
}
Where we could invoke the check_platform() for each platform, instead of
doing a check ourselves. The check_platform() could be implemented by each
distinct platforms and could use one board definition for multiple platform
values. This would also help us to reduce the 'platform' definitions as different
non-related platforms may also have the same set of definitions.
So the loop would look like :
for (; tmp->name!= NULL; tmp++)
if (tmpe->check_platform(platform))
return tmp;
What do you think about this approach ?
And I often see similar comparisons as SRTEQ() in crash code,
also "unsigned long long" is "ulonglong".
I will switch to
ulonglong.
Thank you, for the review !
Suzuki