2023-12-20 10:34:49

by yunhui cui

[permalink] [raw]

Subject: [PATCH] riscv: put va_kernel_xip_pa_offset into CONFIG_XIP_KERNEL

---

opitmize the kernel_mapping_pa_to_va() and kernel_mapping_va_to_pa().

Signed-off-by: Yunhui Cui <[email protected]>
---
arch/riscv/include/asm/page.h | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
index 5488ecc337b6..0d2b479d02cd 100644
--- a/arch/riscv/include/asm/page.h
+++ b/arch/riscv/include/asm/page.h
@@ -113,8 +113,8 @@ struct kernel_mapping {
unsigned long va_pa_offset;
/* Offset between kernel mapping virtual address and kernel load address */
unsigned long va_kernel_pa_offset;
- unsigned long va_kernel_xip_pa_offset;
#ifdef CONFIG_XIP_KERNEL
+ unsigned long va_kernel_xip_pa_offset;
uintptr_t xiprom;
uintptr_t xiprom_sz;
#endif
@@ -134,12 +134,25 @@ extern phys_addr_t phys_ram_base;
#else
void *linear_mapping_pa_to_va(unsigned long x);
#endif
-#define kernel_mapping_pa_to_va(y) ({ \
- unsigned long _y = (unsigned long)(y); \
- (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < phys_ram_base) ? \
- (void *)(_y + kernel_map.va_kernel_xip_pa_offset) : \
- (void *)(_y + kernel_map.va_kernel_pa_offset + XIP_OFFSET); \
- })
+
+#ifdef CONFIG_XIP_KERNEL
+#define kernel_mapping_pa_to_va(y) \
+ (((unsigned long)(y) < phys_ram_base) ? \
+ (void *)((unsigned long)(y) + kernel_map.va_kernel_xip_pa_offset) : \
+ (void *)((unsigned long)(y) + kernel_map.va_kernel_pa_offset + XIP_OFFSET))
+
+#define kernel_mapping_va_to_pa(y) \
+ (((unsigned long)(y) < kernel_map.virt_addr + XIP_OFFSET) ? \
+ ((unsigned long)(y) - kernel_map.va_kernel_xip_pa_offset) : \
+ ((unsigned long)(y) - kernel_map.va_kernel_pa_offset - XIP_OFFSET))
+#else
+#define kernel_mapping_pa_to_va(y) \
+ ((void *)((unsigned long)(y) + kernel_map.va_kernel_pa_offset + XIP_OFFSET))
+
+#define kernel_mapping_va_to_pa(y) \
+ ((unsigned long)(y) - kernel_map.va_kernel_pa_offset - XIP_OFFSET)
+#endif
+
#define __pa_to_va_nodebug(x) linear_mapping_pa_to_va(x)

#ifndef CONFIG_DEBUG_VIRTUAL
@@ -147,12 +160,6 @@ void *linear_mapping_pa_to_va(unsigned long x);
#else
phys_addr_t linear_mapping_va_to_pa(unsigned long x);
#endif
-#define kernel_mapping_va_to_pa(y) ({ \
- unsigned long _y = (unsigned long)(y); \
- (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < kernel_map.virt_addr + XIP_OFFSET) ? \
- (_y - kernel_map.va_kernel_xip_pa_offset) : \
- (_y - kernel_map.va_kernel_pa_offset - XIP_OFFSET); \
- })

#define __va_to_pa_nodebug(x) ({ \
unsigned long _x = x; \
--
2.20.1

2023-12-20 21:15:15

by Alexandre Ghiti

[permalink] [raw]

Subject: Re: [PATCH] riscv: put va_kernel_xip_pa_offset into CONFIG_XIP_KERNEL

---

Hi Yunhui,

On 20/12/2023 11:34, Yunhui Cui wrote:
> opitmize the kernel_mapping_pa_to_va() and kernel_mapping_va_to_pa().
>
> Signed-off-by: Yunhui Cui <[email protected]>
> ---
> arch/riscv/include/asm/page.h | 33 ++++++++++++++++++++-------------
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> index 5488ecc337b6..0d2b479d02cd 100644
> --- a/arch/riscv/include/asm/page.h
> +++ b/arch/riscv/include/asm/page.h
> @@ -113,8 +113,8 @@ struct kernel_mapping {
> unsigned long va_pa_offset;
> /* Offset between kernel mapping virtual address and kernel load address */
> unsigned long va_kernel_pa_offset;
> - unsigned long va_kernel_xip_pa_offset;
> #ifdef CONFIG_XIP_KERNEL
> + unsigned long va_kernel_xip_pa_offset;
> uintptr_t xiprom;
> uintptr_t xiprom_sz;
> #endif
> @@ -134,12 +134,25 @@ extern phys_addr_t phys_ram_base;
> #else
> void *linear_mapping_pa_to_va(unsigned long x);
> #endif
> -#define kernel_mapping_pa_to_va(y) ({ \
> - unsigned long _y = (unsigned long)(y); \
> - (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < phys_ram_base) ? \
> - (void *)(_y + kernel_map.va_kernel_xip_pa_offset) : \
> - (void *)(_y + kernel_map.va_kernel_pa_offset + XIP_OFFSET); \
> - })
> +
> +#ifdef CONFIG_XIP_KERNEL
> +#define kernel_mapping_pa_to_va(y) \
> + (((unsigned long)(y) < phys_ram_base) ? \
> + (void *)((unsigned long)(y) + kernel_map.va_kernel_xip_pa_offset) : \
> + (void *)((unsigned long)(y) + kernel_map.va_kernel_pa_offset + XIP_OFFSET))
> +
> +#define kernel_mapping_va_to_pa(y) \
> + (((unsigned long)(y) < kernel_map.virt_addr + XIP_OFFSET) ? \
> + ((unsigned long)(y) - kernel_map.va_kernel_xip_pa_offset) : \
> + ((unsigned long)(y) - kernel_map.va_kernel_pa_offset - XIP_OFFSET))
> +#else
> +#define kernel_mapping_pa_to_va(y) \
> + ((void *)((unsigned long)(y) + kernel_map.va_kernel_pa_offset + XIP_OFFSET))
> +
> +#define kernel_mapping_va_to_pa(y) \
> + ((unsigned long)(y) - kernel_map.va_kernel_pa_offset - XIP_OFFSET)
> +#endif
> +
> #define __pa_to_va_nodebug(x) linear_mapping_pa_to_va(x)
>
> #ifndef CONFIG_DEBUG_VIRTUAL
> @@ -147,12 +160,6 @@ void *linear_mapping_pa_to_va(unsigned long x);
> #else
> phys_addr_t linear_mapping_va_to_pa(unsigned long x);
> #endif
> -#define kernel_mapping_va_to_pa(y) ({ \
> - unsigned long _y = (unsigned long)(y); \
> - (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < kernel_map.virt_addr + XIP_OFFSET) ? \
> - (_y - kernel_map.va_kernel_xip_pa_offset) : \
> - (_y - kernel_map.va_kernel_pa_offset - XIP_OFFSET); \
> - })
>
> #define __va_to_pa_nodebug(x) ({ \
> unsigned long _x = x; \


Not sure using #ifdef optimizes anything since the compiler should do
the same with the IS_ENABLED(CONFIG_XIP_KERNEL) and it does not really
improve the readability of this file which is already overloaded with
#ifdef, so I don't think this change is needed.

Thanks,

Alex

2023-12-20 22:35:03

by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH] riscv: put va_kernel_xip_pa_offset into CONFIG_XIP_KERNEL

---

On Wed, Dec 20, 2023 at 10:14:59PM +0100, Alexandre Ghiti wrote:
> Hi Yunhui,
>
> On 20/12/2023 11:34, Yunhui Cui wrote:
> > opitmize the kernel_mapping_pa_to_va() and kernel_mapping_va_to_pa().
> >
> > Signed-off-by: Yunhui Cui <[email protected]>
> > ---
> > arch/riscv/include/asm/page.h | 33 ++++++++++++++++++++-------------
> > 1 file changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > index 5488ecc337b6..0d2b479d02cd 100644
> > --- a/arch/riscv/include/asm/page.h
> > +++ b/arch/riscv/include/asm/page.h
> > @@ -113,8 +113,8 @@ struct kernel_mapping {
> > unsigned long va_pa_offset;
> > /* Offset between kernel mapping virtual address and kernel load address */
> > unsigned long va_kernel_pa_offset;
> > - unsigned long va_kernel_xip_pa_offset;
> > #ifdef CONFIG_XIP_KERNEL
> > + unsigned long va_kernel_xip_pa_offset;
> > uintptr_t xiprom;
> > uintptr_t xiprom_sz;
> > #endif
> > @@ -134,12 +134,25 @@ extern phys_addr_t phys_ram_base;
> > #else
> > void *linear_mapping_pa_to_va(unsigned long x);
> > #endif
> > -#define kernel_mapping_pa_to_va(y) ({ \
> > - unsigned long _y = (unsigned long)(y); \
> > - (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < phys_ram_base) ? \
> > - (void *)(_y + kernel_map.va_kernel_xip_pa_offset) : \
> > - (void *)(_y + kernel_map.va_kernel_pa_offset + XIP_OFFSET); \
> > - })
> > +
> > +#ifdef CONFIG_XIP_KERNEL
> > +#define kernel_mapping_pa_to_va(y) \
> > + (((unsigned long)(y) < phys_ram_base) ? \
> > + (void *)((unsigned long)(y) + kernel_map.va_kernel_xip_pa_offset) : \
> > + (void *)((unsigned long)(y) + kernel_map.va_kernel_pa_offset + XIP_OFFSET))
> > +
> > +#define kernel_mapping_va_to_pa(y) \
> > + (((unsigned long)(y) < kernel_map.virt_addr + XIP_OFFSET) ? \
> > + ((unsigned long)(y) - kernel_map.va_kernel_xip_pa_offset) : \
> > + ((unsigned long)(y) - kernel_map.va_kernel_pa_offset - XIP_OFFSET))
> > +#else
> > +#define kernel_mapping_pa_to_va(y) \
> > + ((void *)((unsigned long)(y) + kernel_map.va_kernel_pa_offset + XIP_OFFSET))
> > +
> > +#define kernel_mapping_va_to_pa(y) \
> > + ((unsigned long)(y) - kernel_map.va_kernel_pa_offset - XIP_OFFSET)
> > +#endif
> > +
> > #define __pa_to_va_nodebug(x) linear_mapping_pa_to_va(x)
> > #ifndef CONFIG_DEBUG_VIRTUAL
> > @@ -147,12 +160,6 @@ void *linear_mapping_pa_to_va(unsigned long x);
> > #else
> > phys_addr_t linear_mapping_va_to_pa(unsigned long x);
> > #endif
> > -#define kernel_mapping_va_to_pa(y) ({ \
> > - unsigned long _y = (unsigned long)(y); \
> > - (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < kernel_map.virt_addr + XIP_OFFSET) ? \
> > - (_y - kernel_map.va_kernel_xip_pa_offset) : \
> > - (_y - kernel_map.va_kernel_pa_offset - XIP_OFFSET); \
> > - })
> > #define __va_to_pa_nodebug(x) ({ \
> > unsigned long _x = x; \
>
>
> Not sure using #ifdef optimizes anything since the compiler should do the
> same with the IS_ENABLED(CONFIG_XIP_KERNEL) and it does not really improve
> the readability of this file which is already overloaded with #ifdef, so I
> don't think this change is needed.

I would say that we explicitly do not want to move things that are
guarded by IS_ENABLED() to ifdeffery. In fact, we should move things in
the other direction if possible, especially for stuff like XIP_KERNEL
that nobody ever build tests.

---

Attachments:

(No filename) (3.45 kB)
signature.asc (235.00 B)
Download all attachments

2023-12-21 01:52:00

by yunhui cui

[permalink] [raw]

Subject: Re: [External] Re: [PATCH] riscv: put va_kernel_xip_pa_offset into CONFIG_XIP_KERNEL

---

Hi Conor，Alex

On Thu, Dec 21, 2023 at 6:35 AM Conor Dooley <[email protected]> wrote:
>
> On Wed, Dec 20, 2023 at 10:14:59PM +0100, Alexandre Ghiti wrote:
> > Hi Yunhui,
> >
> > On 20/12/2023 11:34, Yunhui Cui wrote:
> > > opitmize the kernel_mapping_pa_to_va() and kernel_mapping_va_to_pa().
> > >
> > > Signed-off-by: Yunhui Cui <[email protected]>
> > > ---
> > > arch/riscv/include/asm/page.h | 33 ++++++++++++++++++++-------------
> > > 1 file changed, 20 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > > index 5488ecc337b6..0d2b479d02cd 100644
> > > --- a/arch/riscv/include/asm/page.h
> > > +++ b/arch/riscv/include/asm/page.h
> > > @@ -113,8 +113,8 @@ struct kernel_mapping {
> > > unsigned long va_pa_offset;
> > > /* Offset between kernel mapping virtual address and kernel load address */
> > > unsigned long va_kernel_pa_offset;
> > > - unsigned long va_kernel_xip_pa_offset;
> > > #ifdef CONFIG_XIP_KERNEL
> > > + unsigned long va_kernel_xip_pa_offset;
> > > uintptr_t xiprom;
> > > uintptr_t xiprom_sz;
> > > #endif
> > > @@ -134,12 +134,25 @@ extern phys_addr_t phys_ram_base;
> > > #else
> > > void *linear_mapping_pa_to_va(unsigned long x);
> > > #endif
> > > -#define kernel_mapping_pa_to_va(y) ({ \
> > > - unsigned long _y = (unsigned long)(y); \
> > > - (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < phys_ram_base) ? \
> > > - (void *)(_y + kernel_map.va_kernel_xip_pa_offset) : \
> > > - (void *)(_y + kernel_map.va_kernel_pa_offset + XIP_OFFSET); \
> > > - })
> > > +
> > > +#ifdef CONFIG_XIP_KERNEL
> > > +#define kernel_mapping_pa_to_va(y) \
> > > + (((unsigned long)(y) < phys_ram_base) ? \
> > > + (void *)((unsigned long)(y) + kernel_map.va_kernel_xip_pa_offset) : \
> > > + (void *)((unsigned long)(y) + kernel_map.va_kernel_pa_offset + XIP_OFFSET))
> > > +
> > > +#define kernel_mapping_va_to_pa(y) \
> > > + (((unsigned long)(y) < kernel_map.virt_addr + XIP_OFFSET) ? \
> > > + ((unsigned long)(y) - kernel_map.va_kernel_xip_pa_offset) : \
> > > + ((unsigned long)(y) - kernel_map.va_kernel_pa_offset - XIP_OFFSET))
> > > +#else
> > > +#define kernel_mapping_pa_to_va(y) \
> > > + ((void *)((unsigned long)(y) + kernel_map.va_kernel_pa_offset + XIP_OFFSET))
> > > +
> > > +#define kernel_mapping_va_to_pa(y) \
> > > + ((unsigned long)(y) - kernel_map.va_kernel_pa_offset - XIP_OFFSET)
> > > +#endif
> > > +
> > > #define __pa_to_va_nodebug(x) linear_mapping_pa_to_va(x)
> > > #ifndef CONFIG_DEBUG_VIRTUAL
> > > @@ -147,12 +160,6 @@ void *linear_mapping_pa_to_va(unsigned long x);
> > > #else
> > > phys_addr_t linear_mapping_va_to_pa(unsigned long x);
> > > #endif
> > > -#define kernel_mapping_va_to_pa(y) ({ \
> > > - unsigned long _y = (unsigned long)(y); \
> > > - (IS_ENABLED(CONFIG_XIP_KERNEL) && _y < kernel_map.virt_addr + XIP_OFFSET) ? \
> > > - (_y - kernel_map.va_kernel_xip_pa_offset) : \
> > > - (_y - kernel_map.va_kernel_pa_offset - XIP_OFFSET); \
> > > - })
> > > #define __va_to_pa_nodebug(x) ({ \
> > > unsigned long _x = x; \
> >
> >
> > Not sure using #ifdef optimizes anything since the compiler should do the
> > same with the IS_ENABLED(CONFIG_XIP_KERNEL) and it does not really improve
> > the readability of this file which is already overloaded with #ifdef, so I
> > don't think this change is needed.
>
> I would say that we explicitly do not want to move things that are
> guarded by IS_ENABLED() to ifdeffery. In fact, we should move things in
> the other direction if possible, especially for stuff like XIP_KERNEL
> that nobody ever build tests.

The point of this patch is that logically, like xiprom and xiprom_sz,
stuct kernel_mapping.va_kernel_xip_pa_offset should be included in
CONFIG_XIP_KERNEL.

I believe we can agree on this, right?

Thanks,
Yunhui