[ros-dev] [ros-diffs] [tkreuzer] 64589: [NTOSKRNL] Implement MiInsertVadEx, replacing duplicated code from NtAllocateVirtualMemory and MiMapViewOfDataSection
Alex Ionescu
ionucu at videotron.ca
Sat Oct 11 16:41:55 UTC 2014
I think it would be really great that instead of combining functions
together for "removing code duplication" and not even bothering to think
why there's a reason why the code was duplicated, people would work on
actual improvements like Jerome or Pierre, instead of re-factoring code
that works just to make it 'simpler'. Especially since this introduced a
few bugs which had to be later fixed. There's a million bugs in ReactOS,
including in the memory manager, so fixing those seems to make more sense
than refactoring.
Just my 2 cents.
Best regards,
Alex Ionescu
On Tue, Oct 7, 2014 at 5:31 PM, <tkreuzer at svn.reactos.org> wrote:
> Author: tkreuzer
> Date: Wed Oct 8 00:31:17 2014
> New Revision: 64589
>
> URL: http://svn.reactos.org/svn/reactos?rev=64589&view=rev
> Log:
> [NTOSKRNL]
> Implement MiInsertVadEx, replacing duplicated code from
> NtAllocateVirtualMemory and MiMapViewOfDataSection
>
> Modified:
> trunk/reactos/ntoskrnl/mm/ARM3/miarm.h
> trunk/reactos/ntoskrnl/mm/ARM3/section.c
> trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c
> trunk/reactos/ntoskrnl/mm/ARM3/virtual.c
>
> Modified: trunk/reactos/ntoskrnl/mm/ARM3/miarm.h
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/miarm.h?rev=64589&r1=64588&r2=64589&view=diff
>
> ==============================================================================
> --- trunk/reactos/ntoskrnl/mm/ARM3/miarm.h [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/mm/ARM3/miarm.h [iso-8859-1] Wed Oct 8
> 00:31:17 2014
> @@ -2144,6 +2144,16 @@
> IN PEPROCESS Process
> );
>
> +NTSTATUS
> +NTAPI
> +MiInsertVadEx(
> + _Inout_ PMMVAD Vad,
> + _In_ ULONG_PTR *BaseAddress,
> + _In_ SIZE_T ViewSize,
> + _In_ ULONG_PTR HighestAddress,
> + _In_ ULONG_PTR Alignment,
> + _In_ ULONG AllocationType);
> +
> VOID
> NTAPI
> MiInsertBasedSection(
>
> Modified: trunk/reactos/ntoskrnl/mm/ARM3/section.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/section.c?rev=64589&r1=64588&r2=64589&view=diff
>
> ==============================================================================
> --- trunk/reactos/ntoskrnl/mm/ARM3/section.c [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/mm/ARM3/section.c [iso-8859-1] Wed Oct 8
> 00:31:17 2014
> @@ -1252,9 +1252,7 @@
> IN ULONG AllocationType)
> {
> PMMVAD_LONG Vad;
> - PETHREAD Thread = PsGetCurrentThread();
> - PMMSUPPORT AddressSpace;
> - ULONG_PTR StartAddress, EndingAddress;
> + ULONG_PTR StartAddress;
> ULONG_PTR ViewSizeInPages;
> PSUBSECTION Subsection;
> PSEGMENT Segment;
> @@ -1263,8 +1261,6 @@
> ULONG QuotaCharge = 0, QuotaExcess = 0;
> PMMPTE PointerPte, LastPte;
> MMPTE TempPte;
> - PMMADDRESS_NODE Parent;
> - TABLE_SEARCH_RESULT Result;
> DPRINT("Mapping ARM3 data section\n");
>
> /* Get the segment for this section */
> @@ -1361,6 +1357,7 @@
>
> /* Write all the data required in the VAD for handling a fault */
> Vad->ControlArea = ControlArea;
> + Vad->u.VadFlags.CommitCharge = 0;
> Vad->u.VadFlags.Protection = ProtectionMask;
> Vad->u2.VadFlags2.FileOffset = (ULONG)(SectionOffset->QuadPart >> 16);
> Vad->u2.VadFlags2.Inherit = (InheritDisposition == ViewShare);
> @@ -1437,100 +1434,23 @@
> StartAddress = 0;
> }
>
> - /* Lock the address space and make sure the process is alive */
> - AddressSpace = MmGetCurrentAddressSpace();
> - MmLockAddressSpace(AddressSpace);
> - if (Process->VmDeleted)
> - {
> - MmUnlockAddressSpace(AddressSpace);
> - ExFreePoolWithTag(Vad, 'ldaV');
> - DPRINT1("The process is dying\n");
> - return STATUS_PROCESS_IS_TERMINATING;
> - }
> -
> - /* Did the caller specify an address? */
> - if (StartAddress == 0)
> - {
> - /* ARM3 does not support these flags yet */
> - ASSERT(Process->VmTopDown == 0);
> - ASSERT(ZeroBits == 0);
> -
> - /* Which way should we search? */
> - if (AllocationType & MEM_TOP_DOWN)
> - {
> - /* No, find an address top-down */
> - Result = MiFindEmptyAddressRangeDownTree(*ViewSize,
> -
> (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS,
> - _64K,
> - &Process->VadRoot,
> - &StartAddress,
> - &Parent);
> - }
> - else
> - {
> - /* No, find an address bottom-up */
> - Result = MiFindEmptyAddressRangeInTree(*ViewSize,
> - _64K,
> - &Process->VadRoot,
> - &Parent,
> - &StartAddress);
> - }
> -
> - /* Check if we found a suitable location */
> - if (Result == TableFoundNode)
> - {
> - DPRINT1("Not enough free space to insert this section!\n");
> - MmUnlockAddressSpace(AddressSpace);
> - MiDereferenceControlArea(ControlArea);
> - ExFreePoolWithTag(Vad, 'ldaV');
> - return STATUS_CONFLICTING_ADDRESSES;
> - }
> -
> - /* Get the ending address, which is the last piece we need for
> the VAD */
> - EndingAddress = StartAddress + (ViewSizeInPages * PAGE_SIZE) - 1;
> - }
> - else
> - {
> - /* Get the ending address, which is the last piece we need for
> the VAD */
> - EndingAddress = StartAddress + (ViewSizeInPages * PAGE_SIZE) - 1;
> -
> - /* Make sure it doesn't conflict with an existing allocation */
> - Result = MiCheckForConflictingNode(StartAddress >> PAGE_SHIFT,
> - EndingAddress >> PAGE_SHIFT,
> - &Process->VadRoot,
> - &Parent);
> - if (Result == TableFoundNode)
> - {
> - DPRINT1("Conflict with SEC_BASED or manually based
> section!\n");
> - MmUnlockAddressSpace(AddressSpace);
> - MiDereferenceControlArea(ControlArea);
> - ExFreePoolWithTag(Vad, 'ldaV');
> - return STATUS_CONFLICTING_ADDRESSES;
> - }
> - }
> -
> - /* Now set the VAD address */
> - Vad->StartingVpn = StartAddress >> PAGE_SHIFT;
> - Vad->EndingVpn = EndingAddress >> PAGE_SHIFT;
> -
> - /* Pretend as if we own the working set */
> - MiLockProcessWorkingSetUnsafe(Process, Thread);
> -
> /* Insert the VAD */
> - Process->VadRoot.NodeHint = Vad;
> - MiInsertNode(&Process->VadRoot, (PVOID)Vad, Parent, Result);
> -
> - /* Release the working set */
> - MiUnlockProcessWorkingSetUnsafe(Process, Thread);
> -
> - /* Unlock the address space */
> - MmUnlockAddressSpace(AddressSpace);
> + Status = MiInsertVadEx((PMMVAD)Vad,
> + &StartAddress,
> + ViewSizeInPages * PAGE_SIZE,
> + MAXULONG_PTR >> ZeroBits,
> + MM_VIRTMEM_GRANULARITY,
> + AllocationType);
> + if (!NT_SUCCESS(Status))
> + {
> + return Status;
> + }
>
> /* Windows stores this for accounting purposes, do so as well */
> if (!Segment->u2.FirstMappedVa) Segment->u2.FirstMappedVa =
> (PVOID)StartAddress;
>
> /* Finally, let the caller know where, and for what size, the view
> was mapped */
> - *ViewSize = (ULONG_PTR)EndingAddress - (ULONG_PTR)StartAddress + 1;
> + *ViewSize = ViewSizeInPages * PAGE_SIZE;
> *BaseAddress = (PVOID)StartAddress;
> DPRINT("Start and region: 0x%p, 0x%p\n", *BaseAddress, *ViewSize);
> return STATUS_SUCCESS;
>
> Modified: trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c?rev=64589&r1=64588&r2=64589&view=diff
>
> ==============================================================================
> --- trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c [iso-8859-1] Wed Oct 8
> 00:31:17 2014
> @@ -193,6 +193,138 @@
> MiInsertNode(&Process->VadRoot, (PVOID)Vad, Parent, Result);
> }
>
> +NTSTATUS
> +NTAPI
> +MiInsertVadEx(
> + _Inout_ PMMVAD Vad,
> + _In_ ULONG_PTR *BaseAddress,
> + _In_ SIZE_T ViewSize,
> + _In_ ULONG_PTR HighestAddress,
> + _In_ ULONG_PTR Alignment,
> + _In_ ULONG AllocationType)
> +{
> + ULONG_PTR StartingAddress, EndingAddress;
> + PEPROCESS CurrentProcess;
> + PETHREAD CurrentThread;
> + TABLE_SEARCH_RESULT Result;
> + PMMADDRESS_NODE Parent;
> +
> + /* Align the view size to pages */
> + ViewSize = ALIGN_UP_BY(ViewSize, PAGE_SIZE);
> +
> + /* Get the current process */
> + CurrentProcess = PsGetCurrentProcess();
> +
> + /* Acquire the address creation lock and make sure the process is
> alive */
> + KeAcquireGuardedMutex(&CurrentProcess->AddressCreationLock);
> + if (CurrentProcess->VmDeleted)
> + {
> + KeReleaseGuardedMutex(&CurrentProcess->AddressCreationLock);
> + DPRINT1("The process is dying\n");
> + return STATUS_PROCESS_IS_TERMINATING;
> + }
> +
> + /* Did the caller specify an address? */
> + if (*BaseAddress == 0)
> + {
> + /* Make sure HighestAddress is not too large */
> + HighestAddress = min(HighestAddress,
> (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS);
> +
> + /* Which way should we search? */
> + if ((AllocationType & MEM_TOP_DOWN) || CurrentProcess->VmTopDown)
> + {
> + /* Find an address top-down */
> + Result = MiFindEmptyAddressRangeDownTree(ViewSize,
> + HighestAddress,
> + Alignment,
> +
> &CurrentProcess->VadRoot,
> + &StartingAddress,
> + &Parent);
> + }
> + else
> + {
> + /* Find an address bottom-up */
> + Result = MiFindEmptyAddressRangeInTree(ViewSize,
> + Alignment,
> +
> &CurrentProcess->VadRoot,
> + &Parent,
> + &StartingAddress);
> + }
> +
> + /* Get the ending address, which is the last piece we need for
> the VAD */
> + EndingAddress = StartingAddress + ViewSize - 1;
> +
> + /* Check if we found a suitable location */
> + if ((Result == TableFoundNode) || (EndingAddress >
> HighestAddress))
> + {
> + DPRINT1("Not enough free space to insert this VAD node!\n");
> + KeReleaseGuardedMutex(&CurrentProcess->AddressCreationLock);
> + return STATUS_CONFLICTING_ADDRESSES;
> + }
> +
> + ASSERT(StartingAddress != 0);
> + ASSERT(StartingAddress < (ULONG_PTR)HighestAddress);
> + ASSERT(EndingAddress > StartingAddress);
> + }
> + else
> + {
> + /* Calculate the starting and ending address */
> + StartingAddress = ALIGN_DOWN_BY(*BaseAddress, Alignment);
> + EndingAddress = StartingAddress + ViewSize - 1;
> +
> + /* Make sure it doesn't conflict with an existing allocation */
> + Result = MiCheckForConflictingNode(StartingAddress >> PAGE_SHIFT,
> + EndingAddress >> PAGE_SHIFT,
> + &CurrentProcess->VadRoot,
> + &Parent);
> + if (Result == TableFoundNode)
> + {
> + DPRINT1("Given address conflicts with existing node\n");
> + KeReleaseGuardedMutex(&CurrentProcess->AddressCreationLock);
> + return STATUS_CONFLICTING_ADDRESSES;
> + }
> + }
> +
> + /* Now set the VAD address */
> + Vad->StartingVpn = StartingAddress >> PAGE_SHIFT;
> + Vad->EndingVpn = EndingAddress >> PAGE_SHIFT;
> +
> + /* Check if the VAD is to be secured */
> + if (Vad->u2.VadFlags2.OneSecured)
> + {
> + /* This *must* be a long VAD! */
> + ASSERT(Vad->u2.VadFlags2.LongVad);
> +
> + /* Yeah this is retarded, I didn't invent it! */
> + ((PMMVAD_LONG)Vad)->u3.Secured.StartVpn = StartingAddress;
> + ((PMMVAD_LONG)Vad)->u3.Secured.EndVpn = EndingAddress;
> + }
> +
> + /* Lock the working set */
> + CurrentThread = PsGetCurrentThread();
> + MiLockProcessWorkingSetUnsafe(CurrentProcess, CurrentThread);
> +
> + /* Insert the VAD */
> + CurrentProcess->VadRoot.NodeHint = Vad;
> + MiInsertNode(&CurrentProcess->VadRoot, (PVOID)Vad, Parent, Result);
> +
> + /* Release the working set */
> + MiUnlockProcessWorkingSetUnsafe(CurrentProcess, CurrentThread);
> +
> + /* Update the process' virtual size, and peak virtual size */
> + CurrentProcess->VirtualSize += ViewSize;
> + if (CurrentProcess->VirtualSize > CurrentProcess->PeakVirtualSize)
> + {
> + CurrentProcess->PeakVirtualSize = CurrentProcess->VirtualSize;
> + }
> +
> + /* Unlock the address space */
> + KeReleaseGuardedMutex(&CurrentProcess->AddressCreationLock);
> +
> + *BaseAddress = StartingAddress;
> + return STATUS_SUCCESS;
> +}
> +
> VOID
> NTAPI
> MiInsertBasedSection(IN PSECTION Section)
>
> Modified: trunk/reactos/ntoskrnl/mm/ARM3/virtual.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/virtual.c?rev=64589&r1=64588&r2=64589&view=diff
>
> ==============================================================================
> --- trunk/reactos/ntoskrnl/mm/ARM3/virtual.c [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/mm/ARM3/virtual.c [iso-8859-1] Wed Oct 8
> 00:31:17 2014
> @@ -4304,12 +4304,12 @@
> {
> PEPROCESS Process;
> PMEMORY_AREA MemoryArea;
> - PFN_NUMBER PageCount;
> PMMVAD Vad = NULL, FoundVad;
> NTSTATUS Status;
> PMMSUPPORT AddressSpace;
> PVOID PBaseAddress;
> - ULONG_PTR PRegionSize, StartingAddress, EndingAddress, HighestAddress;
> + ULONG_PTR PRegionSize, StartingAddress, EndingAddress;
> + ULONG_PTR HighestAddress = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS;
> PEPROCESS CurrentProcess = PsGetCurrentProcess();
> KPROCESSOR_MODE PreviousMode = KeGetPreviousMode();
> PETHREAD CurrentThread = PsGetCurrentThread();
> @@ -4319,7 +4319,6 @@
> MMPTE TempPte;
> PMMPTE PointerPte, PointerPde, LastPte;
> TABLE_SEARCH_RESULT Result;
> - PMMADDRESS_NODE Parent;
> PAGED_CODE();
>
> /* Check for valid Zero bits */
> @@ -4544,7 +4543,6 @@
> // This is a blind commit, all we need is the region size
> //
> PRegionSize = ROUND_TO_PAGES(PRegionSize);
> - PageCount = BYTES_TO_PAGES(PRegionSize);
> EndingAddress = 0;
> StartingAddress = 0;
>
> @@ -4563,13 +4561,6 @@
> goto FailPathNoLock;
> }
> }
> - else
> - {
> - //
> - // Use the highest VAD address as maximum
> - //
> - HighestAddress = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS;
> - }
> }
> else
> {
> @@ -4579,8 +4570,8 @@
> // fall based on the aligned address and the passed in region
> size
> //
> EndingAddress = ((ULONG_PTR)PBaseAddress + PRegionSize - 1) |
> (PAGE_SIZE - 1);
> - StartingAddress = ROUND_DOWN((ULONG_PTR)PBaseAddress, _64K);
> - PageCount = BYTES_TO_PAGES(EndingAddress - StartingAddress);
> + PRegionSize = EndingAddress + 1 -
> ROUND_DOWN((ULONG_PTR)PBaseAddress, _64K);
> + StartingAddress = (ULONG_PTR)PBaseAddress;
> }
>
> //
> @@ -4594,7 +4585,7 @@
> goto FailPathNoLock;
> }
>
> - Vad->u.LongFlags = 0;
> + RtlZeroMemory(Vad, sizeof(MMVAD_LONG));
> if (AllocationType & MEM_COMMIT) Vad->u.VadFlags.MemCommit = 1;
> Vad->u.VadFlags.Protection = ProtectionMask;
> Vad->u.VadFlags.PrivateMemory = 1;
> @@ -4602,124 +4593,24 @@
> Vad->ControlArea = NULL; // For Memory-Area hack
>
> //
> - // Lock the address space and make sure the process isn't already
> dead
> - //
> - AddressSpace = MmGetCurrentAddressSpace();
> - MmLockAddressSpace(AddressSpace);
> - if (Process->VmDeleted)
> - {
> - Status = STATUS_PROCESS_IS_TERMINATING;
> - goto FailPath;
> - }
> -
> - //
> - // Did we have a base address? If no, find a valid address that
> is 64KB
> - // aligned in the VAD tree. Otherwise, make sure that the address
> range
> - // which was passed in isn't already conflicting with an existing
> address
> - // range.
> - //
> - if (!PBaseAddress)
> - {
> - /* Which way should we search? */
> - if ((AllocationType & MEM_TOP_DOWN) || Process->VmTopDown)
> - {
> - /* Find an address top-down */
> - Result = MiFindEmptyAddressRangeDownTree(PRegionSize,
> - HighestAddress,
> - _64K,
> -
> &Process->VadRoot,
> - &StartingAddress,
> - &Parent);
> - }
> - else
> - {
> - /* Find an address bottom-up */
> - Result = MiFindEmptyAddressRangeInTree(PRegionSize,
> - _64K,
> - &Process->VadRoot,
> - &Parent,
> - &StartingAddress);
> - }
> -
> - if (Result == TableFoundNode)
> - {
> - Status = STATUS_NO_MEMORY;
> - goto FailPath;
> - }
> -
> - //
> - // Now we know where the allocation ends. Make sure it
> doesn't end up
> - // somewhere in kernel mode.
> - //
> - ASSERT(StartingAddress != 0);
> - ASSERT(StartingAddress < (ULONG_PTR)MM_HIGHEST_USER_ADDRESS);
> - EndingAddress = (StartingAddress + PRegionSize - 1) |
> (PAGE_SIZE - 1);
> - ASSERT(EndingAddress > StartingAddress);
> - if (EndingAddress > HighestAddress)
> - {
> - Status = STATUS_NO_MEMORY;
> - goto FailPath;
> - }
> - }
> - else
> - {
> - /* Make sure it doesn't conflict with an existing allocation
> */
> - Result = MiCheckForConflictingNode(StartingAddress >>
> PAGE_SHIFT,
> - EndingAddress >>
> PAGE_SHIFT,
> - &Process->VadRoot,
> - &Parent);
> - if (Result == TableFoundNode)
> - {
> - //
> - // The address specified is in conflict!
> - //
> - Status = STATUS_CONFLICTING_ADDRESSES;
> - goto FailPath;
> - }
> - }
> -
> - //
> - // Write out the VAD fields for this allocation
> - //
> - Vad->StartingVpn = StartingAddress >> PAGE_SHIFT;
> - Vad->EndingVpn = EndingAddress >> PAGE_SHIFT;
> -
> - //
> - // FIXME: Should setup VAD bitmap
> - //
> - Status = STATUS_SUCCESS;
> -
> - //
> - // Lock the working set and insert the VAD into the process VAD
> tree
> - //
> - MiLockProcessWorkingSetUnsafe(Process, CurrentThread);
> - Process->VadRoot.NodeHint = Vad;
> - MiInsertNode(&Process->VadRoot, (PVOID)Vad, Parent, Result);
> - MiUnlockProcessWorkingSetUnsafe(Process, CurrentThread);
> -
> - //
> - // Make sure the actual region size is at least as big as the
> - // requested region size and update the value
> - //
> - ASSERT(PRegionSize <= (EndingAddress + 1 - StartingAddress));
> - PRegionSize = (EndingAddress + 1 - StartingAddress);
> -
> - //
> - // Update the virtual size of the process, and if this is now the
> highest
> - // virtual size we have ever seen, update the peak virtual size
> to reflect
> - // this.
> - //
> - Process->VirtualSize += PRegionSize;
> - if (Process->VirtualSize > Process->PeakVirtualSize)
> - {
> - Process->PeakVirtualSize = Process->VirtualSize;
> - }
> -
> - //
> - // Release address space and detach and dereference the target
> process if
> + // Insert the VAD
> + //
> + Status = MiInsertVadEx(Vad,
> + &StartingAddress,
> + PRegionSize,
> + HighestAddress,
> + MM_VIRTMEM_GRANULARITY,
> + AllocationType);
> + if (!NT_SUCCESS(Status))
> + {
> + DPRINT1("Failed to insert the VAD!\n");
> + goto FailPathNoLock;
> + }
> +
> + //
> + // Detach and dereference the target process if
> // it was different from the current process
> //
> - MmUnlockAddressSpace(AddressSpace);
> if (Attached) KeUnstackDetachProcess(&ApcState);
> if (ProcessHandle != NtCurrentProcess())
> ObDereferenceObject(Process);
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.reactos.org/pipermail/ros-dev/attachments/20141011/4ee41aff/attachment-0001.html>
More information about the Ros-dev
mailing list