Re: [PATCH] pseries: Fix 64 bit logical memory block panic

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

On 7/16/20 7:00 AM, Paul Mackerras wrote:
> On Wed, Jul 15, 2020 at 06:12:25PM +0530, Aneesh Kumar K.V wrote:
>> Anton Blanchard <anton@ozlabs.org> writes:
>>
>>> Booting with a 4GB LMB size causes us to panic:
>>>
>>>    qemu-system-ppc64: OS terminated: OS panic:
>>>        Memory block size not suitable: 0x0
>>>
>>> Fix pseries_memory_block_size() to handle 64 bit LMBs.
>>>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Anton Blanchard <anton@ozlabs.org>
>>> ---
>>>   arch/powerpc/platforms/pseries/hotplug-memory.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> index 5ace2f9a277e..6574ac33e887 100644
>>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> @@ -27,7 +27,7 @@ static bool rtas_hp_event;
>>>   unsigned long pseries_memory_block_size(void)
>>>   {
>>>   	struct device_node *np;
>>> -	unsigned int memblock_size = MIN_MEMORY_BLOCK_SIZE;
>>> +	uint64_t memblock_size = MIN_MEMORY_BLOCK_SIZE;
>>>   	struct resource r;
>>>   
>>>   	np = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
>>
>> We need similar changes at more places?
>>
>> modified   arch/powerpc/include/asm/book3s/64/mmu.h
>> @@ -85,7 +85,7 @@ extern unsigned int mmu_base_pid;
>>   /*
>>    * memory block size used with radix translation.
>>    */
>> -extern unsigned int __ro_after_init radix_mem_block_size;
>> +extern unsigned long __ro_after_init radix_mem_block_size;
>>   
>>   #define PRTB_SIZE_SHIFT	(mmu_pid_bits + 4)
>>   #define PRTB_ENTRIES	(1ul << mmu_pid_bits)
>> modified   arch/powerpc/include/asm/drmem.h
>> @@ -21,7 +21,7 @@ struct drmem_lmb {
>>   struct drmem_lmb_info {
>>   	struct drmem_lmb        *lmbs;
>>   	int                     n_lmbs;
>> -	u32                     lmb_size;
>> +	u64                     lmb_size;
>>   };
>>   
>>   extern struct drmem_lmb_info *drmem_info;
>> modified   arch/powerpc/mm/book3s64/radix_pgtable.c
>> @@ -34,7 +34,7 @@
>>   
>>   unsigned int mmu_pid_bits;
>>   unsigned int mmu_base_pid;
>> -unsigned int radix_mem_block_size __ro_after_init;
>> +unsigned long radix_mem_block_size __ro_after_init;
> 
> These changes look fine.
> 
>>   static __ref void *early_alloc_pgtable(unsigned long size, int nid,
>>   			unsigned long region_start, unsigned long region_end)
>> modified   arch/powerpc/mm/drmem.c
>> @@ -268,14 +268,15 @@ static void __init __walk_drmem_v2_lmbs(const __be32 *prop, const __be32 *usm,
>>   void __init walk_drmem_lmbs_early(unsigned long node,
>>   			void (*func)(struct drmem_lmb *, const __be32 **))
>>   {
>> +	const __be64 *lmb_prop;
>>   	const __be32 *prop, *usm;
>>   	int len;
>>   
>> -	prop = of_get_flat_dt_prop(node, "ibm,lmb-size", &len);
>> -	if (!prop || len < dt_root_size_cells * sizeof(__be32))
>> +	lmb_prop = of_get_flat_dt_prop(node, "ibm,lmb-size", &len);
>> +	if (!lmb_prop || len < sizeof(__be64))
>>   		return;
>>   
>> -	drmem_info->lmb_size = dt_mem_next_cell(dt_root_size_cells, &prop);
>> +	drmem_info->lmb_size = be64_to_cpup(lmb_prop);
> 
> This particular change shouldn't be necessary.  We already have
> dt_mem_next_cell() returning u64, and it knows how to combine two
> cells to give a u64 (for dt_root_size_cells == 2).


agreed. I added it here because in another patch i was confused about 
the usage of dt_root_size_cells. We don't generally use that in other 
device tree parsing code. I will move that to a separate patch as cleanup.

> 
>>   	usm = of_get_flat_dt_prop(node, "linux,drconf-usable-memory", &len);
>>   
>> @@ -296,19 +297,19 @@ void __init walk_drmem_lmbs_early(unsigned long node,
>>   
>>   static int __init init_drmem_lmb_size(struct device_node *dn)
>>   {
>> -	const __be32 *prop;
>> +	const __be64 *prop;
>>   	int len;
>>   
>>   	if (drmem_info->lmb_size)
>>   		return 0;
>>   
>>   	prop = of_get_property(dn, "ibm,lmb-size", &len);
>> -	if (!prop || len < dt_root_size_cells * sizeof(__be32)) {
>> +	if (!prop || len < sizeof(__be64)) {
>>   		pr_info("Could not determine LMB size\n");
>>   		return -1;
>>   	}
>>   
>> -	drmem_info->lmb_size = dt_mem_next_cell(dt_root_size_cells, &prop);
>> +	drmem_info->lmb_size = be64_to_cpup(prop);
> 
> Same comment here.
> 

-aneesh