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 <[email protected]>
CC: Vivek Goyal <[email protected]>
CC: Kirill A. Shutemov <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Steve Wahl <[email protected]>
CC: Borislav Petkov <[email protected]>
---
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
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 <[email protected]>
>CC: Vivek Goyal <[email protected]>
>CC: Kirill A. Shutemov <[email protected]>
>CC: Ingo Molnar <[email protected]>
>CC: Steve Wahl <[email protected]>
>CC: Borislav Petkov <[email protected]>
>---
> 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
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
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
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
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