Re: [Xen-devel] [PATCH] minios: do not pin page tables [was: Really need to pin page tables?] - Samuel Thibault - com.xensource.lists.xen-devel

4 messages in com.xensource.lists.xen-develRe: [Xen-devel] [PATCH] minios: do no...

From	Sent On	Attachments
Samuel Thibault	27 May 2008 03:16
Keir Fraser	27 May 2008 03:47
Jeremy Fitzhardinge	27 May 2008 03:49
Samuel Thibault	27 May 2008 03:58

Subject:	Re: [Xen-devel] [PATCH] minios: do not pin page tables [was: Really need to pin page tables?]
From:	Samuel Thibault (samu...@eu.citrix.com)
Date:	05/27/2008 03:58:13 AM
List:	com.xensource.lists.xen-devel

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 <samu...@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;