Prev: [PATCH 1/5] x86, 64-bit: Move kernel_thread to C
Next: [GIT PULL] hwmon updates for 2.6.33, round 1
From: H. Peter Anvin on 9 Dec 2009 14:00
On 12/09/2009 10:47 AM, Linus Torvalds wrote:
>
> NOTE NOTE NOTE! Totally untested. Is the second argument even in %edx? I
> don't remember, I didn't check, I'm just throwing this out as a "hey,
> maybe something _like_ this can work" patch, and will be immediately
> removing it from my machine after sending this email.
>

The second argument is in %edx, but unlike 64 bits, it is not loaded
into that register a priory ("asmlinkage" means arguments are on the stack.)

As such, we need something looking like:

#define PTREGSCALL0(name) \
ptregs_##name: \
leal 4(%esp),%eax; \
jmp sys_##name

#define PTREGSCALL1(name) \
ptregs_##name: \
movl 4(%esp),%eax; \
leal 4(%esp),%edx; \
jmp sys_##name

#define PTREGSCALL2(name) \
ptregs_##name: \
movl 4(%esp),%eax; \
movl 8(%esp),%edx; \
leal 4(%esp),%ecx; \
jmp sys_##name

If we need more than two arguments + pt_regs, then we have to set up a
temporary stack frame.

[Sorry, I'm sitting in a meeting so I can't actually write up a real patch]

-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: H. Peter Anvin on 9 Dec 2009 14:00
On 12/09/2009 10:31 AM, Jeremy Fitzhardinge wrote:
>
> How does this look?
>
> git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git fix-iopl
>
> From: Jeremy Fitzhardinge<jeremy.fitzhardinge(a)citrix.com>
> Date: Wed, 9 Dec 2009 10:26:59 -0800
> Subject: [PATCH] x86: Make sys_iopl use passed-in pt_regs
>
> Rather than using task_pt_regs, use the pt_regs * passed into the syscall.
> The ABI differences are handled in small 32/64-bit specific functions,
> and everything else is handled in the common do_iopl().
>
> Signed-off-by: Jeremy Fitzhardinge<jeremy.fitzhardinge(a)citrix.com>
>
> diff --git a/arch/x86/include/asm/syscalls.h b/arch/x86/include/asm/syscalls.h
> index 5336ce2..70497f0 100644
> --- a/arch/x86/include/asm/syscalls.h
> +++ b/arch/x86/include/asm/syscalls.h
> @@ -33,11 +33,11 @@ long sys_rt_sigreturn(struct pt_regs *);
> asmlinkage int sys_set_thread_area(struct user_desc __user *);
> asmlinkage int sys_get_thread_area(struct user_desc __user *);
>
> -/* kernel/ioport.c */
> -asmlinkage long sys_iopl(unsigned int);
> -
> /* X86_32 only */
> #ifdef CONFIG_X86_32
> +/* kernel/ioport.c */
> +asmlinkage long sys_iopl(struct pt_regs *);
> +
> /* kernel/process_32.c */

This is the main ugliness that led me to pass up on this in the first
place. What it really comes down to is that on 32 bits we need the
analogue with what 64 bits have with different stubs for different
number of arguments.

-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on 9 Dec 2009 14:10


On Wed, 9 Dec 2009, H. Peter Anvin wrote:
>
> The second argument is in %edx, but unlike 64 bits, it is not loaded
> into that register a priory ("asmlinkage" means arguments are on the stack.)

Oh, I missed the fact that we don't actually use asmlinkage on
sys_iopl() at all on x86-32 for this very reason.

And on x86-64, I think asmlinkage is a no-op, so that's ok - we should
just make the prototype be

long sys_iopl(long level, struct pt_regs *regs);

and your fancier macros.

Linus


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jeremy Fitzhardinge on 9 Dec 2009 14:40
On 12/09/09 10:47, Linus Torvalds wrote:
>
> On Wed, 9 Dec 2009, Jeremy Fitzhardinge wrote:
>
>> How does this look?
>>
> I would actually prefer it if the calling convention was just made to
> match on both x86 and x86-64. Wouldn't it be nice if they both just had
>
>
>> +/* kernel/ioport.c */
>> +asmlinkage long sys_iopl(unsigned int, struct pt_regs *);
>>
> as the prototype, and looked the same?
>
> I realize that right now the 32-bit PTREGSCALL() thing doesn't support
> that (very different macros for entry.S x86-32 and -64), but isn't that
> just another thing we should try to fix too?
>
> IOW, maybe something like this would be good, and would change the x86-32
> calling convention to match the x86-64 one?
>
> NOTE NOTE NOTE! Totally untested. Is the second argument even in %edx? I
> don't remember, I didn't check, I'm just throwing this out as a "hey,
> maybe something _like_ this can work" patch, and will be immediately
> removing it from my machine after sending this email.
>

I came up with this:

From: Jeremy Fitzhardinge<jeremy.fitzhardinge(a)citrix.com>
Date: Wed, 9 Dec 2009 11:17:52 -0800
Subject: [PATCH] x86/iopl: make 32bit iopl also get level argument

This makes it match the 64-bit prototype, and simplifies the whole thing.

Signed-off-by: Jeremy Fitzhardinge<jeremy.fitzhardinge(a)citrix.com>

diff --git a/arch/x86/include/asm/syscalls.h b/arch/x86/include/asm/syscalls.h
index 70497f0..4e567d5 100644
--- a/arch/x86/include/asm/syscalls.h
+++ b/arch/x86/include/asm/syscalls.h
@@ -18,6 +18,7 @@
/* Common in X86_32 and X86_64 */
/* kernel/ioport.c */
asmlinkage long sys_ioperm(unsigned long, unsigned long, int);
+long sys_iopl(unsigned int, struct pt_regs *);

/* kernel/process.c */
int sys_fork(struct pt_regs *);
@@ -36,7 +37,6 @@ asmlinkage int sys_get_thread_area(struct user_desc __user *);
/* X86_32 only */
#ifdef CONFIG_X86_32
/* kernel/ioport.c */
-asmlinkage long sys_iopl(struct pt_regs *);

/* kernel/process_32.c */
int sys_clone(struct pt_regs *);
@@ -71,9 +71,6 @@ int sys_vm86(struct pt_regs *);

/* X86_64 only */

-/* kernel/ioport.c */
-asmlinkage long sys_iopl(unsigned int, struct pt_regs *);
-
/* kernel/process_64.c */
asmlinkage long sys_clone(unsigned long, unsigned long,
void __user *, void __user *,
diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index 50b9c22..737b81f 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -725,13 +725,15 @@ END(syscall_badsys)
/*
* System calls that need a pt_regs pointer.
*/
-#define PTREGSCALL(name) \
+#define PTREGSCALL_ARG(name,arg) \
ALIGN; \
ptregs_##name: \
- leal 4(%esp),%eax; \
+ leal 4(%esp),arg; \
jmp sys_##name;
-
-PTREGSCALL(iopl)
+#define PTREGSCALL(name) \
+ PTREGSCALL_ARG(name, %eax)
+
+PTREGSCALL_ARG(iopl,%edx)
PTREGSCALL(fork)
PTREGSCALL(clone)
PTREGSCALL(vfork)
diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
index b1cbac5..f435e42 100644
--- a/arch/x86/kernel/ioport.c
+++ b/arch/x86/kernel/ioport.c
@@ -103,7 +103,7 @@ asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int turn_on)
* on system-call entry - see also fork() and the signal handling
* code.
*/
-static int do_iopl(unsigned int level, struct pt_regs *regs)
+long sys_iopl(unsigned int level, struct pt_regs *regs)
{
struct thread_struct *t =&current->thread;
unsigned int old = (regs->flags>> 12)& 3;
@@ -122,15 +122,3 @@ static int do_iopl(unsigned int level, struct pt_regs *regs)

return 0;
}
-
-#ifdef CONFIG_X86_64
-long sys_iopl(unsigned int level, struct pt_regs *regs)
-{
- return do_iopl(level, regs);
-}
-#else
-long sys_iopl(struct pt_regs *regs)
-{
- return do_iopl(regs->bx, regs);
-}
-#endif


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: H. Peter Anvin on 9 Dec 2009 15:10
On 12/09/2009 11:32 AM, Jeremy Fitzhardinge wrote:
> /*
> * System calls that need a pt_regs pointer.
> */
> -#define PTREGSCALL(name) \
> +#define PTREGSCALL_ARG(name,arg) \
> ALIGN; \
> ptregs_##name: \
> - leal 4(%esp),%eax; \
> + leal 4(%esp),arg; \
> jmp sys_##name;
> -

I believe this will not work for the reason outlined in the other post.

-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  | 
Pages: 1 2
Prev: [PATCH 1/5] x86, 64-bit: Move kernel_thread to C
Next: [GIT PULL] hwmon updates for 2.6.33, round 1