Memory Manager Patch to allow mapping of device memory into UserMode - please review.

[Gregor Anich]

Hi!

I have changed the memory manager to allow mapping of sections from 
\Device\PhysicalMemory which are outside of RAM - this gets NVIDIA 
OpenGL working.
I hope my changes are correct (according to Hartmuts suggestions in this 
mail: 
http://reactos.com:8080/archives/public/ros-kernel/2004-August/004444.html)

I changed MmProbeAndLockPages to set MDL_IO_SPACE in the MdlFlags of the 
Mdl if one of the Pfns described in the MDL is outside of the RAM (>= 
MmPageArraySize) which is then used in MmMapLockedPagesSpecifyCache to 
decide wether to call MmCreateVirtualMapping or 
MmCreateVirtualMappingUnsafe (in order to prevent bugchecks in 
mm/freelist.c when Pfns are checked against MmPageArraySize)

- blight

Index: include/internal/mm.h
===================================================================
--- include/internal/mm.h	(revision 13428)
+++ include/internal/mm.h	(working copy)
@@ -626,6 +626,7 @@
 VOID MmSetLRULastPage(PFN_TYPE Page);
 
 VOID MmLockPage(PFN_TYPE Page);
+VOID MmLockPageUnsafe(PFN_TYPE Page);
 
 VOID MmUnlockPage(PFN_TYPE Page);
 
@@ -710,6 +711,7 @@
 VOID MmDereferencePage(PFN_TYPE Page);
 
 VOID MmReferencePage(PFN_TYPE Page);
+VOID MmReferencePageUnsafe(PFN_TYPE Page);
 
 BOOLEAN MmIsAccessedAndResetAccessPage(struct _EPROCESS* Process, PVOID Address);
 
Index: mm/section.c
===================================================================
--- mm/section.c	(revision 13428)
+++ mm/section.c	(working copy)
@@ -721,6 +721,9 @@
 
          MmSharePageEntrySectionSegment(Segment, Offset);
 
+         /* FIXME: Should we call MmCreateVirtualMappingUnsafe if
+          * (Section->AllocationAttributes & SEC_PHYSICALMEMORY) is true?
+          */
          Status = MmCreateVirtualMapping(MemoryArea->Process,
                                          Address,
                                          Attributes,
@@ -823,14 +826,14 @@
       * Just map the desired physical page
       */
       Page = (Offset + MemoryArea->Data.SectionData.ViewOffset) >> PAGE_SHIFT;
-      Status = MmCreateVirtualMapping(AddressSpace->Process,
-                                      Address,
-                                      Region->Protect,
-                                      &Page,
-                                      1);
+      Status = MmCreateVirtualMappingUnsafe(AddressSpace->Process,
+                                            Address,
+                                            Region->Protect,
+                                            &Page,
+                                            1);
       if (!NT_SUCCESS(Status))
       {
-         DPRINT("MmCreateVirtualMapping failed, not out of memory\n");
+         DPRINT("MmCreateVirtualMappingUnsafe failed, not out of memory\n");
          KEBUGCHECK(0);
          return(Status);
       }
@@ -840,7 +843,7 @@
       */
       if (Locked)
       {
-         MmLockPage(Page);
+         MmLockPageUnsafe(Page);
       }
 
       /*
Index: mm/mdl.c
===================================================================
--- mm/mdl.c	(revision 13428)
+++ mm/mdl.c	(working copy)
@@ -29,7 +29,7 @@
 /*
 MDL Flags desc.
 
-MDL_PAGES_LOCKED              MmProbelAndLockPages has been called for this mdl
+MDL_PAGES_LOCKED              MmProbeAndLockPages has been called for this mdl
 MDL_SOURCE_IS_NONPAGED_POOL   mdl has been build by MmBuildMdlForNonPagedPool
 MDL_PARTIAL                   mdl has been built by IoBuildPartialMdl
 MDL_MAPPING_CAN_FAIL          in case of an error, MmMapLockedPages will return NULL
instead of to bugcheck
@@ -300,7 +300,7 @@
 {
    memcpy(Mdl + 1, Pages, sizeof(PFN_TYPE) *
(PAGE_ROUND_UP(Mdl->ByteOffset+Mdl->ByteCount)/PAGE_SIZE));
 
-   //FIXME: this flag should be set by the caller perhaps?
+   /* FIXME: this flag should be set by the caller perhaps? */
    Mdl->MdlFlags |= MDL_IO_PAGE_READ;
 }
 
@@ -374,8 +374,8 @@
    ASSERT(NrPages <= (Mdl->Size - sizeof(MDL))/sizeof(PFN_TYPE));
 
 
-   if (Mdl->StartVa >= (PVOID)KERNEL_BASE && 
-       MmGetPfnForProcess(NULL, Mdl->StartVa) > MmPageArraySize)
+   if (Mdl->StartVa >= (PVOID)KERNEL_BASE &&
+       MmGetPfnForProcess(NULL, Mdl->StartVa) >= MmPageArraySize)
    {
        /* phys addr is not phys memory so this must be io memory */
        
@@ -391,13 +391,13 @@
 
    if (Mdl->StartVa >= (PVOID)KERNEL_BASE)
    {
-      //FIXME: why isn't AccessMode used?
+      /* FIXME: why isn't AccessMode used? */
       Mode = KernelMode;
       Mdl->Process = NULL;
    }
    else
    {
-      //FIXME: why isn't AccessMode used?
+      /* FIXME: why isn't AccessMode used? */
       Mode = UserMode;
       Mdl->Process = CurrentProcess;      
    }
@@ -455,7 +455,10 @@
       }
       Page = MmGetPfnForProcess(NULL, Address);
       MdlPages[i] = Page;
-      MmReferencePage(Page);
+      if (Page >= MmPageArraySize)
+         Mdl->MdlFlags |= MDL_IO_SPACE;
+      else
+         MmReferencePage(Page);
    }
    
    MmUnlockAddressSpace(&CurrentProcess->AddressSpace);
@@ -848,11 +851,18 @@
       Protect |= PAGE_NOCACHE;
    else if (CacheType == MmWriteCombined)
       DPRINT("CacheType MmWriteCombined not supported!\n");
-   Status = MmCreateVirtualMapping(CurrentProcess,
-                                   Base,
-                                   Protect,
-                                   MdlPages,
-                                   PageCount);
+   if (Mdl->MdlFlags & MDL_IO_SPACE)
+      Status = MmCreateVirtualMappingUnsafe(CurrentProcess,
+                                            Base,
+                                            Protect,
+                                            MdlPages,
+                                            PageCount);
+   else
+      Status = MmCreateVirtualMapping(CurrentProcess,
+                                      Base,
+                                      Protect,
+                                      MdlPages,
+                                      PageCount);
    if (!NT_SUCCESS(Status))
    {
       DbgPrint("Unable to create virtual mapping\n");
Index: mm/freelist.c
===================================================================
--- mm/freelist.c	(revision 13428)
+++ mm/freelist.c	(working copy)
@@ -612,15 +612,15 @@
 }
 
 VOID
-MmReferencePage(PFN_TYPE Pfn)
+MmReferencePageUnsafe(PFN_TYPE Pfn)
 {
    KIRQL oldIrql;
 
-   DPRINT("MmReferencePage(PysicalAddress %x)\n", Pfn << PAGE_SHIFT);
+   DPRINT("MmReferencePageUnsafe(PysicalAddress %x)\n", Pfn <<
PAGE_SHIFT);
 
    if (Pfn == 0 || Pfn >= MmPageArraySize)
    {
-      KEBUGCHECK(0);
+      return;
    }
 
    KeAcquireSpinLock(&PageListLock, &oldIrql);
@@ -635,6 +635,19 @@
    KeReleaseSpinLock(&PageListLock, oldIrql);
 }
 
+VOID
+MmReferencePage(PFN_TYPE Pfn)
+{
+   DPRINT("MmReferencePage(PysicalAddress %x)\n", Pfn << PAGE_SHIFT);
+
+   if (Pfn == 0 || Pfn >= MmPageArraySize)
+   {
+      KEBUGCHECK(0);
+   }
+
+   MmReferencePageUnsafe(Pfn);
+}
+
 ULONG
 MmGetReferenceCountPage(PFN_TYPE Pfn)
 {
@@ -781,15 +794,15 @@
 }
 
 VOID
-MmLockPage(PFN_TYPE Pfn)
+MmLockPageUnsafe(PFN_TYPE Pfn)
 {
    KIRQL oldIrql;
 
-   DPRINT("MmLockPage(PhysicalAddress %x)\n", Pfn << PAGE_SHIFT);
+   DPRINT("MmLockPageUnsafe(PhysicalAddress %x)\n", Pfn << PAGE_SHIFT);
 
    if (Pfn == 0 || Pfn >= MmPageArraySize)
    {
-      KEBUGCHECK(0);
+      return;
    }
 
    KeAcquireSpinLock(&PageListLock, &oldIrql);
@@ -805,6 +818,19 @@
 }
 
 VOID
+MmLockPage(PFN_TYPE Pfn)
+{
+   DPRINT("MmLockPage(PhysicalAddress %x)\n", Pfn << PAGE_SHIFT);
+
+   if (Pfn == 0 || Pfn >= MmPageArraySize)
+   {
+      KEBUGCHECK(0);
+   }
+   
+   MmLockPageUnsafe(Pfn);
+}
+
+VOID
 MmUnlockPage(PFN_TYPE Pfn)
 {
    KIRQL oldIrql;

[Hartmut Birr]

Gregor Anich schrieb:

>   Hi!
>
>  I have changed the memory manager to allow mapping of sections from 
>  \Device\PhysicalMemory which are outside of RAM - this gets NVIDIA 
>  OpenGL working.
>  I hope my changes are correct (according to Hartmuts suggestions in 
>  this mail: 
>  http://reactos.com:8080/archives/public/ros-kernel/2004-August/004444.html) 
>
>
>  I changed MmProbeAndLockPages to set MDL_IO_SPACE in the MdlFlags of 
>  the Mdl if one of the Pfns described in the MDL is outside of the RAM 
>  (>= MmPageArraySize) which is then used in 
>  MmMapLockedPagesSpecifyCache to decide wether to call 
>  MmCreateVirtualMapping or MmCreateVirtualMappingUnsafe (in order to 
>  prevent bugchecks in mm/freelist.c when Pfns are checked against 
>  MmPageArraySize)
>
>  - blight
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> Ros-dev mailing list
> Ros-dev(a)reactos.com
> http://reactos.com:8080/mailman/listinfo/ros-dev
>  
Hi,

I think that your patch isn't complete.

- Mapping of physical memory for \Device\PhysicalMemory in section.c:
We should use MmCreateVirtualMappingForKernel instead of 
MmCreateVirtualMappingUnsafe because MmCreateVirtualMappingUnsafe marks 
the pages as mapped in the pfn array. If MmCreateVirtualMappingForKernel 
is used for mapping the pages, the delete functions like 
MmUnmapViewOfxxx and MmFreeMemoryArea must also use other functions.

- Lock the pages with MmProbeAndLockPages in mdl.c:
MmProbeAndLockPages may crash ros in some situations. On error there is 
no check which unmap/unlock function must be use. Possible we must check 
the type of the memory area for eache address and page.

- Hartmut

[Gregor Anich]

Hartmut Birr wrote:

>   Gregor Anich schrieb:
>
> >   Hi!
> >
> >  I have changed the memory manager to allow mapping of sections from 
> >  \Device\PhysicalMemory which are outside of RAM - this gets NVIDIA 
> >  OpenGL working.
> >  I hope my changes are correct (according to Hartmuts suggestions in 
> >  this mail: 
> >  http://reactos.com:8080/archives/public/ros-kernel/2004-August/004444.html) 
> >
> >
> >  I changed MmProbeAndLockPages to set MDL_IO_SPACE in the MdlFlags of 
> >  the Mdl if one of the Pfns described in the MDL is outside of the RAM 
> >  (>= MmPageArraySize) which is then used in 
> >  MmMapLockedPagesSpecifyCache to decide wether to call 
> >  MmCreateVirtualMapping or MmCreateVirtualMappingUnsafe (in order to 
> >  prevent bugchecks in mm/freelist.c when Pfns are checked against 
> >  MmPageArraySize)
> >
> >  - blight
> >  
>  Hi,
>
>  I think that your patch isn't complete.
>
>  - Mapping of physical memory for \Device\PhysicalMemory in section.c:
>  We should use MmCreateVirtualMappingForKernel instead of 
>  MmCreateVirtualMappingUnsafe because MmCreateVirtualMappingUnsafe 
>  marks the pages as mapped in the pfn array. If 
>  MmCreateVirtualMappingForKernel is used for mapping the pages, the 
>  delete functions like MmUnmapViewOfxxx and MmFreeMemoryArea must also 
>  use other functions.
>
>  - Lock the pages with MmProbeAndLockPages in mdl.c:
>  MmProbeAndLockPages may crash ros in some situations. On error there 
>  is no check which unmap/unlock function must be use. Possible we must 
>  check the type of the memory area for eache address and page.
>
>  - Hartmut 
Hi!

My reply comes a bit late ... I have taken a little break from OpenGL 
stuff , but today I looked at it again.

Why should MmCreateVirtualMappingForKernel be used instead of 
MmCreateVirtualMappingUnsafe in section.c? the whole point of my changes 
is to allow mapping of the device memory into usermode, which 
MmCreateVirtualMappingForKernel will not do as far as I can tell ;-), so 
I don't think that is needed or don't know what you mean.

I will do a bit more testing and send another patch soon I think.

 - blight