[PATCH] adjust/fix LDT handling for Xen

[PATCH] adjust/fix LDT handling for Xen

[Jan Beulich]

Don't rely on kmalloc(PAGE_SIZE) returning PAGE_SIZE aligned memory
(Xen requires GDT *and* LDT to be page-aligned). Using the page
allocator interface also removes the (albeit small) slab allocator
overhead. The same change being done for 64-bits for consistency.

Further, the Xen hypercall interface expects the LDT address to be
virtual, not machine.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Ingo Molnar <mingo@elte.hu>

---
 arch/x86/kernel/ldt_32.c |    7 +++----
 arch/x86/kernel/ldt_64.c |    7 +++----
 arch/x86/xen/enlighten.c |    9 +--------
 3 files changed, 7 insertions(+), 16 deletions(-)

--- linux-2.6.24-rc7/arch/x86/kernel/ldt_32.c	2008-01-10 16:53:54.000000000 +0100
+++ 2.6.24-rc7-x86-xen-ldt/arch/x86/kernel/ldt_32.c	2008-01-09 13:59:50.000000000 +0100
@@ -9,7 +9,6 @@
 #include <linux/mm.h>
 #include <linux/smp.h>
 #include <linux/vmalloc.h>
-#include <linux/slab.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>
@@ -38,7 +37,7 @@ static int alloc_ldt(mm_context_t *pc, i
 	if (mincount*LDT_ENTRY_SIZE > PAGE_SIZE)
 		newldt = vmalloc(mincount*LDT_ENTRY_SIZE);
 	else
-		newldt = kmalloc(mincount*LDT_ENTRY_SIZE, GFP_KERNEL);
+		newldt = (void *)__get_free_page(GFP_KERNEL);
 
 	if (!newldt)
 		return -ENOMEM;
@@ -69,7 +68,7 @@ static int alloc_ldt(mm_context_t *pc, i
 		if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)
 			vfree(oldldt);
 		else
-			kfree(oldldt);
+			put_page(virt_to_page(oldldt));
 	}
 	return 0;
 }
@@ -114,7 +113,7 @@ void destroy_context(struct mm_struct *m
 		if (mm->context.size*LDT_ENTRY_SIZE > PAGE_SIZE)
 			vfree(mm->context.ldt);
 		else
-			kfree(mm->context.ldt);
+			put_page(virt_to_page(mm->context.ldt));
 		mm->context.size = 0;
 	}
 }
--- linux-2.6.24-rc7/arch/x86/kernel/ldt_64.c	2008-01-10 16:53:54.000000000 +0100
+++ 2.6.24-rc7-x86-xen-ldt/arch/x86/kernel/ldt_64.c	2008-01-09 13:59:50.000000000 +0100
@@ -12,7 +12,6 @@
 #include <linux/mm.h>
 #include <linux/smp.h>
 #include <linux/vmalloc.h>
-#include <linux/slab.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>
@@ -41,7 +40,7 @@ static int alloc_ldt(mm_context_t *pc, u
 	if (mincount*LDT_ENTRY_SIZE > PAGE_SIZE)
 		newldt = vmalloc(mincount*LDT_ENTRY_SIZE);
 	else
-		newldt = kmalloc(mincount*LDT_ENTRY_SIZE, GFP_KERNEL);
+		newldt = (void *)__get_free_page(GFP_KERNEL);
 
 	if (!newldt)
 		return -ENOMEM;
@@ -73,7 +72,7 @@ static int alloc_ldt(mm_context_t *pc, u
 		if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)
 			vfree(oldldt);
 		else
-			kfree(oldldt);
+			put_page(virt_to_page(oldldt));
 	}
 	return 0;
 }
@@ -117,7 +116,7 @@ void destroy_context(struct mm_struct *m
 		if ((unsigned)mm->context.size*LDT_ENTRY_SIZE > PAGE_SIZE)
 			vfree(mm->context.ldt);
 		else
-			kfree(mm->context.ldt);
+			put_page(virt_to_page(mm->context.ldt));
 		mm->context.size = 0;
 	}
 }
--- linux-2.6.24-rc7/arch/x86/xen/enlighten.c	2008-01-10 16:53:55.000000000 +0100
+++ 2.6.24-rc7-x86-xen-ldt/arch/x86/xen/enlighten.c	2008-01-09 14:00:51.000000000 +0100
@@ -275,19 +275,12 @@ static unsigned long xen_store_tr(void)
 
 static void xen_set_ldt(const void *addr, unsigned entries)
 {
-	unsigned long linear_addr = (unsigned long)addr;
 	struct mmuext_op *op;
 	struct multicall_space mcs = xen_mc_entry(sizeof(*op));
 
 	op = mcs.args;
 	op->cmd = MMUEXT_SET_LDT;
-	if (linear_addr) {
-		/* ldt my be vmalloced, use arbitrary_virt_to_machine */
-		xmaddr_t maddr;
-		maddr = arbitrary_virt_to_machine((unsigned long)addr);
-		linear_addr = (unsigned long)maddr.maddr;
-	}
-	op->arg1.linear_addr = linear_addr;
+	op->arg1.linear_addr = (unsigned long)addr;
 	op->arg2.nr_ents = entries;
 
 	MULTI_mmuext_op(mcs.mc, op, 1, NULL, DOMID_SELF);

Re: [PATCH] adjust/fix LDT handling for Xen

[Jiri Slaby]

On 01/11/2008 10:22 AM, Jan Beulich wrote:
> Don't rely on kmalloc(PAGE_SIZE) returning PAGE_SIZE aligned memory
> (Xen requires GDT *and* LDT to be page-aligned). Using the page
> allocator interface also removes the (albeit small) slab allocator
> overhead. The same change being done for 64-bits for consistency.
> 
> Further, the Xen hypercall interface expects the LDT address to be
> virtual, not machine.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> 
> ---
>  arch/x86/kernel/ldt_32.c |    7 +++----
>  arch/x86/kernel/ldt_64.c |    7 +++----
>  arch/x86/xen/enlighten.c |    9 +--------
>  3 files changed, 7 insertions(+), 16 deletions(-)
> 
> --- linux-2.6.24-rc7/arch/x86/kernel/ldt_32.c	2008-01-10 16:53:54.000000000 +0100
> +++ 2.6.24-rc7-x86-xen-ldt/arch/x86/kernel/ldt_32.c	2008-01-09 13:59:50.000000000 +0100
[...]
> @@ -73,7 +72,7 @@ static int alloc_ldt(mm_context_t *pc, u
>  		if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)
>  			vfree(oldldt);
>  		else
> -			kfree(oldldt);
> +			put_page(virt_to_page(oldldt));
>  	}
>  	return 0;
>  }
> @@ -117,7 +116,7 @@ void destroy_context(struct mm_struct *m
>  		if ((unsigned)mm->context.size*LDT_ENTRY_SIZE > PAGE_SIZE)
>  			vfree(mm->context.ldt);
>  		else
> -			kfree(mm->context.ldt);
> +			put_page(virt_to_page(mm->context.ldt));

why not free_page() with all those checks if it is correct virt address which it 
brings?

regards,
-- 
Jiri Slaby
Faculty of Informatics, Masaryk University
Suse Labs

Re: [PATCH] adjust/fix LDT handling for Xen

[Jan Beulich]

>> --- linux-2.6.24-rc7/arch/x86/kernel/ldt_32.c	2008-01-10 16:53:54.000000000 +0100
>> +++ 2.6.24-rc7-x86-xen-ldt/arch/x86/kernel/ldt_32.c	2008-01-09 13:59:50.000000000 +0100
>[...]
>> @@ -73,7 +72,7 @@ static int alloc_ldt(mm_context_t *pc, u
>>  		if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)
>>  			vfree(oldldt);
>>  		else
>> -			kfree(oldldt);
>> +			put_page(virt_to_page(oldldt));
>>  	}
>>  	return 0;
>>  }
>> @@ -117,7 +116,7 @@ void destroy_context(struct mm_struct *m
>>  		if ((unsigned)mm->context.size*LDT_ENTRY_SIZE > PAGE_SIZE)
>>  			vfree(mm->context.ldt);
>>  		else
>> -			kfree(mm->context.ldt);
>> +			put_page(virt_to_page(mm->context.ldt));
>
>why not free_page() with all those checks if it is correct virt address which it 
>brings?

I followed the combination I found elsewhere - __get_free_page/put_page
or alloc_page/free_page. I admit that I'm unclear about when/why to use
one vs. the other pair.

Jan

Re: [PATCH] adjust/fix LDT handling for Xen

[Jeremy Fitzhardinge]

Jan Beulich wrote:
> Don't rely on kmalloc(PAGE_SIZE) returning PAGE_SIZE aligned memory
> (Xen requires GDT *and* LDT to be page-aligned).

Can kmalloc return non-page-aligned PAGE_SIZE allocations?

>  Using the page
> allocator interface also removes the (albeit small) slab allocator
> overhead.

Runtime or space overhead?  Given that they're once-off allocations, the 
time part isn't a big factor.  And apparently LDT is completely unused.

Seems like a reasonable change either way.

>  The same change being done for 64-bits for consistency.
>
> Further, the Xen hypercall interface expects the LDT address to be
> virtual, not machine.
>   

Shows how much the LDT gets used then...  Hm, but I didn't make that 
code up; I wonder where it came from?  But certainly, passing the mfn of 
the first page of a vmalloc area doesn't make any sense.

> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Ingo Molnar <mingo@elte.hu>
>   
Acked-by: Jeremy Fitzhardinge <jeremy@xensource.com>

> ---
>  arch/x86/kernel/ldt_32.c |    7 +++----
>  arch/x86/kernel/ldt_64.c |    7 +++----
>  arch/x86/xen/enlighten.c |    9 +--------
>  3 files changed, 7 insertions(+), 16 deletions(-)
>
> --- linux-2.6.24-rc7/arch/x86/kernel/ldt_32.c	2008-01-10 16:53:54.000000000 +0100
> +++ 2.6.24-rc7-x86-xen-ldt/arch/x86/kernel/ldt_32.c	2008-01-09 13:59:50.000000000 +0100
> @@ -9,7 +9,6 @@
>  #include <linux/mm.h>
>  #include <linux/smp.h>
>  #include <linux/vmalloc.h>
> -#include <linux/slab.h>
>  
>  #include <asm/uaccess.h>
>  #include <asm/system.h>
> @@ -38,7 +37,7 @@ static int alloc_ldt(mm_context_t *pc, i
>  	if (mincount*LDT_ENTRY_SIZE > PAGE_SIZE)
>  		newldt = vmalloc(mincount*LDT_ENTRY_SIZE);
>  	else
> -		newldt = kmalloc(mincount*LDT_ENTRY_SIZE, GFP_KERNEL);
> +		newldt = (void *)__get_free_page(GFP_KERNEL);
>  
>  	if (!newldt)
>  		return -ENOMEM;
> @@ -69,7 +68,7 @@ static int alloc_ldt(mm_context_t *pc, i
>  		if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)
>  			vfree(oldldt);
>  		else
> -			kfree(oldldt);
> +			put_page(virt_to_page(oldldt));
>  	}
>  	return 0;
>  }
> @@ -114,7 +113,7 @@ void destroy_context(struct mm_struct *m
>  		if (mm->context.size*LDT_ENTRY_SIZE > PAGE_SIZE)
>  			vfree(mm->context.ldt);
>  		else
> -			kfree(mm->context.ldt);
> +			put_page(virt_to_page(mm->context.ldt));
>  		mm->context.size = 0;
>  	}
>  }
> --- linux-2.6.24-rc7/arch/x86/kernel/ldt_64.c	2008-01-10 16:53:54.000000000 +0100
> +++ 2.6.24-rc7-x86-xen-ldt/arch/x86/kernel/ldt_64.c	2008-01-09 13:59:50.000000000 +0100
> @@ -12,7 +12,6 @@
>  #include <linux/mm.h>
>  #include <linux/smp.h>
>  #include <linux/vmalloc.h>
> -#include <linux/slab.h>
>  
>  #include <asm/uaccess.h>
>  #include <asm/system.h>
> @@ -41,7 +40,7 @@ static int alloc_ldt(mm_context_t *pc, u
>  	if (mincount*LDT_ENTRY_SIZE > PAGE_SIZE)
>  		newldt = vmalloc(mincount*LDT_ENTRY_SIZE);
>  	else
> -		newldt = kmalloc(mincount*LDT_ENTRY_SIZE, GFP_KERNEL);
> +		newldt = (void *)__get_free_page(GFP_KERNEL);
>  
>  	if (!newldt)
>  		return -ENOMEM;
> @@ -73,7 +72,7 @@ static int alloc_ldt(mm_context_t *pc, u
>  		if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)
>  			vfree(oldldt);
>  		else
> -			kfree(oldldt);
> +			put_page(virt_to_page(oldldt));
>  	}
>  	return 0;
>  }
> @@ -117,7 +116,7 @@ void destroy_context(struct mm_struct *m
>  		if ((unsigned)mm->context.size*LDT_ENTRY_SIZE > PAGE_SIZE)
>  			vfree(mm->context.ldt);
>  		else
> -			kfree(mm->context.ldt);
> +			put_page(virt_to_page(mm->context.ldt));
>  		mm->context.size = 0;
>  	}
>  }
> --- linux-2.6.24-rc7/arch/x86/xen/enlighten.c	2008-01-10 16:53:55.000000000 +0100
> +++ 2.6.24-rc7-x86-xen-ldt/arch/x86/xen/enlighten.c	2008-01-09 14:00:51.000000000 +0100
> @@ -275,19 +275,12 @@ static unsigned long xen_store_tr(void)
>  
>  static void xen_set_ldt(const void *addr, unsigned entries)
>  {
> -	unsigned long linear_addr = (unsigned long)addr;
>  	struct mmuext_op *op;
>  	struct multicall_space mcs = xen_mc_entry(sizeof(*op));
>  
>  	op = mcs.args;
>  	op->cmd = MMUEXT_SET_LDT;
> -	if (linear_addr) {
> -		/* ldt my be vmalloced, use arbitrary_virt_to_machine */
> -		xmaddr_t maddr;
> -		maddr = arbitrary_virt_to_machine((unsigned long)addr);
> -		linear_addr = (unsigned long)maddr.maddr;
> -	}
> -	op->arg1.linear_addr = linear_addr;
> +	op->arg1.linear_addr = (unsigned long)addr;
>  	op->arg2.nr_ents = entries;
>  
>  	MULTI_mmuext_op(mcs.mc, op, 1, NULL, DOMID_SELF);
>
>
>
>   

Re: [PATCH] adjust/fix LDT handling for Xen

[Jan Beulich]

>>> Jeremy Fitzhardinge <Jeremy.Fitzhardinge@citrix.com> 11.01.08 18:28 >>>
>Jan Beulich wrote:
>> Don't rely on kmalloc(PAGE_SIZE) returning PAGE_SIZE aligned memory
>> (Xen requires GDT *and* LDT to be page-aligned).
>
>Can kmalloc return non-page-aligned PAGE_SIZE allocations?

Documentation says it's to return pointer-size aligned memory - any excess
alignment is therefore an implementation detail. (Nevertheless, afaics all
current allocators generate page-aligned chunks.)

>>  Using the page
>> allocator interface also removes the (albeit small) slab allocator
>> overhead.
>
>Runtime or space overhead?  Given that they're once-off allocations, the 
>time part isn't a big factor.  And apparently LDT is completely unused.

Both, but as I also said the saving is small.

Jan

Re: [PATCH] adjust/fix LDT handling for Xen

[Ingo Molnar]

* Jan Beulich <jbeulich@novell.com> wrote:

> Don't rely on kmalloc(PAGE_SIZE) returning PAGE_SIZE aligned memory 
> (Xen requires GDT *and* LDT to be page-aligned). Using the page 
> allocator interface also removes the (albeit small) slab allocator 
> overhead. The same change being done for 64-bits for consistency.
> 
> Further, the Xen hypercall interface expects the LDT address to be 
> virtual, not machine.

coul dyou try this against x86.git as well? We already unified ldt.c as 
part of the paravirt patches.

	Ingo

Re: [PATCH] adjust/fix LDT handling for Xen

[Jan Beulich]

>>> Ingo Molnar <mingo@elte.hu> 14.01.08 16:48 >>>
>
>* Jan Beulich <jbeulich@novell.com> wrote:
>
>> Don't rely on kmalloc(PAGE_SIZE) returning PAGE_SIZE aligned memory 
>> (Xen requires GDT *and* LDT to be page-aligned). Using the page 
>> allocator interface also removes the (albeit small) slab allocator 
>> overhead. The same change being done for 64-bits for consistency.
>> 
>> Further, the Xen hypercall interface expects the LDT address to be 
>> virtual, not machine.
>
>coul dyou try this against x86.git as well? We already unified ldt.c as 
>part of the paravirt patches.

Sure, but it may take me a little while.

Jan

Re: [PATCH] adjust/fix LDT handling for Xen

[Jeremy Fitzhardinge]

Jan Beulich wrote:
>>>> Ingo Molnar <mingo@elte.hu> 14.01.08 16:48 >>>
>>>>         
>> * Jan Beulich <jbeulich@novell.com> wrote:
>>
>>     
>>> Don't rely on kmalloc(PAGE_SIZE) returning PAGE_SIZE aligned memory 
>>> (Xen requires GDT *and* LDT to be page-aligned). Using the page 
>>> allocator interface also removes the (albeit small) slab allocator 
>>> overhead. The same change being done for 64-bits for consistency.
>>>
>>> Further, the Xen hypercall interface expects the LDT address to be 
>>> virtual, not machine.
>>>       
>> coul dyou try this against x86.git as well? We already unified ldt.c as 
>> part of the paravirt patches.
>>     
>
> Sure, but it may take me a little while.

I've got the unified version here. 

Subject: xen: adjust/fix LDT handling for Xen

From: "Jan Beulich" <jbeulich@novell.com>

Don't rely on kmalloc(PAGE_SIZE) returning PAGE_SIZE aligned memory
(Xen requires GDT *and* LDT to be page-aligned). Using the page
allocator interface also removes the (albeit small) slab allocator
overhead. The same change being done for 64-bits for consistency.

Further, the Xen hypercall interface expects the LDT address to be
virtual, not machine.

[ Adjusted to unified ldt.c - Jeremy ]

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Acked-by: Jeremy Fitzhardinge <jeremy@xensource.com>
Cc: Ingo Molnar <mingo@elte.hu>

---
 arch/x86/kernel/ldt.c    |    7 +++----
 arch/x86/xen/enlighten.c |    9 +--------
 2 files changed, 4 insertions(+), 12 deletions(-)

===================================================================
--- a/arch/x86/kernel/ldt.c
+++ b/arch/x86/kernel/ldt.c
@@ -12,7 +12,6 @@
 #include <linux/mm.h>
 #include <linux/smp.h>
 #include <linux/vmalloc.h>
-#include <linux/slab.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>
@@ -40,7 +39,7 @@ static int alloc_ldt(mm_context_t *pc, i
 	if (mincount * LDT_ENTRY_SIZE > PAGE_SIZE)
 		newldt = vmalloc(mincount * LDT_ENTRY_SIZE);
 	else
-		newldt = kmalloc(mincount * LDT_ENTRY_SIZE, GFP_KERNEL);
+		newldt = (void *)__get_free_page(GFP_KERNEL);
 
 	if (!newldt)
 		return -ENOMEM;
@@ -78,7 +77,7 @@ static int alloc_ldt(mm_context_t *pc, i
 		if (oldsize * LDT_ENTRY_SIZE > PAGE_SIZE)
 			vfree(oldldt);
 		else
-			kfree(oldldt);
+			put_page(virt_to_page(oldldt));
 	}
 	return 0;
 }
@@ -129,7 +128,7 @@ void destroy_context(struct mm_struct *m
 		if (mm->context.size * LDT_ENTRY_SIZE > PAGE_SIZE)
 			vfree(mm->context.ldt);
 		else
-			kfree(mm->context.ldt);
+			put_page(virt_to_page(mm->context.ldt));
 		mm->context.size = 0;
 	}
 }
===================================================================
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -275,19 +275,12 @@ static unsigned long xen_store_tr(void)
 
 static void xen_set_ldt(const void *addr, unsigned entries)
 {
-	unsigned long linear_addr = (unsigned long)addr;
 	struct mmuext_op *op;
 	struct multicall_space mcs = xen_mc_entry(sizeof(*op));
 
 	op = mcs.args;
 	op->cmd = MMUEXT_SET_LDT;
-	if (linear_addr) {
-		/* ldt my be vmalloced, use arbitrary_virt_to_machine */
-		xmaddr_t maddr;
-		maddr = arbitrary_virt_to_machine((unsigned long)addr);
-		linear_addr = (unsigned long)maddr.maddr;
-	}
-	op->arg1.linear_addr = linear_addr;
+	op->arg1.linear_addr = (unsigned long)addr;
 	op->arg2.nr_ents = entries;
 
 	MULTI_mmuext_op(mcs.mc, op, 1, NULL, DOMID_SELF);

Re: [PATCH] adjust/fix LDT handling for Xen

[Ingo Molnar]

* Jeremy Fitzhardinge <Jeremy.Fitzhardinge@citrix.com> wrote:

>>> coul dyou try this against x86.git as well? We already unified ldt.c as 
>>> part of the paravirt patches.
>>>     
>>
>> Sure, but it may take me a little while.
>
> I've got the unified version here. 

thanks, applied.

> Acked-by: Jeremy Fitzhardinge <jeremy@xensource.com>

please use Signed-off-by when passing through (or modifying) patches.

	Ingo

Re: [PATCH] adjust/fix LDT handling for Xen

[Jeremy Fitzhardinge]

Ingo Molnar wrote:
> * Jeremy Fitzhardinge <Jeremy.Fitzhardinge@citrix.com> wrote:
>
>   
>>>> coul dyou try this against x86.git as well? We already unified ldt.c as 
>>>> part of the paravirt patches.
>>>>     
>>>>         
>>> Sure, but it may take me a little while.
>>>       
>> I've got the unified version here. 
>>     
>
> thanks, applied.
>
>   
>> Acked-by: Jeremy Fitzhardinge <jeremy@xensource.com>
>>     
>
> please use Signed-off-by when passing through (or modifying) patches.
>   

Will do.  That was left over from trying to apply the original version.

    J