Improvements for MM. #2501

vgalnt · 2020-04-05T14:03:39Z

Purpose:

Removing duplicates:
extern PVOID MmPagedPoolStart;
extern PVOID MmPagedPoolEnd;
extern PVOID MmNonPagedSystemStart;
extern PVOID MmSessionBase;
extern SIZE_T MmSizeOfPagedPoolInBytes;
extern PVOID MiSystemViewStart;
For X86:
using PD_COUNT instead PPE_PER_PAGE;
using MmSystemPageDirectory and MmSystemPagePtes only if _MI_PAGING_LEVELS <= 3
Using MiAddressToPdeOffset() instead of MiGetPdeOffset() for redefine MiGetPdeOffset().
Making translation macros for x86 versatility for systems with PAE;
make also the definitions and macros more human-readable;
add MiGetPteOffset(), MiGetPdeOffset() and MiGetPdIndex() macros;
using naiming "Pte" instead "PointerPte".

SergeGautherie · 2020-04-05T21:57:31Z

[NTOS:MM] For X86 using PD_COUNT instead PPE_PER_PAGE. 91819e9

For the record, this commit reverts parts of 2c6747a. (That's life...)

vgalnt · 2020-04-06T08:01:53Z

MmSystemPageDirectory used only for _MI_PAGING_LEVELS == 2 (for PAE too should) -

reactos/ntoskrnl/mm/ARM3/mminit.c

Line 157 in acf135d

#if (_MI_PAGING_LEVELS == 2)

PPE_PER_PAGE is the number of page directory pointers contained in one page of memory (4 kB). Used when translating addresses for x64 (for next 3-level).

PD_COUNT is the maximum number of page directory pages in the system. Used for MmSystemPageDirectory[PD_COUNT]. It also only for x32.

Using different constants for different purposes and for different architectures will avoid confusion and errors during development and debugging.

Also compare two lines of code:
1 - current code
2 - a possible after this PR.

ParentPage = MmSystemPageDirectory[(PointerPde - MiAddressToPde(NULL)) / PDE_PER_PAGE];
ParentPage = MmSystemPageDirectory[MiGetPdIndex(Pde)];

SergeGautherie · 2020-04-06T12:55:25Z

Using different constants

Yeah, I meant only that it was too bad to update/revert these lines, nothing more/wrong.

Also, I was suggesting to (double) check whether the following might want/need to be updated/sync'ed too:
https://git.reactos.org/?p=reactos.git&a=search&h=HEAD&st=grep&s=PPE_PER_PAGE

ntoskrnl/include/internal/arm/mm.h
  22 #define PPE_PER_PAGE 1
ntoskrnl/mm/ARM3/arm/init.c
  40 PFN_NUMBER MmSystemPageDirectory[PPE_PER_PAGE];

vgalnt · 2020-04-06T14:33:23Z

Probably for ARM architecture this is also unnecessary. But I'm not 100% sure.

tkreuzer · 2020-04-09T20:57:51Z

ntoskrnl/include/internal/i386/mm.h

-                                                  PAGE_SIZE)
+
+/* Hyper space definitions */
+#define HYPER_SPACE            (PTE_TOP + 1)


Does this add any readability or any other benefit? I can see 3 cases:

fully hardcoded addresses, which is easy to read, but obsolete and not portable.

A mixture of hardcoded addresses and relative offsets, which is harder to read, still obsolete and barely portable.

A complete dynamic system, which would be modern, but we don't support it and it would make all of this obsolete.

As of now, I don't see any benefit of moving from 1) to 2). x64 already uses a completely different layout, and pretending that certain relationships between things do exist, while they are arbitrary, doesn't make anything better. Until we move to complete KASLR I think we should not pretend to follow "rules", which do not generally exist, but instead just be honest and hardcode everything. It might add a few extra lines for PAE, but that is really only ONE single exception among all the variations. My suggestion is to actually hardcode te addresses for PAE and non-PAE. This is just my personal feeling though, and in the end I do not have an extreme position on it (I don't actually care). So if other people prefer this, I'm fine with it.

If I wrote new code
#define MiPteToAddress (Pte) ((PVOID) ((ULONG) ( Pte) << 10)),
then they would probably tell me that this is "magic" ...
Now when I answer the question "Why?" (#define MiPteToAddress (_Pte) ((PVOID) ((MiGetPteOffset (_Pte)) * PAGE_SIZE))), you tell me this is an exceptional case. Then the question arises, "Why is this case so exceptional?"

I'd rather want to know: What does it fix? Nothing. It's just noise with the potential to conflict with actual fixes, other developers might work on.

As far as I understand, not all changes should fix something. Some changes are made to help other people better understand how it works. In this case, HYPER_SPACE 0xC0400000 won't tell you much, but HYPER_SPACE (PTE_TOP + 1) hints that HYPER_SPACE starts after the PTE area. And I don't understand how this change can create conflict. Even hypothetically. As for the other changes, yes, there are a number of changes that I believe improve the readability of the code, but do not fix anything. If you have specific comments, I suggest discussing them specifically. In fact, this PR is one of the preparatory steps. In general, if this PR creates problems for you, then you can simply close it.

Yes, and I totally understand the desire to improve things. But we have made the experience, that "improvements" come at a high cost:

We have to review things that do not fix the functionality of the OS, but can easily introduce subtle bugs. This might be useful in some cases, but when it comes to low level kernel functionality, it needs to be seriously justified. We have generally better stuff to do than to review "Make x more consistent" PRs, and we already have plenty of them. Ask Serge, he's a master in creating them.

All changes that change the code base on a broader scale have the potential to break other developer's code. And we all have partly ready things, that we are working on, and that can fix actual problems, but would suddenly conflict with changes, that were done to "improve" things for them.

Changing the name of something might make it better to understand what it does, but changing the purpose of an already existing function will make things only worse.

And I don't understand how this change can create conflict. Even hypothetically.

Well then make sure the amd64_bringup branch will rebase without conflicts on top of this and everything still works. Good luck! Once you did this, and told me with a straight face that there was no issue, we will talk again.

And don't get me wrong. I don't say your changes are bad or broken. I just want to make sure that we get the changes right and we don't introduce needless changes that don't improve anything. For me this is majorly because I know I will have to deal with it in the x64 branch, i.e. additional work for me witout anything being fixed. I'm willing to accept it, because I don't like preventing improvements and I can deal with it. It's just something you have to bargain about with the core (in this case Mm) deveopers. And that is currentyl mostly me, I guess. Deal with it ;-)

I think that apart from
MiProtoPteToPte
MI_MAKE_PROTOTYPE_PTE
MiSubsectionPteToSubsection
MI_MAKE_SUBSECTION_PTE
MI_IS_MAPPED_PTE,
shouldn't have conflicts as 32-bit.
I decided to add them at the last moment.
It makes sense to either divide everything according to architecture, or to make everything universal. I chose the second option.
Unfortunately I have no way to check for 64-bit. My computer is too old)

…ith PAE. - Make also the definitions and macros more human-readable. - Add MiGetPteOffset, MiGetPdeOffset and MiGetPdIndex macros.

…e page directories (PD) and page tables (PT). For more see CORE-16702 https://jira.reactos.org/browse/CORE-16702?focusedCommentId=126164&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-126164

…G_LEVELS == 2).

…versal (also for PAE). Add check bottom 2 bits for x86 nonPAE. Edit comments.

…TE macros universal (also for PAE). Add check bottom 3 bits for x86 nonPAE. Edit comments.

tkreuzer

I don't really like some of the names.

I would like to have all names to be Mi<x>To<Y> to make things consistent and explicit.
Consider a better naming for Offset vs Index of/into. My suggestions to avoid any confusion are:
** Use P*eNumber for the number of a P(t/d/p/x)e entry from the start of the entire range. This avoids confusion between a byte-offset and an "index", as well as confusion between an offset from the start and an offset within the parent structure.
** Use P*Index for an index into a page-table/directory/etc.
I would like to keep changes small for review, so I would suggest not to change all of the stuff in this PR, but only for newly created stuff and make a followup PR for name normalization without functional changes.
Since this interferes with the x64 port, I'd like to make sure I can safely rebase the x64 branch on top of this without breaking everything. If you can, I would appreciate you would test that before. If you can't, I can do it before merging. Best way to get me do this is to ping me on Mattermost.

tkreuzer · 2020-10-24T19:20:22Z

ntoskrnl/include/internal/i386/mm.h

+#define MiGetPteOffset(_Pte) ((((ULONG_PTR)(_Pte)) & PTE_MASK) / sizeof(MMPTE))
+
+/* PDE offset (within all PDs pages) from the pointer to a PDE */
+#define MiGetPdeOffset(_Pde) ((((ULONG_PTR)(_Pde)) & PDE_MASK) / sizeof(MMPDE))


This is a bit confusing. First of all an Offset is usually "from" somewhere, as opposed to an Index, which is from the start. So PdeOffset can be confused as an offset from the start of the PD. For consistency with MiPteToAddress, etc., to make the meaning more obvious and to avoid changing the meaning of something that had a different meaning before, I suggest something like MiPdeToPdeNumber/MiPdeToPtNumber

These changes will be made in the next PR to normalize the names without functional changes:
MiAddressToPteOffset -> MiAddressToPteIndex (for one PT)
MiAddressToPdeOffset -> MiAddressToPdeNumber
MiGetPteOffset -> MiPteToPteNumber
MiGetPdeOffset ->MiPdeToPdeNumber
MiGetPdIndex -> MiPdeToPdIndex

tkreuzer · 2020-10-24T19:27:28Z

ntoskrnl/include/internal/i386/mm.h

+/* Index of PD in which the PDE is located */
+#ifndef _X86PAE_
+/* Only 1 page of memory */
+#define MiGetPdIndex(_Pde) (0)


PdIndex sounds like an index into a page directory, but this is not what it is. Suggestion: MiPdeToPpeNumber/MiPdeToPdNumber.

MiGetPdIndex -> MiPdeToPdIndex (above)
I suggest not using PPE for x86. Absolutely. To avoid confusion.

tkreuzer · 2020-10-24T19:53:37Z

ntoskrnl/mm/ARM3/miarm.h

 FORCEINLINE
 VOID
-MI_MAKE_PROTOTYPE_PTE(IN PMMPTE NewPte,
-                      IN PMMPTE PointerPte)
+MI_MAKE_PROTOTYPE_PTE(OUT PMMPTE ProtoPte,


Please use _In_ and _Out_, wherever you change the parameters or create new functions.

"What does it fix? Nothing. It's just noise" :)
Ok. Done.

tkreuzer · 2020-10-24T19:57:55Z

ntoskrnl/mm/ARM3/miarm.h

+    /* Do MI_MAKE_SUBSECTION_PTE() in the opposite direction. */
+
+#if defined(_M_AMD64) || defined(_X86PAE_)
+    return (PVOID)(SubsectionPte->u.Subsect.SubsectionAddress);


Please add a comment here, that SubsectionAddress is signed on x64 and thus will be properly sign extended as required.

Ok. What do you think of the PAE option? It won't be different?

tkreuzer · 2020-10-24T20:10:15Z

ntoskrnl/mm/ARM3/mminit.c

-        MI_MAKE_SUBSECTION_PTE(&TempPte, PointerPte);
-        TestPte = MiSubsectionPteToSubsection(&TempPte);
-        ASSERT(PointerPte == TestPte);
+        Subsection = (PSUBSECTION)((ULONG_PTR)MI_NONPAGED_POOL_END - _1MB);


Why this change?

It could be like this:
PSUBSECTION MiSubsectionPteToSubsection(In PMMPTE SubsectionPte);
But since besides SUBSECTION there is also MSUBSECTION, then:
PVOID MiSubsectionPteToSubsection(In PMMPTE SubsectionPte);
And SUBSECTION (or MSUBSECTION) is not MMPTE at all ...

vgalnt · 2020-10-25T12:09:39Z

Indeed, the name of this PR is unfortunate, at that moment in time nothing else occurred to me. But in fact, this is a preparatory commit (like the previous one) that makes a revision, puts things in order. My main current goal is to use the native NT sections in the memory manager instead of the "experimental" RoS sections. Also with the system cache manager. This will make them compatible with native file systems (CDFS, FASTFAT, NTFS).
Why did I choose this particular goal? I could continue with the USB stack, there is still a lot that is not implemented. But when I realized that the kernel had significant problems, I decided to suspend from USB. I don't want to waste time doing double work. It seems logical to first solve the problems in the kernel and then develop the drivers.
In general, if we consider the development of ReactOS in general, then the following order looks logical:

HAL
MM + CC + FS
PNP
PO
PAE
CONFIG_SMP
AMD64
This is my purely personal opinion.
And therefore, intersections and conflicts will be inevitable. This could have been avoided if the development was carried out together and in accordance with the plan (roadmap). But unfortunately no one wants to discuss this.

Anyway, my next PRs suggest significant changes in MM and CC, incompatible with the current ones. Therefore, I propose to make additional (parallel) directories (by analogy with USB) "mm_new" and "cache_new", in which to conduct development.

tkreuzer · 2020-10-25T18:02:32Z

5. PAE

6. CONFIG_SMP

7. AMD64

x86 is obsolete. It doesn't matter whether you support PAE or SMP, it's still obsolete. I wouldn't waste a single second on PAE or x86-SMP. I consider it a waste of time. Nobody is interested in that stuff. Nobody has x86-only hardware anymore. And if they have, they should give it to a museum. I care about the future ReactOS, not the museum ReactOS. The main reason developers care about x86, is that x64 isn't working as good yet and they don't have the skills to fix it. So that leaves me as the only person to do it to move ReactOS from an obsolete OS to a halfway modern OS.

vgalnt · 2020-10-25T18:15:35Z

It sounds something like: I will learn to run without being able to walk :)
I have nothing against x64, the problem is that even on x86 it doesn't work as it should. To learn to run, you first need to be good at walking. Evolution from simple to more complex ...

tkreuzer · 2020-10-25T18:25:23Z

It sounds something like: I will learn to run without being able to walk :)

If you need to learn to walk, that is fine. Just don't do it on the road.

binarymaster · 2020-10-25T18:37:00Z

I don't think there is anything wrong with supporting older hardware with PAE and such stuff, at least while somebody really needs it, e.g. @JoachimHenze 😉

The important part here is to support these features without dirty hacks and make them co-exist with the code for modern platforms without additional maintenance cost.

Extravert-ir · 2020-10-26T00:40:17Z

ntoskrnl/include/internal/i386/mm.h

@@ -3,43 +3,38 @@
 */
 #pragma once

+#define _MI_PAGING_LEVELS 2


I don't understand why PAE should not use _MI_PAGING_LEVELS == 3. Can you elaborate?
Setting it to 2 for all i386 means we will need to write code specifically for PAE, which we absolutely want to avoid if possible

https://jira.reactos.org/browse/CORE-16702?focusedCommentId=126164&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-126164

"Paging (or linear-address translation) is the process of translating linear addresses so that they can be used to access memory or I/O devices. Paging translates each linear address to a physical addressand determines, for each translation, what accesses to the linear address are allowed (the address’s access rights) and the type of caching used for such
accesses (the address’s memory type)." - Intel® 64 and IA-32 Architectures Software Developer’s Manual Volume 3: System Programming Guide. Chapter 4 PAGING.

According to the specification, there are three paging modes (can also say that there is a fourth mode - this is when paging is not used):

32-bit paging

PAE paging

IA-32e paging - available only on processors that support the Intel 64 architecture.

PAE requires (in addition to 32-bit paging mode) a 32-byte table with four entries (pointed to by CR3). Logical processor maintains a set of four (4) internal, non-architectural registers, called PDPTE0, PDPTE1, PDPTE2, and PDPTE3. The logical processor loads these registers from the 4 elements of this table in memory.
Therefore PAE paging mode does not require a third level of tables (for MM context). This is only required for IA-32e paging (64-bit).

My opinion is to first make (MM + CC) functionality for 32-bit mode, but it's too early to talk about PAE.

My opinion is to first make (MM + CC) functionality for 32-bit mode, but it's too early to talk about PAE.

You're proposing this change here, that's why I'm asking. We can move the discussion into the next (possible) pull request with full PAE implementation, but in that case please revert the change :)

In the ticket you wrote:

In the context of a memory manager (MM), PAE also uses two levels of tables. The difference is that there is additionally a small (4 elements) table for loading CR3. But for MM it is transparent and not used.

Do you want to say that MM is going to load a constant values into PD table and won't change them while the system is running? Is it how Windows works?

I, personally, don't see much difference with Long mode (IA-32e) paging here - we just have a 4-entry PD table instead of a 512-entry table. Where am I making a mistake?

No problems :)

The processor has four internal registers. They contain the physical addresses of the PDs from the 4-entry table. It is not MM who does this (most likely KE's job). Logically, this should happen with every process change. But this isn't MM. This is the difference. In IA-32e, no internal registers are used and tables are maintained by MM.

Generally PAE mode is an add-on over 32-bit paging mode. The difference is that in PAE mode, you can use a larger number of pages of physical memory (that's what everything was intended for). But both modes use 32 - bit virtual address space. This can be called a "clever move" by Intel engineers :). If you can outsmart them :), then you can make universal code. But I would not spend a lot of time on this (at least at this stage). I probably agree with Timo that x64 is now more important than PAE (but at the same time, up to x64 (in priority) are HAL, MM, CC, PNP, PO).

If you are serious about PAE, then you need to start with the OS bootloader (FreeLoader).

But I would not spend a lot of time on this (at least at this stage). I probably agree with Timo that x64 is now more important than PAE

Than please revert the change. We will continue the discussion some time in future

vgalnt · 2020-10-27T15:19:42Z

Since this PR started back in April, and now it is almost November, and due to the fact that Jérôme Gardou will now work on the MM, I think it would be logical to close it. If Jerome needs my help, he can always turn to me.

vgalnt requested review from HeisSpiter, ThFabba and tkreuzer as code owners April 5, 2020 14:03

tkreuzer reviewed Apr 9, 2020

View reviewed changes

binarymaster added the refactoring For refactoring changes. label Apr 15, 2020

Extravert-ir mentioned this pull request Oct 23, 2020

[NTOS:MM] PAE uses 2 levels of paging only, not 3 #3315

Closed

vgalnt added 10 commits October 24, 2020 18:33

[NTOS:MM] Removing duplicates.

7a6cce9

[NTOS:MM] For X86 using PD_COUNT instead PPE_PER_PAGE.

7a776ab

[NTOS:MM] Using MiAddressToPdeOffset() instead MiGetPdeOffset().

5603b62

[NTOS:MM] Making translation macros for x86 versatility for systems w…

054a049

…ith PAE. - Make also the definitions and macros more human-readable. - Add MiGetPteOffset, MiGetPdeOffset and MiGetPdIndex macros.

[NTOS:MM] Swap definitions for MiGetPdIndex().

5737109

[NTOS:MM] Use a copy of the system page directory only for (_MI_PAGIN…

d54a18c

…G_LEVELS == 2).

[NTOS:MM] On x86 PPE not used.

50a2a45

[NTOS:MM] Fix MI_IS_MAPPED_PTE().

5a22f08

[NTOS:MM] Making MiProtoPteToPte and MI_MAKE_PROTOTYPE_PTE macros uni…

d4da8cf

…versal (also for PAE). Add check bottom 2 bits for x86 nonPAE. Edit comments.

vgalnt force-pushed the i386_MM_H_redefinition2 branch 3 times, most recently from 551d4bd to d4da8cf Compare October 24, 2020 16:27

[NTOS:MM] Making MiSubsectionPteToSubsection and MI_MAKE_SUBSECTION_P…

4517f89

…TE macros universal (also for PAE). Add check bottom 3 bits for x86 nonPAE. Edit comments.

vgalnt force-pushed the i386_MM_H_redefinition2 branch from c6b20ee to 4517f89 Compare October 24, 2020 17:34

HBelusca requested a review from zefklop October 24, 2020 18:17

tkreuzer reviewed Oct 24, 2020

View reviewed changes

[NTOS:MM] Use _In_ and _Out_.

7f757a5

Extravert-ir reviewed Oct 26, 2020

View reviewed changes

vgalnt closed this Oct 27, 2020

binarymaster added the incomplete PRs that were closed without a real reason (check out later) label Dec 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements for MM. #2501

Improvements for MM. #2501

vgalnt commented Apr 5, 2020

SergeGautherie commented Apr 5, 2020

vgalnt commented Apr 6, 2020

SergeGautherie commented Apr 6, 2020

vgalnt commented Apr 6, 2020

tkreuzer Apr 9, 2020

vgalnt Oct 23, 2020

tkreuzer Oct 24, 2020

vgalnt Oct 24, 2020

tkreuzer Oct 24, 2020 •

edited

tkreuzer Oct 24, 2020

vgalnt Oct 25, 2020

tkreuzer left a comment •

edited

tkreuzer Oct 24, 2020

vgalnt Oct 25, 2020

tkreuzer Oct 24, 2020

vgalnt Oct 25, 2020

tkreuzer Oct 24, 2020

vgalnt Oct 25, 2020

tkreuzer Oct 24, 2020

vgalnt Oct 25, 2020

tkreuzer Oct 24, 2020

vgalnt Oct 25, 2020

vgalnt commented Oct 25, 2020

tkreuzer commented Oct 25, 2020

vgalnt commented Oct 25, 2020

tkreuzer commented Oct 25, 2020

binarymaster commented Oct 25, 2020

Extravert-ir Oct 26, 2020

vgalnt Oct 26, 2020

vgalnt Oct 26, 2020

Extravert-ir Oct 26, 2020

vgalnt Oct 26, 2020

Extravert-ir Oct 27, 2020

vgalnt commented Oct 27, 2020

Improvements for MM. #2501

Improvements for MM. #2501

Conversation

vgalnt commented Apr 5, 2020

SergeGautherie commented Apr 5, 2020

vgalnt commented Apr 6, 2020

SergeGautherie commented Apr 6, 2020

vgalnt commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkreuzer Oct 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkreuzer left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgalnt commented Oct 25, 2020

tkreuzer commented Oct 25, 2020

vgalnt commented Oct 25, 2020

tkreuzer commented Oct 25, 2020

binarymaster commented Oct 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgalnt commented Oct 27, 2020

tkreuzer Oct 24, 2020 •

edited

tkreuzer left a comment •

edited