[PATCH] powerpc: Fix preserved memory size for int-vectors

[PATCH] powerpc: Fix preserved memory size for int-vectors

[GUO Zihua]

The first 32k of memory is reserved for interrupt vectors, however for
powerpc64 this might not be enough. Fix this by reserving the maximum
size between 32k and the real size of interrupt vectors.

Signed-off-by: GUO Zihua <guozihua@huawei.com>
---
 arch/powerpc/kernel/prom.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 0b5878c3125b..f374487513b3 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -758,6 +758,7 @@ static inline void save_fscr_to_task(void) {}
 void __init early_init_devtree(void *params)
 {
 	phys_addr_t limit;
+	size_t int_vector_size;
 
 	DBG(" -> early_init_devtree(%px)\n", params);
 
@@ -810,9 +811,17 @@ void __init early_init_devtree(void *params)
 	setup_initial_memory_limit(memstart_addr, first_memblock_size);
 	/* Reserve MEMBLOCK regions used by kernel, initrd, dt, etc... */
 	memblock_reserve(PHYSICAL_START, __pa(_end) - PHYSICAL_START);
+#ifdef CONFIG_PPC64
+	/* If relocatable, reserve at least 32k for interrupt vectors etc. */
+	int_vector_size = (size_t)((uintptr_t)__end_interrupts -
+				   (uintptr_t)_stext);
+	int_vector_size = max_t(size_t, 0x8000, int_vector_size);
+#else
 	/* If relocatable, reserve first 32k for interrupt vectors etc. */
+	int_vector_size = 0x8000;
+#endif
 	if (PHYSICAL_START > MEMORY_START)
-		memblock_reserve(MEMORY_START, 0x8000);
+		memblock_reserve(MEMORY_START, int_vector_size);
 	reserve_kdump_trampoline();
 #if defined(CONFIG_FA_DUMP) || defined(CONFIG_PRESERVE_FA_DUMP)
 	/*
-- 
2.34.1

Re: [PATCH] powerpc: Fix preserved memory size for int-vectors

[Christophe Leroy]

Le 09/01/2024 à 04:38, GUO Zihua a écrit :
> [Vous ne recevez pas souvent de courriers de guozihua@huawei.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> The first 32k of memory is reserved for interrupt vectors, however for
> powerpc64 this might not be enough. Fix this by reserving the maximum
> size between 32k and the real size of interrupt vectors.

You say "this might not be enough". Do you have exemples of when it is 
not enough, or is it just hypothetycal ?

> 
> Signed-off-by: GUO Zihua <guozihua@huawei.com>
> ---
>   arch/powerpc/kernel/prom.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 0b5878c3125b..f374487513b3 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -758,6 +758,7 @@ static inline void save_fscr_to_task(void) {}
>   void __init early_init_devtree(void *params)
>   {
>          phys_addr_t limit;
> +       size_t int_vector_size;

Why size_t ?

memblock_reserve() takes a phys_addr_t

> 
>          DBG(" -> early_init_devtree(%px)\n", params);
> 
> @@ -810,9 +811,17 @@ void __init early_init_devtree(void *params)
>          setup_initial_memory_limit(memstart_addr, first_memblock_size);
>          /* Reserve MEMBLOCK regions used by kernel, initrd, dt, etc... */
>          memblock_reserve(PHYSICAL_START, __pa(_end) - PHYSICAL_START);
> +#ifdef CONFIG_PPC64
> +       /* If relocatable, reserve at least 32k for interrupt vectors etc. */
> +       int_vector_size = (size_t)((uintptr_t)__end_interrupts -
> +                                  (uintptr_t)_stext);

Why do you need so many casts ? When I look into function 
overlaps_interrupt_vector_text() it seems to work well without cast at all.

> +       int_vector_size = max_t(size_t, 0x8000, int_vector_size);

Use SZ_32K instead of 0x8000

> +#else
>          /* If relocatable, reserve first 32k for interrupt vectors etc. */
> +       int_vector_size = 0x8000;

Use SZ_32K

> +#endif
>          if (PHYSICAL_START > MEMORY_START)
> -               memblock_reserve(MEMORY_START, 0x8000);
> +               memblock_reserve(MEMORY_START, int_vector_size);
>          reserve_kdump_trampoline();
>   #if defined(CONFIG_FA_DUMP) || defined(CONFIG_PRESERVE_FA_DUMP)
>          /*
> --
> 2.34.1
> 

Re: [PATCH] powerpc: Fix preserved memory size for int-vectors

[Guozihua (Scott)]

On 2024/1/9 17:59, Christophe Leroy wrote:
> 
> 
> Le 09/01/2024 à 04:38, GUO Zihua a écrit :
>> [Vous ne recevez pas souvent de courriers de guozihua@huawei.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>>
>> The first 32k of memory is reserved for interrupt vectors, however for
>> powerpc64 this might not be enough. Fix this by reserving the maximum
>> size between 32k and the real size of interrupt vectors.
> 
> You say "this might not be enough". Do you have exemples of when it is 
> not enough, or is it just hypothetycal ?

Hi Christophe,

The problem is discovered when I am trying to test ppc64 kaslr
(developed by Yan, ref: https://lwn.net/Articles/811686/) on QEMU. On
QEMU it seems that the size of interrupt vectors is way larger than
0x8000, resulting in the interrupt vectors code overriding memory
allocated for PACAs during do_final_fixups().
> 
>>
>> Signed-off-by: GUO Zihua <guozihua@huawei.com>
>> ---
>>   arch/powerpc/kernel/prom.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> index 0b5878c3125b..f374487513b3 100644
>> --- a/arch/powerpc/kernel/prom.c
>> +++ b/arch/powerpc/kernel/prom.c
>> @@ -758,6 +758,7 @@ static inline void save_fscr_to_task(void) {}
>>   void __init early_init_devtree(void *params)
>>   {
>>          phys_addr_t limit;
>> +       size_t int_vector_size;
> 
> Why size_t ?
> 
> memblock_reserve() takes a phys_addr_t

Will fix that.
> 
>>
>>          DBG(" -> early_init_devtree(%px)\n", params);
>>
>> @@ -810,9 +811,17 @@ void __init early_init_devtree(void *params)
>>          setup_initial_memory_limit(memstart_addr, first_memblock_size);
>>          /* Reserve MEMBLOCK regions used by kernel, initrd, dt, etc... */
>>          memblock_reserve(PHYSICAL_START, __pa(_end) - PHYSICAL_START);
>> +#ifdef CONFIG_PPC64
>> +       /* If relocatable, reserve at least 32k for interrupt vectors etc. */
>> +       int_vector_size = (size_t)((uintptr_t)__end_interrupts -
>> +                                  (uintptr_t)_stext);
> 
> Why do you need so many casts ? When I look into function 
> overlaps_interrupt_vector_text() it seems to work well without cast at all.

I'll look into it.
> 
>> +       int_vector_size = max_t(size_t, 0x8000, int_vector_size);
> 
> Use SZ_32K instead of 0x8000
> 
>> +#else
>>          /* If relocatable, reserve first 32k for interrupt vectors etc. */
>> +       int_vector_size = 0x8000;
> 
> Use SZ_32K

Sure.
> 
>> +#endif
>>          if (PHYSICAL_START > MEMORY_START)
>> -               memblock_reserve(MEMORY_START, 0x8000);
>> +               memblock_reserve(MEMORY_START, int_vector_size);
>>          reserve_kdump_trampoline();
>>   #if defined(CONFIG_FA_DUMP) || defined(CONFIG_PRESERVE_FA_DUMP)
>>          /*
>> --
>> 2.34.1
>>

-- 
Best
GUO Zihua