lkml.org 
[lkml]   [2020]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] arm64/kernel: Fix range on invalidating dcache for boot page tables
From
Date
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

\
 
 \ /
  Last update: 2020-04-28 01:54    [W:0.069 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site