Re: [PATCH v2 14/16] powerpc/book3s64/vmemmap: Switch radix to use a different vmemmap handling function

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

On 6/28/23 7:03 AM, Ritesh Harjani (IBM) wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:

....

>> +int __meminit vmemmap_check_pmd(pmd_t *pmd, int node,
>> +				unsigned long addr, unsigned long next)
>> +{
>> +	int large = pmd_large(*pmd);
>> +
>> +	if (pmd_large(*pmd))
> 
> we already got the value of pmd_large into "large" variable.
> we can use just if (large) right?
> 
>> +		vmemmap_verify((pte_t *)pmd, node, addr, next);
> 
> maybe we can use pmdp_ptep() function here which we used in the 1st patch?
> also shouldn't this be pmdp in the function argument instead of pmd?
> 

updated

>> +
>> +	return large;
>> +}
>> +
>> +void __meminit vmemmap_set_pmd(pmd_t *pmdp, void *p, int node,
>> +			       unsigned long addr, unsigned long next)
>> +{
>> +	pte_t entry;
>> +	pte_t *ptep = pmdp_ptep(pmdp);
>> +
>> +	VM_BUG_ON(!IS_ALIGNED(addr, PMD_SIZE));
>> +	entry = pfn_pte(__pa(p) >> PAGE_SHIFT, PAGE_KERNEL);
>> +	set_pte_at(&init_mm, addr, ptep, entry);
>> +	asm volatile("ptesync": : :"memory");
>> +
>> +	vmemmap_verify(ptep, node, addr, next);
>> +}
>> +
>> +static pte_t * __meminit radix__vmemmap_pte_populate(pmd_t *pmd, unsigned long addr, int node,
>> +						     struct vmem_altmap *altmap,
>> +						     struct page *reuse)
>> +{
>> +	pte_t *pte = pte_offset_kernel(pmd, addr);
>> +
>> +	if (pte_none(*pte)) {
>> +		pte_t entry;
>> +		void *p;
>> +
>> +		if (!reuse) {
>> +			/*
>> +			 * make sure we don't create altmap mappings
>> +			 * covering things outside the device.
>> +			 */
>> +			if (altmap && altmap_cross_boundary(altmap, addr, PAGE_SIZE))
>> +				altmap = NULL;
>> +
>> +			p = vmemmap_alloc_block_buf(PAGE_SIZE, node, altmap);
>> +			if (!p) {
>> +				if (altmap)
>> +					p = vmemmap_alloc_block_buf(PAGE_SIZE, node, NULL);
>> +				if (!p)
>> +					return NULL;
>> +			}
> 
> Above if conditions are quite confusing when looking for the 1st time?
> Can we do this? Did I get it right?
> 
>                 if (!p && altmap)
>                   p = vmemmap_alloc_block_buf(PAGE_SIZE, node, NULL);
> 
>                 if (!p)
>                   return NULL;
> 

updated


-aneesh