Patchwork linux-user: correct argument number for sys_mremap and sys_splice

login
register
mail settings
Submitter Petar Jovanovic
Date July 23, 2013, 5 p.m.
Message ID <1374598810-78021-1-git-send-email-petar.jovanovic@rt-rk.com>
Download mbox | patch
Permalink /patch/261160/
State New
Headers show

Comments

Petar Jovanovic - July 23, 2013, 5 p.m.
From: Petar Jovanovic <petar.jovanovic@imgtec.com>

sys_mremap missed 5th argument (new_address), which caused examples that
remap to a specific address to fail.
sys_splice missed 5th and 6th argument which caused different examples to
fail.
This change has an effect on MIPS target only.

Signed-off-by: Petar Jovanovic <petar.jovanovic@imgtec.com>
---
 linux-user/main.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Michael Tokarev - July 23, 2013, 5:18 p.m.
23.07.2013 21:00, Petar Jovanovic wrote:
> From: Petar Jovanovic <petar.jovanovic@imgtec.com>
> 
> sys_mremap missed 5th argument (new_address), which caused examples that
> remap to a specific address to fail.
> sys_splice missed 5th and 6th argument which caused different examples to
> fail.
> This change has an effect on MIPS target only.

While splice is obvious and appears to be correct, with mremap I'm not
this sure.  The last, 5th argument of mremap(), which is `void *new_address',
is optional and may be either present or not.  So, without understanding
how the underlying tables/code works, I'm not really sure if the resulting
change will work or not.  On the other hand, sys_open is also declared as
having 3 arguments while the 3rd one (mode) is also optional, so the patch
appears to be correct there as well, so I was almost ready to apply it,
until your last comment which states that the change has only effect on
MIPS.  Which is quite puzzling to me who, again, does not really know
how the code works.

So either the patch isn't trivial enough, or maybe you can provide some
more verbose explanation... ;)

Thanks,

/mjt
Peter Maydell - July 23, 2013, 5:33 p.m.
On 23 July 2013 18:18, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 23.07.2013 21:00, Petar Jovanovic wrote:
>> From: Petar Jovanovic <petar.jovanovic@imgtec.com>
>>
>> sys_mremap missed 5th argument (new_address), which caused examples that
>> remap to a specific address to fail.
>> sys_splice missed 5th and 6th argument which caused different examples to
>> fail.
>> This change has an effect on MIPS target only.
>
> While splice is obvious and appears to be correct, with mremap I'm not
> this sure.

This table in QEMU corresponds to the table in the kernel
in arch/mips/kernel/scall32-o32.S and should generally have
the same numbers in it.

The fix to mremap corresponds to Linux kernel commit 7dbdf43c
(from 2006); the fix to splice is 08d30879 (from 2008).

> On the other hand, sys_open is also declared as having 3
> arguments while the 3rd one (mode) is also optional,

Yes. All the number-of-arguments param does is cause
the main.c code to fish the extra arguments out of the stack,
since the MIPS 32 bit ABI doesn't have enough registers for
all of them to be in registers. If the argument is optional
and isn't used in this case, it means we've just copied
garbage as the arg6/7/8 param to do_syscall(), which is
exactly the same situation as if the guest ABI put all
syscall args in registers.

> until your last comment which states that the change has only effect
> on MIPS.  Which is quite puzzling to me

It's trivially obvious that it only has effect on MIPS,
because it's patching a piece of code which is inside
an #ifdef TARGET_MIPS guard.

Petar: I think it would be a good idea if you checked
the whole of our table against a recent kernel and made
sure we're in sync for everything, rather than fixing
things one at a time.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM
Petar Jovanovic - July 24, 2013, 4:22 p.m.

Michael Tokarev - July 25, 2013, 6:23 a.m.
23.07.2013 21:33, Peter Maydell wrote:
> On 23 July 2013 18:18, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> 23.07.2013 21:00, Petar Jovanovic wrote:
>>> From: Petar Jovanovic <petar.jovanovic@imgtec.com>
>>>
>>> sys_mremap missed 5th argument (new_address), which caused examples that
>>> remap to a specific address to fail.
>>> sys_splice missed 5th and 6th argument which caused different examples to
>>> fail.
>>> This change has an effect on MIPS target only.

Thanks, applied to the trivial patches queue.

/mjt
Aurelien Jarno - July 28, 2013, 10:38 p.m.
On Tue, Jul 23, 2013 at 07:00:10PM +0200, Petar Jovanovic wrote:
> From: Petar Jovanovic <petar.jovanovic@imgtec.com>
> 
> sys_mremap missed 5th argument (new_address), which caused examples that
> remap to a specific address to fail.
> sys_splice missed 5th and 6th argument which caused different examples to
> fail.
> This change has an effect on MIPS target only.
> 
> Signed-off-by: Petar Jovanovic <petar.jovanovic@imgtec.com>
> ---
>  linux-user/main.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 99c3b3f..1c20229 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -1957,7 +1957,7 @@ static const uint8_t mips_syscall_args[] = {
>  	MIPS_SYS(sys_sched_get_priority_min, 1)
>  	MIPS_SYS(sys_sched_rr_get_interval, 2)	/* 4165 */
>  	MIPS_SYS(sys_nanosleep,	2)
> -	MIPS_SYS(sys_mremap	, 4)
> +	MIPS_SYS(sys_mremap	, 5)
>  	MIPS_SYS(sys_accept	, 3)
>  	MIPS_SYS(sys_bind	, 3)
>  	MIPS_SYS(sys_connect	, 3)	/* 4170 */
> @@ -2094,7 +2094,7 @@ static const uint8_t mips_syscall_args[] = {
>  	MIPS_SYS(sys_pselect6, 6)
>  	MIPS_SYS(sys_ppoll, 5)
>  	MIPS_SYS(sys_unshare, 1)
> -	MIPS_SYS(sys_splice, 4)
> +	MIPS_SYS(sys_splice, 6)
>  	MIPS_SYS(sys_sync_file_range, 7) /* 4305 */
>  	MIPS_SYS(sys_tee, 4)
>  	MIPS_SYS(sys_vmsplice, 4)

Thanks, applied.

Patch

diff --git a/linux-user/main.c b/linux-user/main.c
index 99c3b3f..1c20229 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -1957,7 +1957,7 @@  static const uint8_t mips_syscall_args[] = {
 	MIPS_SYS(sys_sched_get_priority_min, 1)
 	MIPS_SYS(sys_sched_rr_get_interval, 2)	/* 4165 */
 	MIPS_SYS(sys_nanosleep,	2)
-	MIPS_SYS(sys_mremap	, 4)
+	MIPS_SYS(sys_mremap	, 5)
 	MIPS_SYS(sys_accept	, 3)
 	MIPS_SYS(sys_bind	, 3)
 	MIPS_SYS(sys_connect	, 3)	/* 4170 */
@@ -2094,7 +2094,7 @@  static const uint8_t mips_syscall_args[] = {
 	MIPS_SYS(sys_pselect6, 6)
 	MIPS_SYS(sys_ppoll, 5)
 	MIPS_SYS(sys_unshare, 1)
-	MIPS_SYS(sys_splice, 4)
+	MIPS_SYS(sys_splice, 6)
 	MIPS_SYS(sys_sync_file_range, 7) /* 4305 */
 	MIPS_SYS(sys_tee, 4)
 	MIPS_SYS(sys_vmsplice, 4)