Message-ID: <7b8605d6-0c86-206a-e8ab-27d8ecf344c8@csgroup.eu>
Date:   Wed, 21 Sep 2022 16:40:58 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Baoquan He <bhe@...hat.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "hch@...radead.org" <hch@...radead.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "agordeev@...ux.ibm.com" <agordeev@...ux.ibm.com>,
        "wangkefeng.wang@...wei.com" <wangkefeng.wang@...wei.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page
 prot



Le 13/09/2022 à 17:11, Baoquan He a écrit :
> On 09/12/22 at 07:10am, Christophe Leroy wrote:
>> Hi Baoquan,
>>
>> Le 12/09/2022 à 04:55, Baoquan He a écrit :
>>> Hi Christophe,
>>>
>>> On 08/28/22 at 07:10pm, Baoquan He wrote:
>>>> On 08/23/22 at 07:03pm, Christophe Leroy wrote:
>>> ......
>>>>>>>>> Is it really the best approach ? Wouldn't it be better to have helpers
>>>>>>>>> to do that, those helpers being called by the ioremap_prot(), instead of
>>>>>>>>> doing it inside the arch_ioremap() function ?
>>>>>>>>
>>>>>>>> This is suggested too by Alexander during his v1 reviewing. I tried, but
>>>>>>>> feel the current way taken in this patchset is better. Because not all
>>>>>>>> architecutres need the address fix up, only parisc, and only few need
>>>>>>>> adjust the 'prot'. Introducing other helpers seems too much, that only
>>>>>>>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP
>>>>>>>> method for people to understand and take.
>>>>>>>
>>>>>>> I can't understand. Why is it difficult to do something like:
>>>>>>>
>>>>>>> #ifndef ioremap_adjust_prot
>>>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
>>>>>>> {
>>>>>>> 	return flags;
>>>>>>> }
>>>>>>> #endif
>>>>>>>
>>>>>>> Then for arc you do
>>>>>>>
>>>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
>>>>>>> {
>>>>>>> 	return pgprot_val(pgprot_noncached(__pgprot(flags)));
>>>>>>> }
>>>>>>> #define ioremap_adjust_prot ioremap_adjust_prot
>>>>>>
>>>>>> My thinking is we have four things to do in the added hookers.
>>>>>> 1) check if we should do ioremap on ARCHes. If not, return NULL from
>>>>>> ioremap_prot();
>>>>>> 2) handling the mapping io address specifically on ARCHes, e.g arc,
>>>>>> ia64, s390;
>>>>>> 3) the original physical address passed into ioremap_prot() need be
>>>>>> fixed up, e.g arc;
>>>>>> 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc
>>>>>> and xtensa.
>>>>>>
>>>>>> With Kefeng's patches, the case 1) is handled with introduced
>>>>>> ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is
>>>>>> rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1),
>>>>>> 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each
>>>>>> case is not difficult from my side. I worry that as time goes by, those
>>>>>> several hooks my cause issue, e.g if a new adjustment need be done,
>>>>>> should we introduce a new helper or make do with the existed hook; how
>>>>>>
>>>>>> When I investigated this, one arch_ioremap() looks not complicated
>>>>>> since not all ARCHes need cover all above 4 cases. That's why I finally
>>>>>> choose one hook. I am open to new idea, please let me know if we should
>>>>>> change it to introduce several different helpers.
>>>>>>
>>>>>
>>>>> A new idea that would have my preference would be to do just like we did
>>>>> with arch_get_unmapped_area(). Look at
>>>>> https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638
>>>>> and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131
>>>>>
>>>>> Instead of having the generic that calls the arch specific, make it the
>>>>> other way round, have the arch specific call the generic after doing its
>>>>> specialties.
>>>>
>>>> This sounds good. I made a draft patch to change code in generic code
>>>> part, just showing what it looks like.
>>>>
>>>> Both arch_ioremap() way and the arch sepcific call the generic way look
>>>> good to me. Just it will take less effort for me to continue the
>>>> arch_ioremap() way. I would like to hear Christoph's opinion since he
>>>> introduced the GENERIC_IOREMAP method and suggested the earlier
>>>> arch_ioremap() way and change in this patchset.
>>>
>>> I will make another round change and post. Since Christoph doesn't
>>> reply, I would like to continue with the existing
>>> arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion
>>> on the new idea to reintroduce ioremap().
>>>
>>
>> I still dislike you approach with the architectures modifying local vars
>> by reference, and as you said earlier I'm not the only one : "This is
>> suggested too by Alexander during his v1 reviewing".
> 
> Alexander suggested several helpers, as I have explained earlier, that
> will cause at least four helpers currently. And could be more later if
> new change is introduced. And the address fixup and prot modifcation
> are related in few architecutures. Adding all of them is is not so
> necessary.
> 
>>
>> So I'd really prefer you to reconsider your approach and avoid passign
>> pointers to local vars to architecture helpers.
> 
> If only passing pointers to local vars is disliked, I can explain why I
> did so. Let me take arch_ioremap() of a64 as example. I can derefence
> pointer in arch_ioremap() to avoid assigning pointers to local vars.
> Please see below two version for comparing, and please tell which one is
> better.

Ok, yes I overlooked and didn't remember it right.

> 
> To me, assigning pointers to local vars make code simple and clean,
> honestly.

Well, for me it looks ood, not intellectually natural.

If I understand correctly, you do

ioremap()
   --> Call arch_ioremap()
     --> If the arch doesn't want to handle ioremap itself, it returns NULL
     --> Then you fallback on generic handling.

The arch may say "I don't want to handle it", but at the same time it 
blindly modifies the parameters so that the generic handling is not 
exacly the generic handling.
Not easy to follow for the reader. Do you have any exemple in the kernel 
that works more or less with the same approach ?


What I propose is

Arch specific ioremap()
   --> do proper preparation
   --> call generic_ioremap()

And the generic fallback implementation when the arch doesn't have a 
specific ioremap()

__weak ioremap()
   --> call generic_ioremap().

The above looks a lot more natural and easier to follow, it is clear for 
the reader which function does what.


Christophe



> 
> ******
> The version in which pointers assigned to local vars looks like below.
> The old phys_addr is passed in, the assigning can decrease changed line.
> diff --git a/arch/ia64/mm/ioremap.c b/arch/ia64/mm/ioremap.c
> index 55fd3eb753ff..8a085fc660e3 100644
> --- a/arch/ia64/mm/ioremap.c
> +++ b/arch/ia64/mm/ioremap.c
> @@ -30,15 +30,12 @@ early_ioremap (unsigned long phys_addr, unsigned long size)
>   }
>   
>   void __iomem *
> -ioremap (unsigned long phys_addr, unsigned long size)
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
>   {
> -	void __iomem *addr;
> -	struct vm_struct *area;
> -	unsigned long offset;
> -	pgprot_t prot;
> -	u64 attr;
> +	phys_addr_t phys_addr = *paddr;
>   	unsigned long gran_base, gran_size;
>   	unsigned long page_base;
> +	u64 attr;
>   
>   	/*
>   	 * For things in kern_memmap, we must use the same attribute
> @@ -69,35 +66,18 @@ ioremap (unsigned long phys_addr, unsigned long size)
>   	page_base = phys_addr & PAGE_MASK;
>   	size = PAGE_ALIGN(phys_addr + size) - page_base;
>   	if (efi_mem_attribute(page_base, size) & EFI_MEMORY_WB) {
> -		prot = PAGE_KERNEL;
> -
> -		/*
> -		 * Mappings have to be page-aligned
> -		 */
> -		offset = phys_addr & ~PAGE_MASK;
> -		phys_addr &= PAGE_MASK;
> -
> -		/*
> -		 * Ok, go for it..
> -		 */
> -		area = get_vm_area(size, VM_IOREMAP);
> -		if (!area)
> -			return NULL;
> -
> -		area->phys_addr = phys_addr;
> -		addr = (void __iomem *) area->addr;
> -		if (ioremap_page_range((unsigned long) addr,
> -				(unsigned long) addr + size, phys_addr, prot)) {
> -			vunmap((void __force *) addr);
> -			return NULL;
> -		}
> -
> -		return (void __iomem *) (offset + (char __iomem *)addr);
> +		return NULL;
>   	}
>   
>   	return __ioremap_uc(phys_addr);
>   }
> -EXPORT_SYMBOL(ioremap);
> +
> +int arch_iounmap(void __iomem *addr)
> +{
> +	if (REGION_NUMBER(addr) != RGN_GATE)
> +		return -EINVAL;
> +	return 0;
> +}
>   
>   void __iomem *
>   ioremap_uc(unsigned long phys_addr, unsigned long size)
> 
> 
> *********
> The version in which pointer is dereferenced directly in place. Then
> more lines of code are involved. And some pointer derefencing takes
> place in macro, so bracket is needed.
> diff --git a/arch/ia64/mm/ioremap.c b/arch/ia64/mm/ioremap.c
> index 55fd3eb753ff..e1b991dc2347 100644
> --- a/arch/ia64/mm/ioremap.c
> +++ b/arch/ia64/mm/ioremap.c
> @@ -30,35 +30,32 @@ early_ioremap (unsigned long phys_addr, unsigned long size)
>   }
>   
>   void __iomem *
> -ioremap (unsigned long phys_addr, unsigned long size)
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
>   {
> -	void __iomem *addr;
> -	struct vm_struct *area;
> -	unsigned long offset;
> -	pgprot_t prot;
> -	u64 attr;
> +	phys_addr_t phys_addr = *paddr;
>   	unsigned long gran_base, gran_size;
>   	unsigned long page_base;
> +	u64 attr;
>   
>   	/*
>   	 * For things in kern_memmap, we must use the same attribute
>   	 * as the rest of the kernel.  For more details, see
>   	 * Documentation/ia64/aliasing.rst.
>   	 */
> -	attr = kern_mem_attribute(phys_addr, size);
> +	attr = kern_mem_attribute(*paddr, size);
>   	if (attr & EFI_MEMORY_WB)
> -		return (void __iomem *) phys_to_virt(phys_addr);
> +		return (void __iomem *) phys_to_virt(*paddr);
>   	else if (attr & EFI_MEMORY_UC)
> -		return __ioremap_uc(phys_addr);
> +		return __ioremap_uc(*paddr);
>   
>   	/*
>   	 * Some chipsets don't support UC access to memory.  If
>   	 * WB is supported for the whole granule, we prefer that.
>   	 */
> -	gran_base = GRANULEROUNDDOWN(phys_addr);
> -	gran_size = GRANULEROUNDUP(phys_addr + size) - gran_base;
> +	gran_base = GRANULEROUNDDOWN(*paddr);
> +	gran_size = GRANULEROUNDUP(*paddr + size) - gran_base;
>   	if (efi_mem_attribute(gran_base, gran_size) & EFI_MEMORY_WB)
> -		return (void __iomem *) phys_to_virt(phys_addr);
> +		return (void __iomem *) phys_to_virt(*paddr);
>   
>   	/*
>   	 * WB is not supported for the whole granule, so we can't use
> @@ -66,38 +63,21 @@ ioremap (unsigned long phys_addr, unsigned long size)
>   	 * area with kernel page table mappings, we can use those
>   	 * instead.
>   	 */
> -	page_base = phys_addr & PAGE_MASK;
> -	size = PAGE_ALIGN(phys_addr + size) - page_base;
> +	page_base = (*paddr) & PAGE_MASK;
> +	size = PAGE_ALIGN(*paddr + size) - page_base;
>   	if (efi_mem_attribute(page_base, size) & EFI_MEMORY_WB) {
> -		prot = PAGE_KERNEL;
> -
> -		/*
> -		 * Mappings have to be page-aligned
> -		 */
> -		offset = phys_addr & ~PAGE_MASK;
> -		phys_addr &= PAGE_MASK;
> -
> -		/*
> -		 * Ok, go for it..
> -		 */
> -		area = get_vm_area(size, VM_IOREMAP);
> -		if (!area)
> -			return NULL;
> -
> -		area->phys_addr = phys_addr;
> -		addr = (void __iomem *) area->addr;
> -		if (ioremap_page_range((unsigned long) addr,
> -				(unsigned long) addr + size, phys_addr, prot)) {
> -			vunmap((void __force *) addr);
> -			return NULL;
> -		}
> -
> -		return (void __iomem *) (offset + (char __iomem *)addr);
> +		return NULL;
>   	}
>   
> -	return __ioremap_uc(phys_addr);
> +	return __ioremap_uc(*paddr);
> +}
> +
> +int arch_iounmap(void __iomem *addr)
> +{
> +	if (REGION_NUMBER(addr) != RGN_GATE)
> +		return -EINVAL;
> +	return 0;
>   }
> -EXPORT_SYMBOL(ioremap);
>   
>   void __iomem *
>   ioremap_uc(unsigned long phys_addr, unsigned long size)
> 

Powered by blists - more mailing lists