[Crash-utility] [PATCH v2 2/3] [ppc] Support for platform based Virtual address translation

[Suzuki K. Poulose]

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 at 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