Re: [PATCH] minios: do not pin page tables [was: Really need to pin page tables?]

[Samuel Thibault <samuel.thibault@eu.citrix.com>]

Jeremy Fitzhardinge, le Tue 27 May 2008 11:49:33 +0100, a écrit :
> Samuel Thibault wrote:
> >Hello,
> >
> >In extras/mini-os/arch/x86/mm.c:new_pt_frame, Mini-OS pins its L1,
> >L2, and L3 page tables.  Does that really make a difference from the
> >Hypervisor point of view?  I mean, once L4 is pinned, pointing to these,
> >and thus their content has been checked, is there any performance
> >difference?
> >  
> 
> Shouldn't be.  Pinning an L4 implicitly pins everything else below it.  
> The only reason to pin the leafy parts of a pagetable is if you want to 
> play games with incrementally pinning the pagetable, or if you want to 
> pull them apart and rearrange the pieces for some reason.  For example, 
> I do incremental pagetable pins in the Xen/pvops kernel to limit the 
> number of pte locks I need to hold at any one time.

Ok, that's what I had in mind indeed.  Here is a patch to drop that from
Mini-OS.

Samuel



minios: We do not need to pin the page tables, as they implicitely get
pinned when we point the permanent page directory to them.

Signed-off-by: Samuel Thibault <samuel.thibault@eu.citrix.com>

diff -r 2141ac752316 extras/mini-os/arch/x86/mm.c
--- a/extras/mini-os/arch/x86/mm.c	Fri May 23 15:43:32 2008 +0100
+++ b/extras/mini-os/arch/x86/mm.c	Tue May 27 11:51:43 2008 +0100
@@ -59,11 +59,10 @@
 {   
     pgentry_t *tab = (pgentry_t *)start_info.pt_base;
     unsigned long pt_page = (unsigned long)pfn_to_virt(*pt_pfn); 
-    unsigned long prot_e, prot_t, pincmd;
+    unsigned long prot_e, prot_t;
     mmu_update_t mmu_updates[1];
-    struct mmuext_op pin_request;
     
-    prot_e = prot_t = pincmd = 0;
+    prot_e = prot_t = 0;
     DEBUG("Allocating new L%d pt frame for pt_pfn=%lx, "
            "prev_l_mfn=%lx, offset=%lx", 
            level, *pt_pfn, prev_l_mfn, offset);
@@ -77,18 +76,15 @@
     case L1_FRAME:
          prot_e = L1_PROT;
          prot_t = L2_PROT;
-         pincmd = MMUEXT_PIN_L1_TABLE;
          break;
     case L2_FRAME:
          prot_e = L2_PROT;
          prot_t = L3_PROT;
-         pincmd = MMUEXT_PIN_L2_TABLE;
          break;
 #if defined(__x86_64__)
     case L3_FRAME:
          prot_e = L3_PROT;
          prot_t = L4_PROT;
-         pincmd = MMUEXT_PIN_L3_TABLE;
          break;
 #endif
     default:
@@ -113,15 +109,6 @@
          do_exit();
     }
                         
-    /* Pin the page to provide correct protection */
-    pin_request.cmd = pincmd;
-    pin_request.arg1.mfn = pfn_to_mfn(*pt_pfn);
-    if(HYPERVISOR_mmuext_op(&pin_request, 1, NULL, DOMID_SELF) < 0)
-    {
-        printk("ERROR: pinning failed\n");
-        do_exit();
-    }
-
     /* Now fill the new page table page with entries.
        Update the page directory as well. */
     mmu_updates[0].ptr = ((pgentry_t)prev_l_mfn << PAGE_SHIFT) + sizeof(pgentry_t) * offset;