[PATCH] abstract out bits of ldt.c

[PATCH] abstract out bits of ldt.c

[Martin J. Bligh]

Starting on the work to merge xen cleanly as a subarch.
Introduce make_pages_readonly and make_pages_writable where appropriate 
for Xen, defined as a no-op on other subarches. Same for 
add_context_to_unpinned and del_context_from_unpinned.
Abstract out install_ldt_entry().

This will do have no effect whatsover on platforms other than xen.

Compile-tested with every config I can find. Boot tested on i386.

diff -aurpN -X /home/fletch/.diff.exclude virgin/arch/i386/kernel/ldt.c xen-ldt.c/arch/i386/kernel/ldt.c
--- virgin/arch/i386/kernel/ldt.c	2004-10-24 19:27:13.000000000 -0700
+++ xen-ldt.c/arch/i386/kernel/ldt.c	2005-08-07 10:45:49.000000000 -0700
@@ -18,6 +18,7 @@
 #include <asm/system.h>
 #include <asm/ldt.h>
 #include <asm/desc.h>
+#include <mach_ldt.h>
 
 #ifdef CONFIG_SMP /* avoids "defined but not used" warnig */
 static void flush_ldt(void *null)
@@ -58,16 +59,22 @@ static int alloc_ldt(mm_context_t *pc, i
 #ifdef CONFIG_SMP
 		cpumask_t mask;
 		preempt_disable();
+		make_pages_readonly(pc->ldt, (pc->size * LDT_ENTRY_SIZE) / 
+								PAGE_SIZE);
 		load_LDT(pc);
 		mask = cpumask_of_cpu(smp_processor_id());
 		if (!cpus_equal(current->mm->cpu_vm_mask, mask))
 			smp_call_function(flush_ldt, NULL, 1, 1);
 		preempt_enable();
 #else
+		make_pages_readonly(pc->ldt, (pc->size * LDT_ENTRY_SIZE) / 
+								PAGE_SIZE);
 		load_LDT(pc);
 #endif
 	}
 	if (oldsize) {
+		make_pages_writable(oldldt, (oldsize * LDT_ENTRY_SIZE) /
+								PAGE_SIZE);
 		if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)
 			vfree(oldldt);
 		else
@@ -82,6 +89,8 @@ static inline int copy_ldt(mm_context_t 
 	if (err < 0)
 		return err;
 	memcpy(new->ldt, old->ldt, old->size*LDT_ENTRY_SIZE);
+	make_pages_readonly(new->ldt, (new->size * LDT_ENTRY_SIZE) /
+								PAGE_SIZE);
 	return 0;
 }
 
@@ -94,14 +103,16 @@ int init_new_context(struct task_struct 
 	struct mm_struct * old_mm;
 	int retval = 0;
 
+	memset(&mm->context, 0, sizeof(mm->context));
 	init_MUTEX(&mm->context.sem);
-	mm->context.size = 0;
 	old_mm = current->mm;
 	if (old_mm && old_mm->context.size > 0) {
 		down(&old_mm->context.sem);
 		retval = copy_ldt(&mm->context, &old_mm->context);
 		up(&old_mm->context.sem);
 	}
+	if (retval == 0)
+		add_context_to_unpinned(mm);
 	return retval;
 }
 
@@ -113,12 +124,16 @@ void destroy_context(struct mm_struct *m
 	if (mm->context.size) {
 		if (mm == current->active_mm)
 			clear_LDT();
+		make_pages_writable(mm->context.ldt,
+				(mm->context.size * LDT_ENTRY_SIZE) /
+				PAGE_SIZE);
 		if (mm->context.size*LDT_ENTRY_SIZE > PAGE_SIZE)
 			vfree(mm->context.ldt);
 		else
 			kfree(mm->context.ldt);
 		mm->context.size = 0;
 	}
+	del_context_from_unpinned(mm);
 }
 
 static int read_ldt(void __user * ptr, unsigned long bytecount)
@@ -223,9 +238,7 @@ static int write_ldt(void __user * ptr, 
 
 	/* Install the new entry ...  */
 install:
-	*lp	= entry_1;
-	*(lp+1)	= entry_2;
-	error = 0;
+	error = install_ldt_entry(lp, entry_1, entry_2);
 
 out_unlock:
 	up(&mm->context.sem);
diff -aurpN -X /home/fletch/.diff.exclude virgin/include/asm-i386/mach-default/mach_ldt.h xen-ldt.c/include/asm-i386/mach-default/mach_ldt.h
--- virgin/include/asm-i386/mach-default/mach_ldt.h	1969-12-31 16:00:00.000000000 -0800
+++ xen-ldt.c/include/asm-i386/mach-default/mach_ldt.h	2005-08-07 10:43:58.000000000 -0700
@@ -0,0 +1,27 @@
+#ifndef __ASM_MACH_LDT_H
+#define __ASM_MACH_LDT_H
+
+static inline void make_pages_readonly(void *va, unsigned int nr)
+{
+}
+
+static inline void make_pages_writable(void *va, unsigned int nr)
+{
+}
+
+static inline void add_context_to_unpinned(struct mm_struct *mm)
+{
+}
+
+static inline void del_context_from_unpinned(struct mm_struct *mm)
+{
+}
+
+static inline int install_ldt_entry (__u32 *lp, __u32 entry_1, __u32 entry_2)
+{
+	*lp     = entry_1;
+	*(lp+1) = entry_2;
+	return 0;
+}
+
+#endif /* __ASM_MACH_LDT_H */
diff -aurpN -X /home/fletch/.diff.exclude virgin/include/asm-i386/mach-xen/mach_ldt.h xen-ldt.c/include/asm-i386/mach-xen/mach_ldt.h
--- virgin/include/asm-i386/mach-xen/mach_ldt.h	1969-12-31 16:00:00.000000000 -0800
+++ xen-ldt.c/include/asm-i386/mach-xen/mach_ldt.h	2005-08-07 10:44:45.000000000 -0700
@@ -0,0 +1,27 @@
+#ifndef __ASM_MACH_LDT_H
+#define __ASM_MACH_LDT_H
+
+static inline void add_context_to_unpinned(struct mm_struct *mm)
+{
+	spin_lock(&mm_unpinned_lock);
+	list_add(&mm->context.unpinned, &mm_unpinned);
+	spin_unlock(&mm_unpinned_lock);
+}
+
+static inline void del_context_from_unpinned(struct mm_struct *mm)
+{
+	if (!mm->context.pinned) {
+		spin_lock(&mm_unpinned_lock);
+		list_del(&mm->context.unpinned);
+		spin_unlock(&mm_unpinned_lock);
+	}
+}
+
+static inline int install_ldt_entry (__u32 *lp, __u32 entry_1, __u32 entry_2)
+{
+	unsigned long mach_lp = arbitrary_virt_to_machine(lp);
+
+	return HYPERVISOR_update_descriptor(mach_lp, entry_1, entry_2);
+}
+
+#endif /* __ASM_MACH_LDT_H */

Re: [PATCH] abstract out bits of ldt.c

[Chris Wright]

* Martin J. Bligh (mbligh@mbligh.org) wrote:
> Starting on the work to merge xen cleanly as a subarch.
> Introduce make_pages_readonly and make_pages_writable where appropriate 
> for Xen, defined as a no-op on other subarches. Same for 

Maybe this is a bad name, since make_pages_readonly/writable has
intutitive meaning, and then is non-inutitively a no-op (for default).

thanks,
-chris

Re: [PATCH] abstract out bits of ldt.c

[Martin J. Bligh]

--Chris Wright <chrisw@osdl.org> wrote (on Sunday, August 07, 2005 16:44:11 -0700):

> * Martin J. Bligh (mbligh@mbligh.org) wrote:
>> Starting on the work to merge xen cleanly as a subarch.
>> Introduce make_pages_readonly and make_pages_writable where appropriate 
>> for Xen, defined as a no-op on other subarches. Same for 
> 
> Maybe this is a bad name, since make_pages_readonly/writable has
> intutitive meaning, and then is non-inutitively a no-op (for default).

You're welcome to suggest something else if you want, though it would
have been easier if you'd done it the first time you saw this patch,
not now. Going through this stuff multiple times is going to get very
boring very fast.

xen_make_pages_readonly / xen_make_pages_writable ?

M.

Re: [PATCH] abstract out bits of ldt.c

[Chris Wright]

* Martin J. Bligh (mbligh@mbligh.org) wrote:
> You're welcome to suggest something else if you want, though it would
> have been easier if you'd done it the first time you saw this patch,
> not now. Going through this stuff multiple times is going to get very
> boring very fast.

Sorry, that's my fault, didn't strike me until looking at it this time.

thanks,
-chris

Re: [PATCH] abstract out bits of ldt.c

[Andrew Morton]

"Martin J. Bligh" <mbligh@mbligh.org> wrote:
>
> 
> 
> --Chris Wright <chrisw@osdl.org> wrote (on Sunday, August 07, 2005 16:44:11 -0700):
> 
> > * Martin J. Bligh (mbligh@mbligh.org) wrote:
> >> Starting on the work to merge xen cleanly as a subarch.
> >> Introduce make_pages_readonly and make_pages_writable where appropriate 
> >> for Xen, defined as a no-op on other subarches. Same for 
> > 
> > Maybe this is a bad name, since make_pages_readonly/writable has
> > intutitive meaning, and then is non-inutitively a no-op (for default).
> 
> You're welcome to suggest something else if you want, though it would
> have been easier if you'd done it the first time you saw this patch,
> not now. Going through this stuff multiple times is going to get very
> boring very fast.
> 
> xen_make_pages_readonly / xen_make_pages_writable ?
> 

Well we don't want to assume "xen" at this stage.  We're faced with a
choice at present: to make the linux->hypervisor interface be some
xen-specific and xen-controlled thing, or to make it a more formal and
controlled kernel interface which talks to a generic hypervisor rather than
assuming it's Xen down there.

As long as it doesn't hamper performance or general code sanity, I think it
would be better to make this a well-defined and controlled Linux interface.
Some of the code to do that is starting to accumulate in -mm.  Everyone
needs to sit down, take a look at the patches and the proposal and work out
if this is the way to proceed.

Re: [PATCH] abstract out bits of ldt.c

[Chris Wright]

* Andrew Morton (akpm@osdl.org) wrote:
> "Martin J. Bligh" <mbligh@mbligh.org> wrote:
> > xen_make_pages_readonly / xen_make_pages_writable ?
> 
> Well we don't want to assume "xen" at this stage.  We're faced with a
> choice at present: to make the linux->hypervisor interface be some
> xen-specific and xen-controlled thing, or to make it a more formal and
> controlled kernel interface which talks to a generic hypervisor rather than
> assuming it's Xen down there.

No, definietly not.  Xen is not appropriate global namespace.  Also,
it's not about pages at this point, it's about ldt handling.

> As long as it doesn't hamper performance or general code sanity, I think it
> would be better to make this a well-defined and controlled Linux interface.
> Some of the code to do that is starting to accumulate in -mm.  Everyone
> needs to sit down, take a look at the patches and the proposal and work out
> if this is the way to proceed.

We're doing that, but it's splintered and coming in from different angles.
It'd be better to get the story straight then submit patches, IMO.

thanks,
-chris

Re: [PATCH] abstract out bits of ldt.c

[Zachary Amsden]

Chris Wright wrote:

>* Andrew Morton (akpm@osdl.org) wrote:
>  
>
>>"Martin J. Bligh" <mbligh@mbligh.org> wrote:
>>    
>>
>>>xen_make_pages_readonly / xen_make_pages_writable ?
>>>      
>>>
>>Well we don't want to assume "xen" at this stage.  We're faced with a
>>choice at present: to make the linux->hypervisor interface be some
>>xen-specific and xen-controlled thing, or to make it a more formal and
>>controlled kernel interface which talks to a generic hypervisor rather than
>>assuming it's Xen down there.
>>    
>>
>
>No, definietly not.  Xen is not appropriate global namespace.  Also,
>it's not about pages at this point, it's about ldt handling.
>  
>

I like these patches.  They greatly simplify a lot of the work I had 
anticipated was necessary for Xen.  I.e. - LDT / GDT accessors are not 
needed for most updates, only updates to live descriptor table entries 
(for GDT this is TLS, LDT, TSS?, entries and there is 1 LDT update case).

BTW, Martin, did you see my ldt-accessors patch?  It also encapsulates 
that 1 LDT update case you show here, just named differently.

Yours:

+static inline int install_ldt_entry (__u32 *lp, __u32 entry_1, __u32 entry_2)
+{
+	*lp     = entry_1;
+	*(lp+1) = entry_2;
+	return 0;
+}

Mine:

+static inline void write_ldt_entry(void *ldt, int entry, __u32 entry_a, __u32 entry_b)
+{
+	__u32 *lp = (__u32 *)((char *)ldt + entry*8);
+	*lp = entry_a;
+	*(lp+1) = entry_b;
+}


They both work, but mine does not assume page aligned LDTs (necessary to 
extract entry number).  This is moot right now because LDTs are page 
aligned anyway in Linux.  I actually don't care which one we use, but it 
might be even nicer if we got one with C type checking (struct 
desc_struct for ldt).

Does Xen assume page aligned descriptor tables?  I assume from this 
patch and snippets I have gathered from others, that is a yes, and other 
things here imply that DT pages are not shadowed.  If so, Xen itself 
must have live segments in the GDT pages, so how do you allocate space 
for the per-CPU GDT pages on SMP?


>>As long as it doesn't hamper performance or general code sanity, I think it
>>would be better to make this a well-defined and controlled Linux interface.
>>Some of the code to do that is starting to accumulate in -mm.  Everyone
>>needs to sit down, take a look at the patches and the proposal and work out
>>if this is the way to proceed.
>>    
>>
>
>We're doing that, but it's splintered and coming in from different angles.
>It'd be better to get the story straight then submit patches, IMO.
>  
>

I think introducing mach-xen headers is a bit premature though - lets 
get the interface nailed down first so that the hypervisor developers 
have time to settle the include/asm-i386/mach-xxx files without dealing 
unneeded churn onto the maintainers.

Zach

Re: [PATCH] abstract out bits of ldt.c

[Chris Wright]

* Zachary Amsden (zach@vmware.com) wrote:
> Does Xen assume page aligned descriptor tables?  I assume from this 

Yes.

> patch and snippets I have gathered from others, that is a yes, and other 
> things here imply that DT pages are not shadowed.  If so, Xen itself 
> must have live segments in the GDT pages, so how do you allocate space 
> for the per-CPU GDT pages on SMP?

early during boot.


> I think introducing mach-xen headers is a bit premature though - lets 
> get the interface nailed down first so that the hypervisor developers 
> have time to settle the include/asm-i386/mach-xxx files without dealing 
> unneeded churn onto the maintainers.

I don't think there's any point in putting mach-xen stuff in either.
Can we please discuss these patches in some sane way (say on the
virutalization list) so we're not cross-firing at each other.  It's just
going to be too messy otherwiswe.

thanks,
-chris

Re: [PATCH] abstract out bits of ldt.c

[Zachary Amsden]

Chris Wright wrote:

>* Zachary Amsden (zach@vmware.com) wrote:
>  
>
>>Does Xen assume page aligned descriptor tables?  I assume from this 
>>    
>>
>
>Yes.
>
>  
>
>>patch and snippets I have gathered from others, that is a yes, and other 
>>things here imply that DT pages are not shadowed.  If so, Xen itself 
>>must have live segments in the GDT pages, so how do you allocate space 
>>for the per-CPU GDT pages on SMP?
>>    
>>
>
>early during boot.
>  
>

Doesn't that require 16 pages per CPU?  That seems excessive to impose 
on a native build.  Perhaps we could get away with 1 page per CPU for 
the GDT on native boots and bump that up to 16 if compiling for a 
virtualized sub-architecture - i.e. move GDT to a page aligned struct 
for native (doesn't cost too much), and give it MACH_GDT_PAGES of space 
which is defined by the sub-architecture.

Let's take this thread over to virtualization@lists.osdl.org as well.

Zach

Re: [PATCH] abstract out bits of ldt.c

[Chris Wright]

* Zachary Amsden (zach@vmware.com) wrote:
> Doesn't that require 16 pages per CPU?  That seems excessive to impose 
> on a native build.  Perhaps we could get away with 1 page per CPU for 
> the GDT on native boots and bump that up to 16 if compiling for a 
> virtualized sub-architecture - i.e. move GDT to a page aligned struct 
> for native (doesn't cost too much), and give it MACH_GDT_PAGES of space 
> which is defined by the sub-architecture.

For Xen, the gdt is one page per cpu (so it's not one page per table
entry).

thanks,
-chris

Re: [PATCH] abstract out bits of ldt.c

[Martin J. Bligh]

> I like these patches.  They greatly simplify a lot of the work I 
> had anticipated was necessary for Xen.  I.e. - LDT / GDT accessors 
> are not needed for most updates, only updates to live descriptor 
> table entries (for GDT this is TLS, LDT, TSS?, entries and there 
> is 1 LDT update case).

I'm just trying to get rid of as much code duplication as possible.

> BTW, Martin, did you see my ldt-accessors patch?  It also 
> encapsulates that 1 LDT update case you show here, just named 
> differently.

I was focussing on creating a whole Xen stack before looking at
your stuff much, then seeing what was common between them, as I
think it's a bit hard to read the current Xen code because of the
copied files. Unfortunately, is going to be harder then I thought
to maintain that stack out of tree, so I wanted to shovel out
the basic refactoring stuff. Then the line got a bit blurred.
Humpf. And this is the easy part. Damn it.

Doing the whole thing and comparing is going to be a total PITA.
Perhaps the right thing to do is go one file at a time, and sync
up on that basis.

> Yours:
> 
> +static inline int install_ldt_entry (__u32 *lp, __u32 entry_1, __u32 entry_2)
> +{
> +	*lp     = entry_1;
> +	*(lp+1) = entry_2;
> +	return 0;
> +}
> 
> Mine:
> 
> +static inline void write_ldt_entry(void *ldt, int entry, __u32 entry_a, __u32 entry_b)
> +{
> +	__u32 *lp = (__u32 *)((char *)ldt + entry*8);
> +	*lp = entry_a;
> +	*(lp+1) = entry_b;
> +}
> 
> 
> They both work, but mine does not assume page aligned LDTs (necessary 
> to extract entry number).  This is moot right now because LDTs are 
> page aligned anyway in Linux.  I actually don't care which one we 
> use, but it might be even nicer if we got one with C type checking 
> (struct desc_struct for ldt).

Heh, is similar, considering we're working from completely different
angles. I'm just refactoring the current code without changing it
too much at first, we can make it more robust later. otherwise it's
going to be a pig to review if we mix those up.

> I think introducing mach-xen headers is a bit premature though - lets 
> get the interface nailed down first so that the hypervisor developers 
> have time to settle the include/asm-i386/mach-xxx files without 
> dealing unneeded churn onto the maintainers.

I can easily leave those bits out. There's going to be lots of bits common
with std i386, and bits that are common amongst the hypervisor layers,
then bits that are specific. Hopefully more bits that are common, but
still.

Humpf. I shall go back into my corner and have a rethink. Will read through
your patches some more, then send you some email.

M.

Re: [PATCH] abstract out bits of ldt.c

[Martin J. Bligh]

--Andrew Morton <akpm@osdl.org> wrote (on Sunday, August 07, 2005 17:41:29 -0700):

> "Martin J. Bligh" <mbligh@mbligh.org> wrote:
>> 
>> 
>> 
>> --Chris Wright <chrisw@osdl.org> wrote (on Sunday, August 07, 2005 16:44:11 -0700):
>> 
>> > * Martin J. Bligh (mbligh@mbligh.org) wrote:
>> >> Starting on the work to merge xen cleanly as a subarch.
>> >> Introduce make_pages_readonly and make_pages_writable where appropriate 
>> >> for Xen, defined as a no-op on other subarches. Same for 
>> > 
>> > Maybe this is a bad name, since make_pages_readonly/writable has
>> > intutitive meaning, and then is non-inutitively a no-op (for default).
>> 
>> You're welcome to suggest something else if you want, though it would
>> have been easier if you'd done it the first time you saw this patch,
>> not now. Going through this stuff multiple times is going to get very
>> boring very fast.
>> 
>> xen_make_pages_readonly / xen_make_pages_writable ?
>> 
> 
> Well we don't want to assume "xen" at this stage.  We're faced with a
> choice at present: to make the linux->hypervisor interface be some
> xen-specific and xen-controlled thing, or to make it a more formal and
> controlled kernel interface which talks to a generic hypervisor rather than
> assuming it's Xen down there.

Yes, more generic than xen would be better. I think mach-xen is probably
OK for now, but I agree we should avoid wedging xen_* in generic code
callouts. What I'm trying to do right now is rip the whole duplicated
files out of the xen patches.

> As long as it doesn't hamper performance or general code sanity, I think it
> would be better to make this a well-defined and controlled Linux interface.
> Some of the code to do that is starting to accumulate in -mm.  Everyone
> needs to sit down, take a look at the patches and the proposal and work out
> if this is the way to proceed.

We're faced with a difficult choice - the xen patches are hard to read
without refactoring them. On the other hand it's going to be very painful
to keep them in sync out of tree whilst doing the refactoring. I'd prefer
to merge up some bits as we go, though maybe I should keep those more
neutral than this. Sigh ... this is going to be extremely painful whatever
we do.

M.

Re: [PATCH] abstract out bits of ldt.c

[Ingo Molnar]

* Andrew Morton <akpm@osdl.org> wrote:

> > Furthermore, why should we hamper Xen by going to _any_ sort of 
> > "formalized" hypervisor API, when we dont even know what we want, as Xen 
> > is pretty much work in progress? And whatever Xen support is exported 
> > from the kernel, it should be a _GPL symbol export. We dont want to tie 
> > down things like pagetable format or access methods. It's a very much 
> > kernel-internal thing.
> 
> Well that's the other side of the coin.  I told the vmware guys to GPL 
> their hypervisor to make this issue go away, but that hasn't happened 
> yet ;)
> 
> I think we need to deliberately deal with all this purely at the 
> technical level and to carefully set aside thoughts such as the above.  
> Others would disagree with that.

mine are mostly technical arguments. I just also wanted to vent away 
this slowly gathering false notion of building 'interoperability', while 
the only apparent goal seems to be to maximize benefits to the closed 
hypervisors, while allowing them to not open up their code.

just grep the historic Linux changelogs for contributions from the most 
prominent of these closed-source hypervisors. It's quite close to zero.  
That's what Linux wins from this one-way relationship. Now the action 
was forced by Xen, and it sounds so hypocritical to me to talk about 
'interoperability'. Once the goal of having the APIs to pull even with 
Xen has been achieved, that prominent closed-source hypervisor vendor 
could just as easily go back into 'take from Linux'-only mode, as 
they've done for so many years.

> At the technical level, I do think that the kernel->hypervisor 
> interface is a brand new and *major* kernel interface.  As such, it 
> simply seems good design to put some thought and some formality into 
> it.  If that interface suits more than one paravirtualised hypervisor 
> implementation then that's a sign that it has succeeded.

it just wont be done right first time around. It will be like with all 
the other APIs we had: they went through dozens of major iterations, and 
will go through iterations as the hardware changes and things get 
cleaned up.

And yes, i do believe the Xen hypervisor should eventually live within 
Linux itself. (i.e. Linux should be extended to offer hypervisor 
services. This would enable to share drivers and technologies, etc.) No 
specifications, no formal agreements necessary - just hack away and 
things will be sorted out as they happen.

and no, it's not like with syscalls, for a number of reasons - 
hypervisor/OS-kernel integration is a brand-new thing (at least in the 
OSS space).

> The other design approach is to make this interface purely some 
> xen-private thing which the Xen guys maintain and the rest of us don't 
> worry about much.  That's also a legitimate approach, but my current 
> thinking is that it's technically not as good.  Plus other hypervisor 
> development teams may come along and have to graft their stuff onto 
> the xen-interface-of-the-day, which isn't good.

having a robust API never hurts, no doubt about that - and Xen has 
pretty good hypervisor APIs to begin with. But there is no connection 
between formality and a technological sound API - you can have a crap 
formal API just as much as you can have a good non-formal API. (e.g. the 
current Linux device driver APIs are good non-formal APIs)

in fact, history has shown that formality often _hurts_ the 
technological soundness of an API, mostly due to the inflebility and the 
inherent lag inserted between design and implementation.

Formalizing an API if Xen asks for it is OK in my book, but formalizing 
an API mostly for the sake of bin-only virtualization, under the 
pretense of 'interoperability' sounds pretty backwards to me.

	Ingo

Re: [PATCH] abstract out bits of ldt.c

[Christoph Hellwig]

On Tue, Aug 09, 2005 at 11:23:18AM +0200, Ingo Molnar wrote:
> mine are mostly technical arguments. I just also wanted to vent away 
> this slowly gathering false notion of building 'interoperability', while 
> the only apparent goal seems to be to maximize benefits to the closed 
> hypervisors, while allowing them to not open up their code.

...

I agree completely with the points you make here, but can't find the mail
you are replying to.  Where is it coming from?

Re: [PATCH] abstract out bits of ldt.c

[Dave Hansen]

On Sun, 2005-08-07 at 16:20 -0700, Martin J. Bligh wrote: 
> Starting on the work to merge xen cleanly as a subarch.
> Introduce make_pages_readonly and make_pages_writable where appropriate 
> for Xen, defined as a no-op on other subarches. Same for 
> add_context_to_unpinned and del_context_from_unpinned.
> Abstract out install_ldt_entry()
... 
>  		cpumask_t mask;
>  		preempt_disable();
> +		make_pages_readonly(pc->ldt, (pc->size * LDT_ENTRY_SIZE) / 
> +								PAGE_SIZE);
>  		load_LDT(pc);
>  		mask = cpumask_of_cpu(smp_processor_id());
>  		if (!cpus_equal(current->mm->cpu_vm_mask, mask))
>  			smp_call_function(flush_ldt, NULL, 1, 1);
>  		preempt_enable();
>  #else
> +		make_pages_readonly(pc->ldt, (pc->size * LDT_ENTRY_SIZE) / 
> +								PAGE_SIZE);

You do that (size * LDT_ENTRY_SIZE / PAGE_SIZE) operation an awfully
large number of times.  Could you consider introducing a little helper,
say ldt_size_pages()?  

Or, could you have a helper like make_ldt_readonly()?  You don't have to
export it, just use it in that one file.

> This will do have no effect whatsover on platforms other than xen.
...
> +       memset(&mm->context, 0, sizeof(mm->context));
>         init_MUTEX(&mm->context.sem);
> -       mm->context.size = 0;

Could you please explain what this is for?  It doesn't appear to be part
of the abstraction.  

Every call path I can see to init_new_context() is immediately preceded
by mm_alloc(), which memsets the entire mm.  The context is a direct
member of mm_struct, and should be zeroed along with the mm_alloc()
memset.  So, this seems a bit superfluous.

In any future patches that you might post, please do one thing per
patch, it makes them much easier to audit.

-- Dave