[PATCH] x86/head/64: level2_kernel_pgt's kernel area is built with _PAGE_PRESENT set

[PATCH] x86/head/64: level2_kernel_pgt's kernel area is built with _PAGE_PRESENT set

[Wei Yang]

The code is first introduced in 'commit 1ab60e0f72f7 ("[PATCH] x86-64:
Relocatable Kernel Support")'.  Then 'commit c88d71508e36b
("x86/boot/64: Rewrite startup_64() in C")', convert it to c. And
'commit 2aa85f246c181 ("x86/boot/64: Make level2_kernel_pgt pages
invalid outside kernel area")' limit the range from _text to _end.

Originally, it does the check because the loop iterate the whole
level2_kernel_pgt, while currently it just fixup the kernel area. This
area is built with _PAGE_PRESENT set.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Vivek Goyal <vgoyal@in.ibm.com>
CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
CC: Ingo Molnar <mingo@kernel.org>
CC: Steve Wahl <steve.wahl@hpe.com>
CC: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/head64.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 212e8e06aeba..75c69f8620d8 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -253,8 +253,7 @@ unsigned long __head __startup_64(unsigned long physaddr,
 
 	/* fixup pages that are part of the kernel image */
 	for (; i <= pmd_index((unsigned long)_end); i++)
-		if (pmd[i] & _PAGE_PRESENT)
-			pmd[i] += load_delta;
+		pmd[i] += load_delta;
 
 	/* invalidate pages after the kernel image */
 	for (; i < PTRS_PER_PMD; i++)
-- 
2.34.1

Re: [PATCH] x86/head/64: level2_kernel_pgt's kernel area is built with _PAGE_PRESENT set

[Wei Yang]

On Sat, Mar 23, 2024 at 11:26:21PM +0000, Wei Yang wrote:
>The code is first introduced in 'commit 1ab60e0f72f7 ("[PATCH] x86-64:
>Relocatable Kernel Support")'.  Then 'commit c88d71508e36b
>("x86/boot/64: Rewrite startup_64() in C")', convert it to c. And
>'commit 2aa85f246c181 ("x86/boot/64: Make level2_kernel_pgt pages
>invalid outside kernel area")' limit the range from _text to _end.
>
>Originally, it does the check because the loop iterate the whole
>level2_kernel_pgt, while currently it just fixup the kernel area. This
>area is built with _PAGE_PRESENT set.

Ping

>
>Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>CC: Vivek Goyal <vgoyal@in.ibm.com>
>CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>CC: Ingo Molnar <mingo@kernel.org>
>CC: Steve Wahl <steve.wahl@hpe.com>
>CC: Borislav Petkov <bp@suse.de>
>---
> arch/x86/kernel/head64.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
>diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
>index 212e8e06aeba..75c69f8620d8 100644
>--- a/arch/x86/kernel/head64.c
>+++ b/arch/x86/kernel/head64.c
>@@ -253,8 +253,7 @@ unsigned long __head __startup_64(unsigned long physaddr,
> 
> 	/* fixup pages that are part of the kernel image */
> 	for (; i <= pmd_index((unsigned long)_end); i++)
>-		if (pmd[i] & _PAGE_PRESENT)
>-			pmd[i] += load_delta;
>+		pmd[i] += load_delta;
> 
> 	/* invalidate pages after the kernel image */
> 	for (; i < PTRS_PER_PMD; i++)
>-- 
>2.34.1

-- 
Wei Yang
Help you, Help me

Re: [PATCH] x86/head/64: level2_kernel_pgt's kernel area is built with _PAGE_PRESENT set

[Thomas Gleixner]

On Sat, Mar 23 2024 at 23:26, Wei Yang wrote:
> The code is first introduced in 'commit 1ab60e0f72f7 ("[PATCH] x86-64:
> Relocatable Kernel Support")'.  Then 'commit c88d71508e36b
> ("x86/boot/64: Rewrite startup_64() in C")', convert it to c. And
> 'commit 2aa85f246c181 ("x86/boot/64: Make level2_kernel_pgt pages
> invalid outside kernel area")' limit the range from _text to _end.
>
> Originally, it does the check because the loop iterate the whole
> level2_kernel_pgt, while currently it just fixup the kernel area. This
> area is built with _PAGE_PRESENT set.

What's the actual problem you are trying to solve?

>  	/* fixup pages that are part of the kernel image */
>  	for (; i <= pmd_index((unsigned long)_end); i++)
> -		if (pmd[i] & _PAGE_PRESENT)
> -			pmd[i] += load_delta;
> +		pmd[i] += load_delta;

Fixing up non-present PMDs is a pointless exercise.

Thanks,

        tglx

Re: [PATCH] x86/head/64: level2_kernel_pgt's kernel area is built with _PAGE_PRESENT set

[Wei Yang]

On Wed, May 22, 2024 at 11:58:01AM +0200, Thomas Gleixner wrote:
>On Sat, Mar 23 2024 at 23:26, Wei Yang wrote:
>> The code is first introduced in 'commit 1ab60e0f72f7 ("[PATCH] x86-64:
>> Relocatable Kernel Support")'.  Then 'commit c88d71508e36b
>> ("x86/boot/64: Rewrite startup_64() in C")', convert it to c. And
>> 'commit 2aa85f246c181 ("x86/boot/64: Make level2_kernel_pgt pages
>> invalid outside kernel area")' limit the range from _text to _end.
>>
>> Originally, it does the check because the loop iterate the whole
>> level2_kernel_pgt, while currently it just fixup the kernel area. This
>> area is built with _PAGE_PRESENT set.
>
>What's the actual problem you are trying to solve?

Not a problem. It tries to remove some duplicate check.

>
>>  	/* fixup pages that are part of the kernel image */
>>  	for (; i <= pmd_index((unsigned long)_end); i++)
>> -		if (pmd[i] & _PAGE_PRESENT)
>> -			pmd[i] += load_delta;
>> +		pmd[i] += load_delta;
>
>Fixing up non-present PMDs is a pointless exercise.
>

Agree. While we are sure then range here must present.

The whole process looks like this

    pmd in [0, _text)
        unset _PAGE_PRESENT
    pmd in [_text, _end]
        fix up delta
    pmd in (_end, 256)
        unset _PAGE_PRESENT

Since we have compiled in _PAGE_PRESENT in this page table, it is not
necessary to check _PAGE_PRESENT again before fixing up delta.

BTW, if one entry between _text and _end is not present, we will failed to
fixing the kernel code pmd entry, which will lead to some problem.

>Thanks,
>
>        tglx

-- 
Wei Yang
Help you, Help me

Re: [PATCH] x86/head/64: level2_kernel_pgt's kernel area is built with _PAGE_PRESENT set

[Thomas Gleixner]

On Wed, May 22 2024 at 14:06, Wei Yang wrote:
> On Wed, May 22, 2024 at 11:58:01AM +0200, Thomas Gleixner wrote:
>>
>>What's the actual problem you are trying to solve?
>
> Not a problem. It tries to remove some duplicate check.

I assume you mean redundant check, right?

The changelog should explain that. I really could not figure out
what this is about.

>>>  	/* fixup pages that are part of the kernel image */
>>>  	for (; i <= pmd_index((unsigned long)_end); i++)
>>> -		if (pmd[i] & _PAGE_PRESENT)
>>> -			pmd[i] += load_delta;
>>> +		pmd[i] += load_delta;
>>
>>Fixing up non-present PMDs is a pointless exercise.
>>
>
> Agree. While we are sure then range here must present.
>
> The whole process looks like this
>
>     pmd in [0, _text)
>         unset _PAGE_PRESENT
>     pmd in [_text, _end]
>         fix up delta
>     pmd in (_end, 256)
>         unset _PAGE_PRESENT
>
> Since we have compiled in _PAGE_PRESENT in this page table, it is not
> necessary to check _PAGE_PRESENT again before fixing up delta.

That wants to be in the change log.

Referencing the history of the code is definitely interesting and you
did a great job on decoding it, but for the change itself the only
relevant information is that all PMDs between _text and _end are marked
present because the whole table is marked so.

> BTW, if one entry between _text and _end is not present, we will failed to
> fixing the kernel code pmd entry, which will lead to some problem.

It does not because a non-present entry does not care about the load
delta obviously.

Thanks,

        tglx

Re: [PATCH] x86/head/64: level2_kernel_pgt's kernel area is built with _PAGE_PRESENT set

[Wei Yang]

On Wed, May 22, 2024 at 10:33:20PM +0200, Thomas Gleixner wrote:
>On Wed, May 22 2024 at 14:06, Wei Yang wrote:
>> On Wed, May 22, 2024 at 11:58:01AM +0200, Thomas Gleixner wrote:
>>>
>>>What's the actual problem you are trying to solve?
>>
>> Not a problem. It tries to remove some duplicate check.
>
>I assume you mean redundant check, right?

Yep, you are right.

>
>The changelog should explain that. I really could not figure out
>what this is about.
>
>>>>  	/* fixup pages that are part of the kernel image */
>>>>  	for (; i <= pmd_index((unsigned long)_end); i++)
>>>> -		if (pmd[i] & _PAGE_PRESENT)
>>>> -			pmd[i] += load_delta;
>>>> +		pmd[i] += load_delta;
>>>
>>>Fixing up non-present PMDs is a pointless exercise.
>>>
>>
>> Agree. While we are sure then range here must present.
>>
>> The whole process looks like this
>>
>>     pmd in [0, _text)
>>         unset _PAGE_PRESENT
>>     pmd in [_text, _end]
>>         fix up delta
>>     pmd in (_end, 256)
>>         unset _PAGE_PRESENT
>>
>> Since we have compiled in _PAGE_PRESENT in this page table, it is not
>> necessary to check _PAGE_PRESENT again before fixing up delta.
>
>That wants to be in the change log.
>

Sure, I would put this in the change log in next version.

>Referencing the history of the code is definitely interesting and you
>did a great job on decoding it, but for the change itself the only
>relevant information is that all PMDs between _text and _end are marked
>present because the whole table is marked so.
>
>> BTW, if one entry between _text and _end is not present, we will failed to
>> fixing the kernel code pmd entry, which will lead to some problem.
>
>It does not because a non-present entry does not care about the load
>delta obviously.
>
>Thanks,
>
>        tglx

-- 
Wei Yang
Help you, Help me