| Message ID | 20210603082749.1256129-2-alex@ghiti.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: Map the kernel with correct permissions the first time | expand |
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 >
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__
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__ > >
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 >
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 > >
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 > > >
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 >
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 > >
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
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 --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__
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(-)