diff mbox series

[v3,1/3] riscv: Factorize xip and !xip kernel address conversion macros

Message ID 20210603082749.1256129-2-alex@ghiti.fr (mailing list archive)
State New
Headers show
Series riscv: Map the kernel with correct permissions the first time | expand

Commit Message

Alex Ghiti June 3, 2021, 8:27 a.m. UTC
To simplify the kernel address conversion code, make the same definition of
kernel_mapping_pa_to_va and kernel_mapping_va_to_pa compatible for both xip
and !xip kernel by defining XIP_OFFSET to 0 in !xip kernel.

Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
---
 arch/riscv/include/asm/page.h    | 14 +++-----------
 arch/riscv/include/asm/pgtable.h |  2 ++
 2 files changed, 5 insertions(+), 11 deletions(-)

Comments

Anup Patel June 3, 2021, 11:39 a.m. UTC | #1
On Thu, Jun 3, 2021 at 1:59 PM Alexandre Ghiti <alex@ghiti.fr> wrote:
>
> To simplify the kernel address conversion code, make the same definition of
> kernel_mapping_pa_to_va and kernel_mapping_va_to_pa compatible for both xip
> and !xip kernel by defining XIP_OFFSET to 0 in !xip kernel.
>
> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>

I think subject of the patch should be:
"Simplify address conversion macros for xip and !xip kernel"

Otherwise it looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  arch/riscv/include/asm/page.h    | 14 +++-----------
>  arch/riscv/include/asm/pgtable.h |  2 ++
>  2 files changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> index 6a7761c86ec2..6e004d8fda4d 100644
> --- a/arch/riscv/include/asm/page.h
> +++ b/arch/riscv/include/asm/page.h
> @@ -93,9 +93,7 @@ extern unsigned long va_pa_offset;
>  #ifdef CONFIG_64BIT
>  extern unsigned long va_kernel_pa_offset;
>  #endif
> -#ifdef CONFIG_XIP_KERNEL
>  extern unsigned long va_kernel_xip_pa_offset;
> -#endif
>  extern unsigned long pfn_base;
>  #define ARCH_PFN_OFFSET                (pfn_base)
>  #else
> @@ -103,6 +101,7 @@ extern unsigned long pfn_base;
>  #ifdef CONFIG_64BIT
>  #define va_kernel_pa_offset    0
>  #endif
> +#define va_kernel_xip_pa_offset 0
>  #define ARCH_PFN_OFFSET                (PAGE_OFFSET >> PAGE_SHIFT)
>  #endif /* CONFIG_MMU */
>
> @@ -110,29 +109,22 @@ extern unsigned long kernel_virt_addr;
>
>  #ifdef CONFIG_64BIT
>  #define linear_mapping_pa_to_va(x)     ((void *)((unsigned long)(x) + va_pa_offset))
> -#ifdef CONFIG_XIP_KERNEL
>  #define kernel_mapping_pa_to_va(y)     ({                                              \
>         unsigned long _y = y;                                                           \
>         (_y >= CONFIG_PHYS_RAM_BASE) ?                                                  \
>                 (void *)((unsigned long)(_y) + va_kernel_pa_offset + XIP_OFFSET) :      \
>                 (void *)((unsigned long)(_y) + va_kernel_xip_pa_offset);                \
>         })
> -#else
> -#define kernel_mapping_pa_to_va(x)     ((void *)((unsigned long)(x) + va_kernel_pa_offset))
> -#endif
>  #define __pa_to_va_nodebug(x)          linear_mapping_pa_to_va(x)
>
>  #define linear_mapping_va_to_pa(x)     ((unsigned long)(x) - va_pa_offset)
> -#ifdef CONFIG_XIP_KERNEL
>  #define kernel_mapping_va_to_pa(y) ({                                          \
>         unsigned long _y = y;                                                   \
>         (_y < kernel_virt_addr + XIP_OFFSET) ?                                  \
>                 ((unsigned long)(_y) - va_kernel_xip_pa_offset) :               \
>                 ((unsigned long)(_y) - va_kernel_pa_offset - XIP_OFFSET);       \
>         })
> -#else
> -#define kernel_mapping_va_to_pa(x)     ((unsigned long)(x) - va_kernel_pa_offset)
> -#endif
> +
>  #define __va_to_pa_nodebug(x)  ({                                              \
>         unsigned long _x = x;                                                   \
>         (_x < kernel_virt_addr) ?                                               \
> @@ -141,7 +133,7 @@ extern unsigned long kernel_virt_addr;
>  #else
>  #define __pa_to_va_nodebug(x)  ((void *)((unsigned long) (x) + va_pa_offset))
>  #define __va_to_pa_nodebug(x)  ((unsigned long)(x) - va_pa_offset)
> -#endif
> +#endif /* CONFIG_64BIT */
>
>  #ifdef CONFIG_DEBUG_VIRTUAL
>  extern phys_addr_t __virt_to_phys(unsigned long x);
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index bde8ce3bfe7c..d98e931a31e5 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -77,6 +77,8 @@
>
>  #ifdef CONFIG_XIP_KERNEL
>  #define XIP_OFFSET             SZ_8M
> +#else
> +#define XIP_OFFSET             0
>  #endif
>
>  #ifndef __ASSEMBLY__
> --
> 2.30.2
>
Jisheng Zhang June 3, 2021, 12:27 p.m. UTC | #2
On Thu,  3 Jun 2021 10:27:47 +0200
Alexandre Ghiti <alex@ghiti.fr> wrote:

> To simplify the kernel address conversion code, make the same definition of
> kernel_mapping_pa_to_va and kernel_mapping_va_to_pa compatible for both xip
> and !xip kernel by defining XIP_OFFSET to 0 in !xip kernel.
> 
> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> ---
>  arch/riscv/include/asm/page.h    | 14 +++-----------
>  arch/riscv/include/asm/pgtable.h |  2 ++
>  2 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> index 6a7761c86ec2..6e004d8fda4d 100644
> --- a/arch/riscv/include/asm/page.h
> +++ b/arch/riscv/include/asm/page.h
> @@ -93,9 +93,7 @@ extern unsigned long va_pa_offset;
>  #ifdef CONFIG_64BIT
>  extern unsigned long va_kernel_pa_offset;
>  #endif
> -#ifdef CONFIG_XIP_KERNEL
>  extern unsigned long va_kernel_xip_pa_offset;
> -#endif
>  extern unsigned long pfn_base;
>  #define ARCH_PFN_OFFSET		(pfn_base)
>  #else
> @@ -103,6 +101,7 @@ extern unsigned long pfn_base;
>  #ifdef CONFIG_64BIT
>  #define va_kernel_pa_offset	0
>  #endif
> +#define va_kernel_xip_pa_offset 0
>  #define ARCH_PFN_OFFSET		(PAGE_OFFSET >> PAGE_SHIFT)
>  #endif /* CONFIG_MMU */
>  
> @@ -110,29 +109,22 @@ extern unsigned long kernel_virt_addr;
>  
>  #ifdef CONFIG_64BIT
>  #define linear_mapping_pa_to_va(x)	((void *)((unsigned long)(x) + va_pa_offset))
> -#ifdef CONFIG_XIP_KERNEL
>  #define kernel_mapping_pa_to_va(y)	({						\
>  	unsigned long _y = y;								\
>  	(_y >= CONFIG_PHYS_RAM_BASE) ?	

This CONFIG_PHYS_RAM_BASE is only available for XIP, could result in a
compiler error for !XIP?

I'm also concerned with the unecessary overhead of kernel_mapping_pa_to_va()
for !XIP case, there's a "if" condition branch, and extra symbol: va_kernel_xip_pa_offset

>  		(void *)((unsigned long)(_y) + va_kernel_pa_offset + XIP_OFFSET) :	\
>  		(void *)((unsigned long)(_y) + va_kernel_xip_pa_offset);		\
>  	})
> -#else
> -#define kernel_mapping_pa_to_va(x)	((void *)((unsigned long)(x) + va_kernel_pa_offset))
> -#endif
>  #define __pa_to_va_nodebug(x)		linear_mapping_pa_to_va(x)
>  
>  #define linear_mapping_va_to_pa(x)	((unsigned long)(x) - va_pa_offset)
> -#ifdef CONFIG_XIP_KERNEL
>  #define kernel_mapping_va_to_pa(y) ({						\
>  	unsigned long _y = y;							\
>  	(_y < kernel_virt_addr + XIP_OFFSET) ?					\
>  		((unsigned long)(_y) - va_kernel_xip_pa_offset) :		\
>  		((unsigned long)(_y) - va_kernel_pa_offset - XIP_OFFSET);	\
>  	})

Similar as kernel_mapping_pa_to_va(), an overhead of "if" condition branch
for !XIP and extra va_kernel_xip_pa_offset symbol.

> -#else
> -#define kernel_mapping_va_to_pa(x)	((unsigned long)(x) - va_kernel_pa_offset)
> -#endif
> +
>  #define __va_to_pa_nodebug(x)	({						\
>  	unsigned long _x = x;							\
>  	(_x < kernel_virt_addr) ?						\
> @@ -141,7 +133,7 @@ extern unsigned long kernel_virt_addr;
>  #else
>  #define __pa_to_va_nodebug(x)  ((void *)((unsigned long) (x) + va_pa_offset))
>  #define __va_to_pa_nodebug(x)  ((unsigned long)(x) - va_pa_offset)
> -#endif
> +#endif /* CONFIG_64BIT */
>  
>  #ifdef CONFIG_DEBUG_VIRTUAL
>  extern phys_addr_t __virt_to_phys(unsigned long x);
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index bde8ce3bfe7c..d98e931a31e5 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -77,6 +77,8 @@
>  
>  #ifdef CONFIG_XIP_KERNEL
>  #define XIP_OFFSET		SZ_8M
> +#else
> +#define XIP_OFFSET		0
>  #endif
>  
>  #ifndef __ASSEMBLY__
Jisheng Zhang June 3, 2021, 12:49 p.m. UTC | #3
On Thu, 3 Jun 2021 20:27:48 +0800
Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:

> On Thu,  3 Jun 2021 10:27:47 +0200
> Alexandre Ghiti <alex@ghiti.fr> wrote:
> 
> > To simplify the kernel address conversion code, make the same definition of
> > kernel_mapping_pa_to_va and kernel_mapping_va_to_pa compatible for both xip
> > and !xip kernel by defining XIP_OFFSET to 0 in !xip kernel.
> > 
> > Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> > ---
> >  arch/riscv/include/asm/page.h    | 14 +++-----------
> >  arch/riscv/include/asm/pgtable.h |  2 ++
> >  2 files changed, 5 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > index 6a7761c86ec2..6e004d8fda4d 100644
> > --- a/arch/riscv/include/asm/page.h
> > +++ b/arch/riscv/include/asm/page.h
> > @@ -93,9 +93,7 @@ extern unsigned long va_pa_offset;
> >  #ifdef CONFIG_64BIT
> >  extern unsigned long va_kernel_pa_offset;
> >  #endif
> > -#ifdef CONFIG_XIP_KERNEL
> >  extern unsigned long va_kernel_xip_pa_offset;
> > -#endif
> >  extern unsigned long pfn_base;
> >  #define ARCH_PFN_OFFSET		(pfn_base)
> >  #else
> > @@ -103,6 +101,7 @@ extern unsigned long pfn_base;
> >  #ifdef CONFIG_64BIT
> >  #define va_kernel_pa_offset	0
> >  #endif
> > +#define va_kernel_xip_pa_offset 0
> >  #define ARCH_PFN_OFFSET		(PAGE_OFFSET >> PAGE_SHIFT)
> >  #endif /* CONFIG_MMU */
> >  
> > @@ -110,29 +109,22 @@ extern unsigned long kernel_virt_addr;
> >  
> >  #ifdef CONFIG_64BIT
> >  #define linear_mapping_pa_to_va(x)	((void *)((unsigned long)(x) + va_pa_offset))
> > -#ifdef CONFIG_XIP_KERNEL
> >  #define kernel_mapping_pa_to_va(y)	({						\
> >  	unsigned long _y = y;								\
> >  	(_y >= CONFIG_PHYS_RAM_BASE) ?	  
> 
> This CONFIG_PHYS_RAM_BASE is only available for XIP, could result in a
> compiler error for !XIP?
> 
> I'm also concerned with the unecessary overhead of kernel_mapping_pa_to_va()
> for !XIP case, there's a "if" condition branch, and extra symbol: va_kernel_xip_pa_offset

Err, I just found this symobl always exists no matter XIP is enabled or not.
I will send out a patch for this clean up

> 
> >  		(void *)((unsigned long)(_y) + va_kernel_pa_offset + XIP_OFFSET) :	\
> >  		(void *)((unsigned long)(_y) + va_kernel_xip_pa_offset);		\
> >  	})
> > -#else
> > -#define kernel_mapping_pa_to_va(x)	((void *)((unsigned long)(x) + va_kernel_pa_offset))
> > -#endif
> >  #define __pa_to_va_nodebug(x)		linear_mapping_pa_to_va(x)
> >  
> >  #define linear_mapping_va_to_pa(x)	((unsigned long)(x) - va_pa_offset)
> > -#ifdef CONFIG_XIP_KERNEL
> >  #define kernel_mapping_va_to_pa(y) ({						\
> >  	unsigned long _y = y;							\
> >  	(_y < kernel_virt_addr + XIP_OFFSET) ?					\
> >  		((unsigned long)(_y) - va_kernel_xip_pa_offset) :		\
> >  		((unsigned long)(_y) - va_kernel_pa_offset - XIP_OFFSET);	\
> >  	})  
> 
> Similar as kernel_mapping_pa_to_va(), an overhead of "if" condition branch
> for !XIP and extra va_kernel_xip_pa_offset symbol.
> 
> > -#else
> > -#define kernel_mapping_va_to_pa(x)	((unsigned long)(x) - va_kernel_pa_offset)
> > -#endif
> > +
> >  #define __va_to_pa_nodebug(x)	({						\
> >  	unsigned long _x = x;							\
> >  	(_x < kernel_virt_addr) ?						\
> > @@ -141,7 +133,7 @@ extern unsigned long kernel_virt_addr;
> >  #else
> >  #define __pa_to_va_nodebug(x)  ((void *)((unsigned long) (x) + va_pa_offset))
> >  #define __va_to_pa_nodebug(x)  ((unsigned long)(x) - va_pa_offset)
> > -#endif
> > +#endif /* CONFIG_64BIT */
> >  
> >  #ifdef CONFIG_DEBUG_VIRTUAL
> >  extern phys_addr_t __virt_to_phys(unsigned long x);
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index bde8ce3bfe7c..d98e931a31e5 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -77,6 +77,8 @@
> >  
> >  #ifdef CONFIG_XIP_KERNEL
> >  #define XIP_OFFSET		SZ_8M
> > +#else
> > +#define XIP_OFFSET		0
> >  #endif
> >  
> >  #ifndef __ASSEMBLY__  
> 
>
Alex Ghiti June 3, 2021, 12:57 p.m. UTC | #4
Hi Jisheng,

Le 3/06/2021 à 14:27, Jisheng Zhang a écrit :
> On Thu,  3 Jun 2021 10:27:47 +0200
> Alexandre Ghiti <alex@ghiti.fr> wrote:
> 
>> To simplify the kernel address conversion code, make the same definition of
>> kernel_mapping_pa_to_va and kernel_mapping_va_to_pa compatible for both xip
>> and !xip kernel by defining XIP_OFFSET to 0 in !xip kernel.
>>
>> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
>> ---
>>   arch/riscv/include/asm/page.h    | 14 +++-----------
>>   arch/riscv/include/asm/pgtable.h |  2 ++
>>   2 files changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
>> index 6a7761c86ec2..6e004d8fda4d 100644
>> --- a/arch/riscv/include/asm/page.h
>> +++ b/arch/riscv/include/asm/page.h
>> @@ -93,9 +93,7 @@ extern unsigned long va_pa_offset;
>>   #ifdef CONFIG_64BIT
>>   extern unsigned long va_kernel_pa_offset;
>>   #endif
>> -#ifdef CONFIG_XIP_KERNEL
>>   extern unsigned long va_kernel_xip_pa_offset;
>> -#endif
>>   extern unsigned long pfn_base;
>>   #define ARCH_PFN_OFFSET		(pfn_base)
>>   #else
>> @@ -103,6 +101,7 @@ extern unsigned long pfn_base;
>>   #ifdef CONFIG_64BIT
>>   #define va_kernel_pa_offset	0
>>   #endif
>> +#define va_kernel_xip_pa_offset 0
>>   #define ARCH_PFN_OFFSET		(PAGE_OFFSET >> PAGE_SHIFT)
>>   #endif /* CONFIG_MMU */
>>   
>> @@ -110,29 +109,22 @@ extern unsigned long kernel_virt_addr;
>>   
>>   #ifdef CONFIG_64BIT
>>   #define linear_mapping_pa_to_va(x)	((void *)((unsigned long)(x) + va_pa_offset))
>> -#ifdef CONFIG_XIP_KERNEL
>>   #define kernel_mapping_pa_to_va(y)	({						\
>>   	unsigned long _y = y;								\
>>   	(_y >= CONFIG_PHYS_RAM_BASE) ?	
> 
> This CONFIG_PHYS_RAM_BASE is only available for XIP, could result in a
> compiler error for !XIP?

You're right, I have this patch in my branch and forgot to squash it.

> 
> I'm also concerned with the unecessary overhead of kernel_mapping_pa_to_va()
> for !XIP case, there's a "if" condition branch, and extra symbol: va_kernel_xip_pa_offset

I understand your concerns even if I don't find that the overhead is 
that important here, I prefer the readability improvement. I can always 
add unlikely/likely builtin to improve things or completely remove this 
patch if others agree with you.

Thanks,

Alex

> 
>>   		(void *)((unsigned long)(_y) + va_kernel_pa_offset + XIP_OFFSET) :	\
>>   		(void *)((unsigned long)(_y) + va_kernel_xip_pa_offset);		\
>>   	})
>> -#else
>> -#define kernel_mapping_pa_to_va(x)	((void *)((unsigned long)(x) + va_kernel_pa_offset))
>> -#endif
>>   #define __pa_to_va_nodebug(x)		linear_mapping_pa_to_va(x)
>>   
>>   #define linear_mapping_va_to_pa(x)	((unsigned long)(x) - va_pa_offset)
>> -#ifdef CONFIG_XIP_KERNEL
>>   #define kernel_mapping_va_to_pa(y) ({						\
>>   	unsigned long _y = y;							\
>>   	(_y < kernel_virt_addr + XIP_OFFSET) ?					\
>>   		((unsigned long)(_y) - va_kernel_xip_pa_offset) :		\
>>   		((unsigned long)(_y) - va_kernel_pa_offset - XIP_OFFSET);	\
>>   	})
> 
> Similar as kernel_mapping_pa_to_va(), an overhead of "if" condition branch
> for !XIP and extra va_kernel_xip_pa_offset symbol.
> 
>> -#else
>> -#define kernel_mapping_va_to_pa(x)	((unsigned long)(x) - va_kernel_pa_offset)
>> -#endif
>> +
>>   #define __va_to_pa_nodebug(x)	({						\
>>   	unsigned long _x = x;							\
>>   	(_x < kernel_virt_addr) ?						\
>> @@ -141,7 +133,7 @@ extern unsigned long kernel_virt_addr;
>>   #else
>>   #define __pa_to_va_nodebug(x)  ((void *)((unsigned long) (x) + va_pa_offset))
>>   #define __va_to_pa_nodebug(x)  ((unsigned long)(x) - va_pa_offset)
>> -#endif
>> +#endif /* CONFIG_64BIT */
>>   
>>   #ifdef CONFIG_DEBUG_VIRTUAL
>>   extern phys_addr_t __virt_to_phys(unsigned long x);
>> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
>> index bde8ce3bfe7c..d98e931a31e5 100644
>> --- a/arch/riscv/include/asm/pgtable.h
>> +++ b/arch/riscv/include/asm/pgtable.h
>> @@ -77,6 +77,8 @@
>>   
>>   #ifdef CONFIG_XIP_KERNEL
>>   #define XIP_OFFSET		SZ_8M
>> +#else
>> +#define XIP_OFFSET		0
>>   #endif
>>   
>>   #ifndef __ASSEMBLY__
> 
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
Anup Patel June 3, 2021, 1:16 p.m. UTC | #5
On Thu, Jun 3, 2021 at 6:27 PM Alex Ghiti <alex@ghiti.fr> wrote:
>
> Hi Jisheng,
>
> Le 3/06/2021 à 14:27, Jisheng Zhang a écrit :
> > On Thu,  3 Jun 2021 10:27:47 +0200
> > Alexandre Ghiti <alex@ghiti.fr> wrote:
> >
> >> To simplify the kernel address conversion code, make the same definition of
> >> kernel_mapping_pa_to_va and kernel_mapping_va_to_pa compatible for both xip
> >> and !xip kernel by defining XIP_OFFSET to 0 in !xip kernel.
> >>
> >> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> >> ---
> >>   arch/riscv/include/asm/page.h    | 14 +++-----------
> >>   arch/riscv/include/asm/pgtable.h |  2 ++
> >>   2 files changed, 5 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> >> index 6a7761c86ec2..6e004d8fda4d 100644
> >> --- a/arch/riscv/include/asm/page.h
> >> +++ b/arch/riscv/include/asm/page.h
> >> @@ -93,9 +93,7 @@ extern unsigned long va_pa_offset;
> >>   #ifdef CONFIG_64BIT
> >>   extern unsigned long va_kernel_pa_offset;
> >>   #endif
> >> -#ifdef CONFIG_XIP_KERNEL
> >>   extern unsigned long va_kernel_xip_pa_offset;
> >> -#endif
> >>   extern unsigned long pfn_base;
> >>   #define ARCH_PFN_OFFSET            (pfn_base)
> >>   #else
> >> @@ -103,6 +101,7 @@ extern unsigned long pfn_base;
> >>   #ifdef CONFIG_64BIT
> >>   #define va_kernel_pa_offset        0
> >>   #endif
> >> +#define va_kernel_xip_pa_offset 0
> >>   #define ARCH_PFN_OFFSET            (PAGE_OFFSET >> PAGE_SHIFT)
> >>   #endif /* CONFIG_MMU */
> >>
> >> @@ -110,29 +109,22 @@ extern unsigned long kernel_virt_addr;
> >>
> >>   #ifdef CONFIG_64BIT
> >>   #define linear_mapping_pa_to_va(x) ((void *)((unsigned long)(x) + va_pa_offset))
> >> -#ifdef CONFIG_XIP_KERNEL
> >>   #define kernel_mapping_pa_to_va(y) ({                                              \
> >>      unsigned long _y = y;                                                           \
> >>      (_y >= CONFIG_PHYS_RAM_BASE) ?
> >
> > This CONFIG_PHYS_RAM_BASE is only available for XIP, could result in a
> > compiler error for !XIP?
>
> You're right, I have this patch in my branch and forgot to squash it.
>
> >
> > I'm also concerned with the unecessary overhead of kernel_mapping_pa_to_va()
> > for !XIP case, there's a "if" condition branch, and extra symbol: va_kernel_xip_pa_offset
>
> I understand your concerns even if I don't find that the overhead is
> that important here, I prefer the readability improvement. I can always
> add unlikely/likely builtin to improve things or completely remove this
> patch if others agree with you.

I would also prefer readable code for long-term maintainability. Currently,
the nested "#ifdefs" are increasing causing developers to easily break
untested combinations.

Regards,
Anup

>
> Thanks,
>
> Alex
>
> >
> >>              (void *)((unsigned long)(_y) + va_kernel_pa_offset + XIP_OFFSET) :      \
> >>              (void *)((unsigned long)(_y) + va_kernel_xip_pa_offset);                \
> >>      })
> >> -#else
> >> -#define kernel_mapping_pa_to_va(x)  ((void *)((unsigned long)(x) + va_kernel_pa_offset))
> >> -#endif
> >>   #define __pa_to_va_nodebug(x)              linear_mapping_pa_to_va(x)
> >>
> >>   #define linear_mapping_va_to_pa(x) ((unsigned long)(x) - va_pa_offset)
> >> -#ifdef CONFIG_XIP_KERNEL
> >>   #define kernel_mapping_va_to_pa(y) ({                                              \
> >>      unsigned long _y = y;                                                   \
> >>      (_y < kernel_virt_addr + XIP_OFFSET) ?                                  \
> >>              ((unsigned long)(_y) - va_kernel_xip_pa_offset) :               \
> >>              ((unsigned long)(_y) - va_kernel_pa_offset - XIP_OFFSET);       \
> >>      })
> >
> > Similar as kernel_mapping_pa_to_va(), an overhead of "if" condition branch
> > for !XIP and extra va_kernel_xip_pa_offset symbol.
> >
> >> -#else
> >> -#define kernel_mapping_va_to_pa(x)  ((unsigned long)(x) - va_kernel_pa_offset)
> >> -#endif
> >> +
> >>   #define __va_to_pa_nodebug(x)      ({                                              \
> >>      unsigned long _x = x;                                                   \
> >>      (_x < kernel_virt_addr) ?                                               \
> >> @@ -141,7 +133,7 @@ extern unsigned long kernel_virt_addr;
> >>   #else
> >>   #define __pa_to_va_nodebug(x)  ((void *)((unsigned long) (x) + va_pa_offset))
> >>   #define __va_to_pa_nodebug(x)  ((unsigned long)(x) - va_pa_offset)
> >> -#endif
> >> +#endif /* CONFIG_64BIT */
> >>
> >>   #ifdef CONFIG_DEBUG_VIRTUAL
> >>   extern phys_addr_t __virt_to_phys(unsigned long x);
> >> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> >> index bde8ce3bfe7c..d98e931a31e5 100644
> >> --- a/arch/riscv/include/asm/pgtable.h
> >> +++ b/arch/riscv/include/asm/pgtable.h
> >> @@ -77,6 +77,8 @@
> >>
> >>   #ifdef CONFIG_XIP_KERNEL
> >>   #define XIP_OFFSET         SZ_8M
> >> +#else
> >> +#define XIP_OFFSET          0
> >>   #endif
> >>
> >>   #ifndef __ASSEMBLY__
> >
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
Jisheng Zhang June 3, 2021, 1:53 p.m. UTC | #6
On Thu, 3 Jun 2021 18:46:47 +0530
Anup Patel <anup@brainfault.org> wrote:

> On Thu, Jun 3, 2021 at 6:27 PM Alex Ghiti <alex@ghiti.fr> wrote:
> >
> > Hi Jisheng,

Hi,

> >
> > Le 3/06/2021 à 14:27, Jisheng Zhang a écrit :  
> > > On Thu,  3 Jun 2021 10:27:47 +0200
> > > Alexandre Ghiti <alex@ghiti.fr> wrote:
> > >  
> > >> To simplify the kernel address conversion code, make the same definition of
> > >> kernel_mapping_pa_to_va and kernel_mapping_va_to_pa compatible for both xip
> > >> and !xip kernel by defining XIP_OFFSET to 0 in !xip kernel.
> > >>
> > >> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> > >> ---
> > >>   arch/riscv/include/asm/page.h    | 14 +++-----------
> > >>   arch/riscv/include/asm/pgtable.h |  2 ++
> > >>   2 files changed, 5 insertions(+), 11 deletions(-)
> > >>
> > >> diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > >> index 6a7761c86ec2..6e004d8fda4d 100644
> > >> --- a/arch/riscv/include/asm/page.h
> > >> +++ b/arch/riscv/include/asm/page.h
> > >> @@ -93,9 +93,7 @@ extern unsigned long va_pa_offset;
> > >>   #ifdef CONFIG_64BIT
> > >>   extern unsigned long va_kernel_pa_offset;
> > >>   #endif
> > >> -#ifdef CONFIG_XIP_KERNEL
> > >>   extern unsigned long va_kernel_xip_pa_offset;
> > >> -#endif
> > >>   extern unsigned long pfn_base;
> > >>   #define ARCH_PFN_OFFSET            (pfn_base)
> > >>   #else
> > >> @@ -103,6 +101,7 @@ extern unsigned long pfn_base;
> > >>   #ifdef CONFIG_64BIT
> > >>   #define va_kernel_pa_offset        0
> > >>   #endif
> > >> +#define va_kernel_xip_pa_offset 0
> > >>   #define ARCH_PFN_OFFSET            (PAGE_OFFSET >> PAGE_SHIFT)
> > >>   #endif /* CONFIG_MMU */
> > >>
> > >> @@ -110,29 +109,22 @@ extern unsigned long kernel_virt_addr;
> > >>
> > >>   #ifdef CONFIG_64BIT
> > >>   #define linear_mapping_pa_to_va(x) ((void *)((unsigned long)(x) + va_pa_offset))
> > >> -#ifdef CONFIG_XIP_KERNEL
> > >>   #define kernel_mapping_pa_to_va(y) ({                                              \
> > >>      unsigned long _y = y;                                                           \
> > >>      (_y >= CONFIG_PHYS_RAM_BASE) ?  
> > >
> > > This CONFIG_PHYS_RAM_BASE is only available for XIP, could result in a
> > > compiler error for !XIP?  
> >
> > You're right, I have this patch in my branch and forgot to squash it
> >  
> > >
> > > I'm also concerned with the unecessary overhead of kernel_mapping_pa_to_va()
> > > for !XIP case, there's a "if" condition branch, and extra symbol: va_kernel_xip_pa_offset  
> >
> > I understand your concerns even if I don't find that the overhead is
> > that important here, I prefer the readability improvement. I can always

For readability, we still can avoid introducing va_kernel_xip_pa_offset
symbol by simply define va_kernel_xip_pa_offset as 0 if XIP as you did
for XIP_OFFSET

PS: this may need a preparation patch:
http://lists.infradead.org/pipermail/linux-riscv/2021-June/006802.html

> > add unlikely/likely builtin to improve things or completely remove this
> > patch if others agree with you.  
> 
> I would also prefer readable code for long-term maintainability. Currently,
> the nested "#ifdefs" are increasing causing developers to easily break
> untested combinations.
> 
> Regards,
> Anup
> 
> >
> > Thanks,
> >
> > Alex
> >  
> > >  
> > >>              (void *)((unsigned long)(_y) + va_kernel_pa_offset + XIP_OFFSET) :      \
> > >>              (void *)((unsigned long)(_y) + va_kernel_xip_pa_offset);                \
> > >>      })
> > >> -#else
> > >> -#define kernel_mapping_pa_to_va(x)  ((void *)((unsigned long)(x) + va_kernel_pa_offset))
> > >> -#endif
> > >>   #define __pa_to_va_nodebug(x)              linear_mapping_pa_to_va(x)
> > >>
> > >>   #define linear_mapping_va_to_pa(x) ((unsigned long)(x) - va_pa_offset)
> > >> -#ifdef CONFIG_XIP_KERNEL
> > >>   #define kernel_mapping_va_to_pa(y) ({                                              \
> > >>      unsigned long _y = y;                                                   \
> > >>      (_y < kernel_virt_addr + XIP_OFFSET) ?                                  \
> > >>              ((unsigned long)(_y) - va_kernel_xip_pa_offset) :               \
> > >>              ((unsigned long)(_y) - va_kernel_pa_offset - XIP_OFFSET);       \
> > >>      })  
> > >
> > > Similar as kernel_mapping_pa_to_va(), an overhead of "if" condition branch
> > > for !XIP and extra va_kernel_xip_pa_offset symbol.
> > >  
> > >> -#else
> > >> -#define kernel_mapping_va_to_pa(x)  ((unsigned long)(x) - va_kernel_pa_offset)
> > >> -#endif
> > >> +
> > >>   #define __va_to_pa_nodebug(x)      ({                                              \
> > >>      unsigned long _x = x;                                                   \
> > >>      (_x < kernel_virt_addr) ?                                               \
> > >> @@ -141,7 +133,7 @@ extern unsigned long kernel_virt_addr;
> > >>   #else
> > >>   #define __pa_to_va_nodebug(x)  ((void *)((unsigned long) (x) + va_pa_offset))
> > >>   #define __va_to_pa_nodebug(x)  ((unsigned long)(x) - va_pa_offset)
> > >> -#endif
> > >> +#endif /* CONFIG_64BIT */
> > >>
> > >>   #ifdef CONFIG_DEBUG_VIRTUAL
> > >>   extern phys_addr_t __virt_to_phys(unsigned long x);
> > >> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > >> index bde8ce3bfe7c..d98e931a31e5 100644
> > >> --- a/arch/riscv/include/asm/pgtable.h
> > >> +++ b/arch/riscv/include/asm/pgtable.h
> > >> @@ -77,6 +77,8 @@
> > >>
> > >>   #ifdef CONFIG_XIP_KERNEL
> > >>   #define XIP_OFFSET         SZ_8M
> > >> +#else
> > >> +#define XIP_OFFSET          0
> > >>   #endif
> > >>
> > >>   #ifndef __ASSEMBLY__  
> > >
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > >
Alex Ghiti June 3, 2021, 3:06 p.m. UTC | #7
Le 3/06/2021 à 15:53, Jisheng Zhang a écrit :
> On Thu, 3 Jun 2021 18:46:47 +0530
> Anup Patel <anup@brainfault.org> wrote:
> 
>> On Thu, Jun 3, 2021 at 6:27 PM Alex Ghiti <alex@ghiti.fr> wrote:
>>>
>>> Hi Jisheng,
> 
> Hi,
> 
>>>
>>> Le 3/06/2021 à 14:27, Jisheng Zhang a écrit :
>>>> On Thu,  3 Jun 2021 10:27:47 +0200
>>>> Alexandre Ghiti <alex@ghiti.fr> wrote:
>>>>   
>>>>> To simplify the kernel address conversion code, make the same definition of
>>>>> kernel_mapping_pa_to_va and kernel_mapping_va_to_pa compatible for both xip
>>>>> and !xip kernel by defining XIP_OFFSET to 0 in !xip kernel.
>>>>>
>>>>> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
>>>>> ---
>>>>>    arch/riscv/include/asm/page.h    | 14 +++-----------
>>>>>    arch/riscv/include/asm/pgtable.h |  2 ++
>>>>>    2 files changed, 5 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
>>>>> index 6a7761c86ec2..6e004d8fda4d 100644
>>>>> --- a/arch/riscv/include/asm/page.h
>>>>> +++ b/arch/riscv/include/asm/page.h
>>>>> @@ -93,9 +93,7 @@ extern unsigned long va_pa_offset;
>>>>>    #ifdef CONFIG_64BIT
>>>>>    extern unsigned long va_kernel_pa_offset;
>>>>>    #endif
>>>>> -#ifdef CONFIG_XIP_KERNEL
>>>>>    extern unsigned long va_kernel_xip_pa_offset;
>>>>> -#endif
>>>>>    extern unsigned long pfn_base;
>>>>>    #define ARCH_PFN_OFFSET            (pfn_base)
>>>>>    #else
>>>>> @@ -103,6 +101,7 @@ extern unsigned long pfn_base;
>>>>>    #ifdef CONFIG_64BIT
>>>>>    #define va_kernel_pa_offset        0
>>>>>    #endif
>>>>> +#define va_kernel_xip_pa_offset 0
>>>>>    #define ARCH_PFN_OFFSET            (PAGE_OFFSET >> PAGE_SHIFT)
>>>>>    #endif /* CONFIG_MMU */
>>>>>
>>>>> @@ -110,29 +109,22 @@ extern unsigned long kernel_virt_addr;
>>>>>
>>>>>    #ifdef CONFIG_64BIT
>>>>>    #define linear_mapping_pa_to_va(x) ((void *)((unsigned long)(x) + va_pa_offset))
>>>>> -#ifdef CONFIG_XIP_KERNEL
>>>>>    #define kernel_mapping_pa_to_va(y) ({                                              \
>>>>>       unsigned long _y = y;                                                           \
>>>>>       (_y >= CONFIG_PHYS_RAM_BASE) ?
>>>>
>>>> This CONFIG_PHYS_RAM_BASE is only available for XIP, could result in a
>>>> compiler error for !XIP?
>>>
>>> You're right, I have this patch in my branch and forgot to squash it
>>>   
>>>>
>>>> I'm also concerned with the unecessary overhead of kernel_mapping_pa_to_va()
>>>> for !XIP case, there's a "if" condition branch, and extra symbol: va_kernel_xip_pa_offset
>>>
>>> I understand your concerns even if I don't find that the overhead is
>>> that important here, I prefer the readability improvement. I can always
> 
> For readability, we still can avoid introducing va_kernel_xip_pa_offset
> symbol by simply define va_kernel_xip_pa_offset as 0 if XIP as you did
> for XIP_OFFSET
> 
> PS: this may need a preparation patch:
> http://lists.infradead.org/pipermail/linux-riscv/2021-June/006802.html

IIUC, that won't improve readability, just avoid to allocate 
va_kernel_xip_pa_offset in !XIP kernel right?

> 
>>> add unlikely/likely builtin to improve things or completely remove this
>>> patch if others agree with you.
>>
>> I would also prefer readable code for long-term maintainability. Currently,
>> the nested "#ifdefs" are increasing causing developers to easily break
>> untested combinations.
>>
>> Regards,
>> Anup
>>
>>>
>>> Thanks,
>>>
>>> Alex
>>>   
>>>>   
>>>>>               (void *)((unsigned long)(_y) + va_kernel_pa_offset + XIP_OFFSET) :      \
>>>>>               (void *)((unsigned long)(_y) + va_kernel_xip_pa_offset);                \
>>>>>       })
>>>>> -#else
>>>>> -#define kernel_mapping_pa_to_va(x)  ((void *)((unsigned long)(x) + va_kernel_pa_offset))
>>>>> -#endif
>>>>>    #define __pa_to_va_nodebug(x)              linear_mapping_pa_to_va(x)
>>>>>
>>>>>    #define linear_mapping_va_to_pa(x) ((unsigned long)(x) - va_pa_offset)
>>>>> -#ifdef CONFIG_XIP_KERNEL
>>>>>    #define kernel_mapping_va_to_pa(y) ({                                              \
>>>>>       unsigned long _y = y;                                                   \
>>>>>       (_y < kernel_virt_addr + XIP_OFFSET) ?                                  \
>>>>>               ((unsigned long)(_y) - va_kernel_xip_pa_offset) :               \
>>>>>               ((unsigned long)(_y) - va_kernel_pa_offset - XIP_OFFSET);       \
>>>>>       })
>>>>
>>>> Similar as kernel_mapping_pa_to_va(), an overhead of "if" condition branch
>>>> for !XIP and extra va_kernel_xip_pa_offset symbol.
>>>>   
>>>>> -#else
>>>>> -#define kernel_mapping_va_to_pa(x)  ((unsigned long)(x) - va_kernel_pa_offset)
>>>>> -#endif
>>>>> +
>>>>>    #define __va_to_pa_nodebug(x)      ({                                              \
>>>>>       unsigned long _x = x;                                                   \
>>>>>       (_x < kernel_virt_addr) ?                                               \
>>>>> @@ -141,7 +133,7 @@ extern unsigned long kernel_virt_addr;
>>>>>    #else
>>>>>    #define __pa_to_va_nodebug(x)  ((void *)((unsigned long) (x) + va_pa_offset))
>>>>>    #define __va_to_pa_nodebug(x)  ((unsigned long)(x) - va_pa_offset)
>>>>> -#endif
>>>>> +#endif /* CONFIG_64BIT */
>>>>>
>>>>>    #ifdef CONFIG_DEBUG_VIRTUAL
>>>>>    extern phys_addr_t __virt_to_phys(unsigned long x);
>>>>> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
>>>>> index bde8ce3bfe7c..d98e931a31e5 100644
>>>>> --- a/arch/riscv/include/asm/pgtable.h
>>>>> +++ b/arch/riscv/include/asm/pgtable.h
>>>>> @@ -77,6 +77,8 @@
>>>>>
>>>>>    #ifdef CONFIG_XIP_KERNEL
>>>>>    #define XIP_OFFSET         SZ_8M
>>>>> +#else
>>>>> +#define XIP_OFFSET          0
>>>>>    #endif
>>>>>
>>>>>    #ifndef __ASSEMBLY__
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> linux-riscv mailing list
>>>> linux-riscv@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>>>   
> 
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
Jisheng Zhang June 3, 2021, 3:16 p.m. UTC | #8
On Thu, 3 Jun 2021 17:06:39 +0200
Alex Ghiti <alex@ghiti.fr> wrote:

> Le 3/06/2021 à 15:53, Jisheng Zhang a écrit :
> > On Thu, 3 Jun 2021 18:46:47 +0530
> > Anup Patel <anup@brainfault.org> wrote:
> >   
> >> On Thu, Jun 3, 2021 at 6:27 PM Alex Ghiti <alex@ghiti.fr> wrote:  
> >>>
> >>> Hi Jisheng,  
> > 
> > Hi,
> >   
> >>>
> >>> Le 3/06/2021 à 14:27, Jisheng Zhang a écrit :  
> >>>> On Thu,  3 Jun 2021 10:27:47 +0200
> >>>> Alexandre Ghiti <alex@ghiti.fr> wrote:
> >>>>     
> >>>>> To simplify the kernel address conversion code, make the same definition of
> >>>>> kernel_mapping_pa_to_va and kernel_mapping_va_to_pa compatible for both xip
> >>>>> and !xip kernel by defining XIP_OFFSET to 0 in !xip kernel.
> >>>>>
> >>>>> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> >>>>> ---
> >>>>>    arch/riscv/include/asm/page.h    | 14 +++-----------
> >>>>>    arch/riscv/include/asm/pgtable.h |  2 ++
> >>>>>    2 files changed, 5 insertions(+), 11 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> >>>>> index 6a7761c86ec2..6e004d8fda4d 100644
> >>>>> --- a/arch/riscv/include/asm/page.h
> >>>>> +++ b/arch/riscv/include/asm/page.h
> >>>>> @@ -93,9 +93,7 @@ extern unsigned long va_pa_offset;
> >>>>>    #ifdef CONFIG_64BIT
> >>>>>    extern unsigned long va_kernel_pa_offset;
> >>>>>    #endif
> >>>>> -#ifdef CONFIG_XIP_KERNEL
> >>>>>    extern unsigned long va_kernel_xip_pa_offset;
> >>>>> -#endif
> >>>>>    extern unsigned long pfn_base;
> >>>>>    #define ARCH_PFN_OFFSET            (pfn_base)
> >>>>>    #else
> >>>>> @@ -103,6 +101,7 @@ extern unsigned long pfn_base;
> >>>>>    #ifdef CONFIG_64BIT
> >>>>>    #define va_kernel_pa_offset        0
> >>>>>    #endif
> >>>>> +#define va_kernel_xip_pa_offset 0
> >>>>>    #define ARCH_PFN_OFFSET            (PAGE_OFFSET >> PAGE_SHIFT)
> >>>>>    #endif /* CONFIG_MMU */
> >>>>>
> >>>>> @@ -110,29 +109,22 @@ extern unsigned long kernel_virt_addr;
> >>>>>
> >>>>>    #ifdef CONFIG_64BIT
> >>>>>    #define linear_mapping_pa_to_va(x) ((void *)((unsigned long)(x) + va_pa_offset))
> >>>>> -#ifdef CONFIG_XIP_KERNEL
> >>>>>    #define kernel_mapping_pa_to_va(y) ({                                              \
> >>>>>       unsigned long _y = y;                                                           \
> >>>>>       (_y >= CONFIG_PHYS_RAM_BASE) ?  
> >>>>
> >>>> This CONFIG_PHYS_RAM_BASE is only available for XIP, could result in a
> >>>> compiler error for !XIP?  
> >>>
> >>> You're right, I have this patch in my branch and forgot to squash it
> >>>     
> >>>>
> >>>> I'm also concerned with the unecessary overhead of kernel_mapping_pa_to_va()
> >>>> for !XIP case, there's a "if" condition branch, and extra symbol: va_kernel_xip_pa_offset  
> >>>
> >>> I understand your concerns even if I don't find that the overhead is
> >>> that important here, I prefer the readability improvement. I can always  
> > 
> > For readability, we still can avoid introducing va_kernel_xip_pa_offset
> > symbol by simply define va_kernel_xip_pa_offset as 0 if XIP as you did
> > for XIP_OFFSET
> > 
> > PS: this may need a preparation patch:
> > http://lists.infradead.org/pipermail/linux-riscv/2021-June/006802.html  
> 
> IIUC, that won't improve readability, just avoid to allocate 
> va_kernel_xip_pa_offset in !XIP kernel right?

I mean even we can improve code readability while still avoid the
va_kernel_xip_pa_offset symbol for !XIP case. Probably it's implemented
as you did for XIP_OFFSET:

#ifdef CONFIG_XIP_KERNEL
extern unsigned long va_kernel_xip_pa_offset;
#else
#define va_kernel_xip_pa_offset 0
#endif

But since currently va_kernel_xip_pa_offset always exisits no matter XIP
is enabled or not, so you may need the preparation patch to clean up, otherwise
there may be compiler error.
> 
> >   
> >>> add unlikely/likely builtin to improve things or completely remove this
> >>> patch if others agree with you.  
> >>
> >> I would also prefer readable code for long-term maintainability. Currently,
> >> the nested "#ifdefs" are increasing causing developers to easily break
> >> untested combinations.
> >>
> >> Regards,
> >> Anup
> >>  
> >>>
> >>> Thanks,
> >>>
> >>> Alex
> >>>     
> >>>>     
> >>>>>               (void *)((unsigned long)(_y) + va_kernel_pa_offset + XIP_OFFSET) :      \
> >>>>>               (void *)((unsigned long)(_y) + va_kernel_xip_pa_offset);                \
> >>>>>       })
> >>>>> -#else
> >>>>> -#define kernel_mapping_pa_to_va(x)  ((void *)((unsigned long)(x) + va_kernel_pa_offset))
> >>>>> -#endif
> >>>>>    #define __pa_to_va_nodebug(x)              linear_mapping_pa_to_va(x)
> >>>>>
> >>>>>    #define linear_mapping_va_to_pa(x) ((unsigned long)(x) - va_pa_offset)
> >>>>> -#ifdef CONFIG_XIP_KERNEL
> >>>>>    #define kernel_mapping_va_to_pa(y) ({                                              \
> >>>>>       unsigned long _y = y;                                                   \
> >>>>>       (_y < kernel_virt_addr + XIP_OFFSET) ?                                  \
> >>>>>               ((unsigned long)(_y) - va_kernel_xip_pa_offset) :               \
> >>>>>               ((unsigned long)(_y) - va_kernel_pa_offset - XIP_OFFSET);       \
> >>>>>       })  
> >>>>
> >>>> Similar as kernel_mapping_pa_to_va(), an overhead of "if" condition branch
> >>>> for !XIP and extra va_kernel_xip_pa_offset symbol.
> >>>>     
> >>>>> -#else
> >>>>> -#define kernel_mapping_va_to_pa(x)  ((unsigned long)(x) - va_kernel_pa_offset)
> >>>>> -#endif
> >>>>> +
> >>>>>    #define __va_to_pa_nodebug(x)      ({                                              \
> >>>>>       unsigned long _x = x;                                                   \
> >>>>>       (_x < kernel_virt_addr) ?                                               \
> >>>>> @@ -141,7 +133,7 @@ extern unsigned long kernel_virt_addr;
> >>>>>    #else
> >>>>>    #define __pa_to_va_nodebug(x)  ((void *)((unsigned long) (x) + va_pa_offset))
> >>>>>    #define __va_to_pa_nodebug(x)  ((unsigned long)(x) - va_pa_offset)
> >>>>> -#endif
> >>>>> +#endif /* CONFIG_64BIT */
> >>>>>
> >>>>>    #ifdef CONFIG_DEBUG_VIRTUAL
> >>>>>    extern phys_addr_t __virt_to_phys(unsigned long x);
> >>>>> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> >>>>> index bde8ce3bfe7c..d98e931a31e5 100644
> >>>>> --- a/arch/riscv/include/asm/pgtable.h
> >>>>> +++ b/arch/riscv/include/asm/pgtable.h
> >>>>> @@ -77,6 +77,8 @@
> >>>>>
> >>>>>    #ifdef CONFIG_XIP_KERNEL
> >>>>>    #define XIP_OFFSET         SZ_8M
> >>>>> +#else
> >>>>> +#define XIP_OFFSET          0
> >>>>>    #endif
> >>>>>
> >>>>>    #ifndef __ASSEMBLY__  
> >>>>
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> linux-riscv mailing list
> >>>> linux-riscv@lists.infradead.org
> >>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> >>>>     
> > 
> > 
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
Jisheng Zhang June 3, 2021, 3:49 p.m. UTC | #9
On Thu, 3 Jun 2021 17:51:25 +0200
Vitaly Wool <vitaly.wool@konsulko.com> wrote:

> On Thu, Jun 3, 2021, 14:57 Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
> >
> > On Thu, 3 Jun 2021 20:27:48 +0800
> > Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
> >  
> > > On Thu,  3 Jun 2021 10:27:47 +0200
> > > Alexandre Ghiti <alex@ghiti.fr> wrote:
> > >  
> > > > To simplify the kernel address conversion code, make the same definition of
> > > > kernel_mapping_pa_to_va and kernel_mapping_va_to_pa compatible for both xip
> > > > and !xip kernel by defining XIP_OFFSET to 0 in !xip kernel.
> > > >
> > > > Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> > > > ---
> > > >  arch/riscv/include/asm/page.h    | 14 +++-----------
> > > >  arch/riscv/include/asm/pgtable.h |  2 ++
> > > >  2 files changed, 5 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > > > index 6a7761c86ec2..6e004d8fda4d 100644
> > > > --- a/arch/riscv/include/asm/page.h
> > > > +++ b/arch/riscv/include/asm/page.h
> > > > @@ -93,9 +93,7 @@ extern unsigned long va_pa_offset;
> > > >  #ifdef CONFIG_64BIT
> > > >  extern unsigned long va_kernel_pa_offset;
> > > >  #endif
> > > > -#ifdef CONFIG_XIP_KERNEL
> > > >  extern unsigned long va_kernel_xip_pa_offset;
> > > > -#endif
> > > >  extern unsigned long pfn_base;
> > > >  #define ARCH_PFN_OFFSET            (pfn_base)
> > > >  #else
> > > > @@ -103,6 +101,7 @@ extern unsigned long pfn_base;
> > > >  #ifdef CONFIG_64BIT
> > > >  #define va_kernel_pa_offset        0
> > > >  #endif
> > > > +#define va_kernel_xip_pa_offset 0
> > > >  #define ARCH_PFN_OFFSET            (PAGE_OFFSET >> PAGE_SHIFT)
> > > >  #endif /* CONFIG_MMU */
> > > >
> > > > @@ -110,29 +109,22 @@ extern unsigned long kernel_virt_addr;
> > > >
> > > >  #ifdef CONFIG_64BIT
> > > >  #define linear_mapping_pa_to_va(x) ((void *)((unsigned long)(x) + va_pa_offset))
> > > > -#ifdef CONFIG_XIP_KERNEL
> > > >  #define kernel_mapping_pa_to_va(y) ({                                              \
> > > >     unsigned long _y = y;                                                           \
> > > >     (_y >= CONFIG_PHYS_RAM_BASE) ?  
> > >
> > > This CONFIG_PHYS_RAM_BASE is only available for XIP, could result in a
> > > compiler error for !XIP?
> > >
> > > I'm also concerned with the unecessary overhead of kernel_mapping_pa_to_va()
> > > for !XIP case, there's a "if" condition branch, and extra symbol: va_kernel_xip_pa_offset  
> >
> > Err, I just found this symobl always exists no matter XIP is enabled or not.
> > I will send out a patch for this clean up  
> 
> What cleanup?
> 

http://lists.infradead.org/pipermail/linux-riscv/2021-June/006802.html
Vitaly Wool June 3, 2021, 3:51 p.m. UTC | #10
On Thu, Jun 3, 2021, 14:57 Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
>
> On Thu, 3 Jun 2021 20:27:48 +0800
> Jisheng Zhang <jszhang3@mail.ustc.edu.cn> wrote:
>
> > On Thu,  3 Jun 2021 10:27:47 +0200
> > Alexandre Ghiti <alex@ghiti.fr> wrote:
> >
> > > To simplify the kernel address conversion code, make the same definition of
> > > kernel_mapping_pa_to_va and kernel_mapping_va_to_pa compatible for both xip
> > > and !xip kernel by defining XIP_OFFSET to 0 in !xip kernel.
> > >
> > > Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> > > ---
> > >  arch/riscv/include/asm/page.h    | 14 +++-----------
> > >  arch/riscv/include/asm/pgtable.h |  2 ++
> > >  2 files changed, 5 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > > index 6a7761c86ec2..6e004d8fda4d 100644
> > > --- a/arch/riscv/include/asm/page.h
> > > +++ b/arch/riscv/include/asm/page.h
> > > @@ -93,9 +93,7 @@ extern unsigned long va_pa_offset;
> > >  #ifdef CONFIG_64BIT
> > >  extern unsigned long va_kernel_pa_offset;
> > >  #endif
> > > -#ifdef CONFIG_XIP_KERNEL
> > >  extern unsigned long va_kernel_xip_pa_offset;
> > > -#endif
> > >  extern unsigned long pfn_base;
> > >  #define ARCH_PFN_OFFSET            (pfn_base)
> > >  #else
> > > @@ -103,6 +101,7 @@ extern unsigned long pfn_base;
> > >  #ifdef CONFIG_64BIT
> > >  #define va_kernel_pa_offset        0
> > >  #endif
> > > +#define va_kernel_xip_pa_offset 0
> > >  #define ARCH_PFN_OFFSET            (PAGE_OFFSET >> PAGE_SHIFT)
> > >  #endif /* CONFIG_MMU */
> > >
> > > @@ -110,29 +109,22 @@ extern unsigned long kernel_virt_addr;
> > >
> > >  #ifdef CONFIG_64BIT
> > >  #define linear_mapping_pa_to_va(x) ((void *)((unsigned long)(x) + va_pa_offset))
> > > -#ifdef CONFIG_XIP_KERNEL
> > >  #define kernel_mapping_pa_to_va(y) ({                                              \
> > >     unsigned long _y = y;                                                           \
> > >     (_y >= CONFIG_PHYS_RAM_BASE) ?
> >
> > This CONFIG_PHYS_RAM_BASE is only available for XIP, could result in a
> > compiler error for !XIP?
> >
> > I'm also concerned with the unecessary overhead of kernel_mapping_pa_to_va()
> > for !XIP case, there's a "if" condition branch, and extra symbol: va_kernel_xip_pa_offset
>
> Err, I just found this symobl always exists no matter XIP is enabled or not.
> I will send out a patch for this clean up

What cleanup?

Best regards,
   Vitaly

>
> >
> > >             (void *)((unsigned long)(_y) + va_kernel_pa_offset + XIP_OFFSET) :      \
> > >             (void *)((unsigned long)(_y) + va_kernel_xip_pa_offset);                \
> > >     })
> > > -#else
> > > -#define kernel_mapping_pa_to_va(x) ((void *)((unsigned long)(x) + va_kernel_pa_offset))
> > > -#endif
> > >  #define __pa_to_va_nodebug(x)              linear_mapping_pa_to_va(x)
> > >
> > >  #define linear_mapping_va_to_pa(x) ((unsigned long)(x) - va_pa_offset)
> > > -#ifdef CONFIG_XIP_KERNEL
> > >  #define kernel_mapping_va_to_pa(y) ({                                              \
> > >     unsigned long _y = y;                                                   \
> > >     (_y < kernel_virt_addr + XIP_OFFSET) ?                                  \
> > >             ((unsigned long)(_y) - va_kernel_xip_pa_offset) :               \
> > >             ((unsigned long)(_y) - va_kernel_pa_offset - XIP_OFFSET);       \
> > >     })
> >
> > Similar as kernel_mapping_pa_to_va(), an overhead of "if" condition branch
> > for !XIP and extra va_kernel_xip_pa_offset symbol.
> >
> > > -#else
> > > -#define kernel_mapping_va_to_pa(x) ((unsigned long)(x) - va_kernel_pa_offset)
> > > -#endif
> > > +
> > >  #define __va_to_pa_nodebug(x)      ({                                              \
> > >     unsigned long _x = x;                                                   \
> > >     (_x < kernel_virt_addr) ?                                               \
> > > @@ -141,7 +133,7 @@ extern unsigned long kernel_virt_addr;
> > >  #else
> > >  #define __pa_to_va_nodebug(x)  ((void *)((unsigned long) (x) + va_pa_offset))
> > >  #define __va_to_pa_nodebug(x)  ((unsigned long)(x) - va_pa_offset)
> > > -#endif
> > > +#endif /* CONFIG_64BIT */
> > >
> > >  #ifdef CONFIG_DEBUG_VIRTUAL
> > >  extern phys_addr_t __virt_to_phys(unsigned long x);
> > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > index bde8ce3bfe7c..d98e931a31e5 100644
> > > --- a/arch/riscv/include/asm/pgtable.h
> > > +++ b/arch/riscv/include/asm/pgtable.h
> > > @@ -77,6 +77,8 @@
> > >
> > >  #ifdef CONFIG_XIP_KERNEL
> > >  #define XIP_OFFSET         SZ_8M
> > > +#else
> > > +#define XIP_OFFSET         0
> > >  #endif
> > >
> > >  #ifndef __ASSEMBLY__
> >
> >
>
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
index 6a7761c86ec2..6e004d8fda4d 100644
--- a/arch/riscv/include/asm/page.h
+++ b/arch/riscv/include/asm/page.h
@@ -93,9 +93,7 @@  extern unsigned long va_pa_offset;
 #ifdef CONFIG_64BIT
 extern unsigned long va_kernel_pa_offset;
 #endif
-#ifdef CONFIG_XIP_KERNEL
 extern unsigned long va_kernel_xip_pa_offset;
-#endif
 extern unsigned long pfn_base;
 #define ARCH_PFN_OFFSET		(pfn_base)
 #else
@@ -103,6 +101,7 @@  extern unsigned long pfn_base;
 #ifdef CONFIG_64BIT
 #define va_kernel_pa_offset	0
 #endif
+#define va_kernel_xip_pa_offset 0
 #define ARCH_PFN_OFFSET		(PAGE_OFFSET >> PAGE_SHIFT)
 #endif /* CONFIG_MMU */
 
@@ -110,29 +109,22 @@  extern unsigned long kernel_virt_addr;
 
 #ifdef CONFIG_64BIT
 #define linear_mapping_pa_to_va(x)	((void *)((unsigned long)(x) + va_pa_offset))
-#ifdef CONFIG_XIP_KERNEL
 #define kernel_mapping_pa_to_va(y)	({						\
 	unsigned long _y = y;								\
 	(_y >= CONFIG_PHYS_RAM_BASE) ?							\
 		(void *)((unsigned long)(_y) + va_kernel_pa_offset + XIP_OFFSET) :	\
 		(void *)((unsigned long)(_y) + va_kernel_xip_pa_offset);		\
 	})
-#else
-#define kernel_mapping_pa_to_va(x)	((void *)((unsigned long)(x) + va_kernel_pa_offset))
-#endif
 #define __pa_to_va_nodebug(x)		linear_mapping_pa_to_va(x)
 
 #define linear_mapping_va_to_pa(x)	((unsigned long)(x) - va_pa_offset)
-#ifdef CONFIG_XIP_KERNEL
 #define kernel_mapping_va_to_pa(y) ({						\
 	unsigned long _y = y;							\
 	(_y < kernel_virt_addr + XIP_OFFSET) ?					\
 		((unsigned long)(_y) - va_kernel_xip_pa_offset) :		\
 		((unsigned long)(_y) - va_kernel_pa_offset - XIP_OFFSET);	\
 	})
-#else
-#define kernel_mapping_va_to_pa(x)	((unsigned long)(x) - va_kernel_pa_offset)
-#endif
+
 #define __va_to_pa_nodebug(x)	({						\
 	unsigned long _x = x;							\
 	(_x < kernel_virt_addr) ?						\
@@ -141,7 +133,7 @@  extern unsigned long kernel_virt_addr;
 #else
 #define __pa_to_va_nodebug(x)  ((void *)((unsigned long) (x) + va_pa_offset))
 #define __va_to_pa_nodebug(x)  ((unsigned long)(x) - va_pa_offset)
-#endif
+#endif /* CONFIG_64BIT */
 
 #ifdef CONFIG_DEBUG_VIRTUAL
 extern phys_addr_t __virt_to_phys(unsigned long x);
diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index bde8ce3bfe7c..d98e931a31e5 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -77,6 +77,8 @@ 
 
 #ifdef CONFIG_XIP_KERNEL
 #define XIP_OFFSET		SZ_8M
+#else
+#define XIP_OFFSET		0
 #endif
 
 #ifndef __ASSEMBLY__