Re: [Patch 2/2] Kexec/Kdump support POWER6

["Sachin P. Sant" <sachinp@in.ibm.com>]

Hi Olof, thanks for the review. 

>> +	if (have_vrma)
>> +		pSeries_find_hpte_vrma();
>> +
>>     
>
> This will break kexec builds on non-pseries. It's referring to platform
> code that might not be built.
>
>   
Ok. Will call this function from lpar.c instead of
default_machine_kexec().

> Does this function find the vrma, or save it away? Seems like the name
> is misleading.
>
>   
Well it finds a vrma entry and saves it. I thought of
pSeries_find_save_hpte_vrma(), but decided against it. I could
change it to pSeries_save_hpte_vrma().

> Is ppc64_vrma_page_size really the size, or the shift? Above would
> indicate that it's really a shift value.
>
>   
It is a shift. I will change it to ppc64_vrma_page_shift.

> Why is 16M hardcoded here, when you're taking such great care to read
> out the pagesize earlier?
>
>   
Hrmm. Ok will use the vrma_page_shift value.

>> +			((dword0 & HPTE_V_MASK) == MAGIC_SKIP_HPTE)) {
>>     
> Indentation
>   
Done.

>> +				/* store the hpte */
>> +				hpte_vrma_slots[num_hpte_vrma_slots++] = slot;
>>     
>
> Here you rely on global exported state (num_hpte_vrma_slots), increasing it without
> checking for limits. What happens if this function is ever called twice? Should you
> set it to 0 in the beginning of the function and check it against the size of the
> hpte_vrma_slots array instead?
>   
Will add proper checks for num_hpte_vrma_slots variable value.

>> +extern void pSeries_find_hpte_vrma(void);
>>
>>     
> Same comment as above: This isn't a kexec function as much as a pseries function, so
> it should be defined in some other header instead.
>   
>>  
>> +#define HPTE_V_RMA_VPN         ASM_CONST(0x001FFFFFF0000000)
>> +#define HPTE_V_MASK            ASM_CONST(0xc000000000000000)
>> +#define MAGIC_SKIP_HPTE        ASM_CONST(0x4000000000000000)
>> +#define HPTE_V_RMA_NUM         16
>>     
>
> "MAGIC_SKIP_HPTE"? I'm sure there's a proper name for this field in the
> PAPR, isn't there? Also, HPTE_V_RMA_NUM isn't a HPTE_V field, it shouldn't
> have that prefix. It's not a property of the mmu in the first place.
>
> These should maybe be local defines in the pseries lpar code instead, since it's
> more of a lpar<->phyp interface than mmu programming interface.
>   
Will move them to pseries lpar code.
Updated patch on its way.

Thanks
-Sachin