[PATCH] arm64/kernel: Fix range on invalidating dcache for boot page tables

[PATCH] arm64/kernel: Fix range on invalidating dcache for boot page tables

[Gavin Shan]

The MMU is disabled when __create_page_tables() is called. The data
cache corresponding to these two page tables, which are tracked by
@idmap_pg_dir and @init_pg_dir, is invalidated after the page tables
are populated. However, the wrong or inappropriate size have been used
and more data cache are invalidated than it need.

This fixes the issue by invalidating the data cache for these two
page tables separately as they aren't necessarily physically adjacent.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/kernel/head.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 57a91032b4c2..66947873c9e7 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -398,6 +398,10 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
 	 * tables again to remove any speculatively loaded cache lines.
 	 */
 	adrp	x0, idmap_pg_dir
+	mov	x1, #IDMAP_DIR_SIZE
+	dmb	sy
+	bl	__inval_dcache_area
+	adrp	x0, init_pg_dir
 	adrp	x1, init_pg_end
 	sub	x1, x1, x0
 	dmb	sy
-- 
2.23.0

Re: [PATCH] arm64/kernel: Fix range on invalidating dcache for boot page tables

[Mark Rutland]

Hi Gavin,

On Fri, Apr 24, 2020 at 03:02:30PM +1000, Gavin Shan wrote:
> The MMU is disabled when __create_page_tables() is called. The data
> cache corresponding to these two page tables, which are tracked by
> @idmap_pg_dir and @init_pg_dir, is invalidated after the page tables
> are populated. However, the wrong or inappropriate size have been used
> and more data cache are invalidated than it need.
> 
> This fixes the issue by invalidating the data cache for these two
> page tables separately as they aren't necessarily physically adjacent.

Thanks for this!

I think the commit message needs to explain the issue more explicitly,
e.g.

| Prior to commit:
| 
|   8eb7e28d4c642c31i ("arm64/mm: move runtime pgds to rodata")
|
| ... idmap_pgd_dir, tramp_pg_dir, reserved_ttbr0, swapper_pg_dir, and
| init_pg_dir were contiguous at the end of the kernel image. The
| maintenance at the end of __create_page_tables assumed these were
| contiguous, and affected everything from the start of idmap_pg_dir to
| the end of init_pg_dir.
|
| That commit moved all but init_pg_dir into the .rodata section, with
| other data placed between idmap_pg_dir and init_pg_dir, but did not
| update the maintenance. Hence the maintenance is performed on much
| more data than necessary (but as the bootloader previously made this
| clean to the PoC there is no functional problem).
|
| As we only alter idmap_pg_dir, and init_pg_dir, we only need to
| perform maintenance for these. As the other dirs are in .rodata, the
| bootloader will have initialised them as expected and cleaned them to
| the PoC. The kernel will initialize them as necessary after enabling
| the MMU.
|
| This patch reworks the maintenance to only cover the idmap_pg_dir and
| init_pg_dir to avoid this unnecessary work.

> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  arch/arm64/kernel/head.S | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 57a91032b4c2..66947873c9e7 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -398,6 +398,10 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
>  	 * tables again to remove any speculatively loaded cache lines.
>  	 */

The comment above has been stale for a while, since it says:

| 	/*
| 	 * Since the page tables have been populated with non-cacheable
| 	 * accesses (MMU disabled), invalidate the idmap and swapper page
| 	 * tables again to remove any speculatively loaded cache lines.
| 	 */

... can we please update that at the same time? We can avoid mention of
the specific tables and say:

| 	/*
| 	 * Since the page tables have been populated with non-cacheable
| 	 * accesses (MMU disabled), invalidate those tables again to
| 	 * remove any speculatively loaded cache lines.
| 	 */

>  	adrp	x0, idmap_pg_dir
> +	mov	x1, #IDMAP_DIR_SIZE
> +	dmb	sy
> +	bl	__inval_dcache_area
> +	adrp	x0, init_pg_dir
>  	adrp	x1, init_pg_end
>  	sub	x1, x1, x0
>  	dmb	sy

The existing DMB is to order prior non-cacheable accesses against cache
maintenance, so we only need one of those at the start of the sequence.
For consistency, we should use the same idiom to generate the size of
both dirs. Given we use ADRP+ADRP+SUB here and elsewhere in head.S, I
think that's preferable for now.

So I reckon this should be:

|	dmb	sy
|
|	adrp	x0, idmap_pg_dir
|	adrp	x1, idmap_pg_end
|	sub	x1, x1, x0
|	bl	__inval_dcache_area
|
|	adrp	x0, init_pg_dir
|	adrp	x1, init_pg_end
|	sub	x1, x1, x0
|	bl	__inval_dcache_area

... with those line gaps to make the distinct blocks clearer.

Thanks,
Mark.

Re: [PATCH] arm64/kernel: Fix range on invalidating dcache for boot page tables

[Gavin Shan]

Hi Mark,

On 4/24/20 8:01 PM, Mark Rutland wrote:
> Hi Gavin,
> 
> On Fri, Apr 24, 2020 at 03:02:30PM +1000, Gavin Shan wrote:
>> The MMU is disabled when __create_page_tables() is called. The data
>> cache corresponding to these two page tables, which are tracked by
>> @idmap_pg_dir and @init_pg_dir, is invalidated after the page tables
>> are populated. However, the wrong or inappropriate size have been used
>> and more data cache are invalidated than it need.
>>
>> This fixes the issue by invalidating the data cache for these two
>> page tables separately as they aren't necessarily physically adjacent.
> 
> Thanks for this!
> 
> I think the commit message needs to explain the issue more explicitly,
> e.g.
> 
> | Prior to commit:
> |
> |   8eb7e28d4c642c31i ("arm64/mm: move runtime pgds to rodata")
> |
> | ... idmap_pgd_dir, tramp_pg_dir, reserved_ttbr0, swapper_pg_dir, and
> | init_pg_dir were contiguous at the end of the kernel image. The
> | maintenance at the end of __create_page_tables assumed these were
> | contiguous, and affected everything from the start of idmap_pg_dir to
> | the end of init_pg_dir.
> |
> | That commit moved all but init_pg_dir into the .rodata section, with
> | other data placed between idmap_pg_dir and init_pg_dir, but did not
> | update the maintenance. Hence the maintenance is performed on much
> | more data than necessary (but as the bootloader previously made this
> | clean to the PoC there is no functional problem).
> |
> | As we only alter idmap_pg_dir, and init_pg_dir, we only need to
> | perform maintenance for these. As the other dirs are in .rodata, the
> | bootloader will have initialised them as expected and cleaned them to
> | the PoC. The kernel will initialize them as necessary after enabling
> | the MMU.
> |
> | This patch reworks the maintenance to only cover the idmap_pg_dir and
> | init_pg_dir to avoid this unnecessary work.
> 

Thanks for detailed changelog. I will use yours in v2, which will be posted
shortly. A nit is the correct commit ID would be 8eb7e28d4c642c31 instead
of 8eb7e28d4c642c31i :)

>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   arch/arm64/kernel/head.S | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index 57a91032b4c2..66947873c9e7 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -398,6 +398,10 @@ SYM_FUNC_START_LOCAL(__create_page_tables)
>>   	 * tables again to remove any speculatively loaded cache lines.
>>   	 */
> 
> The comment above has been stale for a while, since it says:
> 
> | 	/*
> | 	 * Since the page tables have been populated with non-cacheable
> | 	 * accesses (MMU disabled), invalidate the idmap and swapper page
> | 	 * tables again to remove any speculatively loaded cache lines.
> | 	 */
> 
> ... can we please update that at the same time? We can avoid mention of
> the specific tables and say:
> 
> | 	/*
> | 	 * Since the page tables have been populated with non-cacheable
> | 	 * accesses (MMU disabled), invalidate those tables again to
> | 	 * remove any speculatively loaded cache lines.
> | 	 */
> 

Sure, It will be included in v2.

>>   	adrp	x0, idmap_pg_dir
>> +	mov	x1, #IDMAP_DIR_SIZE
>> +	dmb	sy
>> +	bl	__inval_dcache_area
>> +	adrp	x0, init_pg_dir
>>   	adrp	x1, init_pg_end
>>   	sub	x1, x1, x0
>>   	dmb	sy
> 
> The existing DMB is to order prior non-cacheable accesses against cache
> maintenance, so we only need one of those at the start of the sequence.
> For consistency, we should use the same idiom to generate the size of
> both dirs. Given we use ADRP+ADRP+SUB here and elsewhere in head.S, I
> think that's preferable for now.
> 
> So I reckon this should be:
> 
> |	dmb	sy
> |
> |	adrp	x0, idmap_pg_dir
> |	adrp	x1, idmap_pg_end
> |	sub	x1, x1, x0
> |	bl	__inval_dcache_area
> |
> |	adrp	x0, init_pg_dir
> |	adrp	x1, init_pg_end
> |	sub	x1, x1, x0
> |	bl	__inval_dcache_area
> 
> ... with those line gaps to make the distinct blocks clearer.
> 

Yep, I'll change the code accordingly in v2. Also, symbol @idmap_pg_end
will be added to vmlinux.lds.S as it's not existing.

> Thanks,
> Mark.
> 

Thanks,
Gavin