xen/arm: Properly clean update to init_ttbr and smp_up_cpu

[PATCH] xen/arm: Properly clean update to init_ttbr and smp_up_cpu

Posted by Julien Grall 2 years ago

From: Julien Grall <jgrall@amazon.com>

Recent rework to the secondary boot code modified how init_ttbr and
smp_up_cpu are accessed. Rather than directly accessing them, we
are using a pointer to them.

The helper clean_dcache() is expected to take the variable in parameter
and then clean its content. As we now pass a pointer to the variable,
we will clean the area storing the address rather than the content itself.

Switch to use clean_dcache_va_range() to avoid casting the pointer.

Fixes: a5ed59e62c6f ("arm/mmu: Move init_ttbr to a new section .data.idmap")
Fixes: 9a5114074b04 ("arm/smpboot: Move smp_up_cpu to a new section .data.idmap)

Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/mmu/smpboot.c | 2 +-
 xen/arch/arm/smpboot.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
index bc91fdfe3331..4ffc8254a44b 100644
--- a/xen/arch/arm/mmu/smpboot.c
+++ b/xen/arch/arm/mmu/smpboot.c
@@ -88,7 +88,7 @@ static void set_init_ttbr(lpae_t *root)
      * init_ttbr will be accessed with the MMU off, so ensure the update
      * is visible by cleaning the cache.
      */
-    clean_dcache(ptr);
+    clean_dcache_va_range(ptr, sizeof(uint64_t));
 
     unmap_domain_page(ptr);
 }
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 119bfa3160ad..a84e706d77da 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -449,7 +449,7 @@ static void set_smp_up_cpu(unsigned long mpidr)
      * smp_up_cpu will be accessed with the MMU off, so ensure the update
      * is visible by cleaning the cache.
      */
-    clean_dcache(ptr);
+    clean_dcache_va_range(ptr, sizeof(unsigned long));
 
     unmap_domain_page(ptr);
 
-- 
2.40.1

Re: [PATCH] xen/arm: Properly clean update to init_ttbr and smp_up_cpu

Posted by Bertrand Marquis 2 years ago

Hi Julien,

Nice finding :-)

> On 30 Jan 2024, at 18:29, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Recent rework to the secondary boot code modified how init_ttbr and
> smp_up_cpu are accessed. Rather than directly accessing them, we
> are using a pointer to them.
> 
> The helper clean_dcache() is expected to take the variable in parameter
> and then clean its content. As we now pass a pointer to the variable,
> we will clean the area storing the address rather than the content itself.
> 
> Switch to use clean_dcache_va_range() to avoid casting the pointer.
> 
> Fixes: a5ed59e62c6f ("arm/mmu: Move init_ttbr to a new section .data.idmap")
> Fixes: 9a5114074b04 ("arm/smpboot: Move smp_up_cpu to a new section .data.idmap)
> 
> Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/arch/arm/mmu/smpboot.c | 2 +-
> xen/arch/arm/smpboot.c     | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
> index bc91fdfe3331..4ffc8254a44b 100644
> --- a/xen/arch/arm/mmu/smpboot.c
> +++ b/xen/arch/arm/mmu/smpboot.c
> @@ -88,7 +88,7 @@ static void set_init_ttbr(lpae_t *root)
>      * init_ttbr will be accessed with the MMU off, so ensure the update
>      * is visible by cleaning the cache.
>      */
> -    clean_dcache(ptr);
> +    clean_dcache_va_range(ptr, sizeof(uint64_t));
> 
>     unmap_domain_page(ptr);
> }
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 119bfa3160ad..a84e706d77da 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -449,7 +449,7 @@ static void set_smp_up_cpu(unsigned long mpidr)
>      * smp_up_cpu will be accessed with the MMU off, so ensure the update
>      * is visible by cleaning the cache.
>      */
> -    clean_dcache(ptr);
> +    clean_dcache_va_range(ptr, sizeof(unsigned long));
> 
>     unmap_domain_page(ptr);
> 
> -- 
> 2.40.1
>

Re: [PATCH] xen/arm: Properly clean update to init_ttbr and smp_up_cpu

Posted by Julien Grall 2 years ago

Hi,

On 31/01/2024 07:57, Bertrand Marquis wrote:
>> On 30 Jan 2024, at 18:29, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Recent rework to the secondary boot code modified how init_ttbr and
>> smp_up_cpu are accessed. Rather than directly accessing them, we
>> are using a pointer to them.
>>
>> The helper clean_dcache() is expected to take the variable in parameter
>> and then clean its content. As we now pass a pointer to the variable,
>> we will clean the area storing the address rather than the content itself.
>>
>> Switch to use clean_dcache_va_range() to avoid casting the pointer.
>>
>> Fixes: a5ed59e62c6f ("arm/mmu: Move init_ttbr to a new section .data.idmap")
>> Fixes: 9a5114074b04 ("arm/smpboot: Move smp_up_cpu to a new section .data.idmap)
>>
>> Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Committed. Thanks.

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: Properly clean update to init_ttbr and smp_up_cpu

Posted by Oleksandr Tyshchenko 2 years ago


On 30.01.24 19:29, Julien Grall wrote:

Hello Julien


> From: Julien Grall <jgrall@amazon.com>
> 
> Recent rework to the secondary boot code modified how init_ttbr and
> smp_up_cpu are accessed. Rather than directly accessing them, we
> are using a pointer to them.
> 
> The helper clean_dcache() is expected to take the variable in parameter
> and then clean its content. As we now pass a pointer to the variable,
> we will clean the area storing the address rather than the content itself.
> 
> Switch to use clean_dcache_va_range() to avoid casting the pointer.
> 
> Fixes: a5ed59e62c6f ("arm/mmu: Move init_ttbr to a new section .data.idmap")
> Fixes: 9a5114074b04 ("arm/smpboot: Move smp_up_cpu to a new section .data.idmap)
> 
> Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>


[on Renesas R-Car Gen3 SoC with 8 cores (Arm64)]
Tested-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>


> ---
>   xen/arch/arm/mmu/smpboot.c | 2 +-
>   xen/arch/arm/smpboot.c     | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
> index bc91fdfe3331..4ffc8254a44b 100644
> --- a/xen/arch/arm/mmu/smpboot.c
> +++ b/xen/arch/arm/mmu/smpboot.c
> @@ -88,7 +88,7 @@ static void set_init_ttbr(lpae_t *root)
>        * init_ttbr will be accessed with the MMU off, so ensure the update
>        * is visible by cleaning the cache.
>        */
> -    clean_dcache(ptr);
> +    clean_dcache_va_range(ptr, sizeof(uint64_t));
>   
>       unmap_domain_page(ptr);
>   }
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 119bfa3160ad..a84e706d77da 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -449,7 +449,7 @@ static void set_smp_up_cpu(unsigned long mpidr)
>        * smp_up_cpu will be accessed with the MMU off, so ensure the update
>        * is visible by cleaning the cache.
>        */
> -    clean_dcache(ptr);
> +    clean_dcache_va_range(ptr, sizeof(unsigned long));
>   
>       unmap_domain_page(ptr);
>