[Qemu-devel] [PATCH] linux-user: correct argument number for sys_mremap and sys_splice

[Qemu-devel] [PATCH] linux-user: correct argument number for sys_mremap and sys_splice

[Petar Jovanovic]

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)
-- 
1.7.9.5

Re: [Qemu-devel] [Qemu-trivial] [PATCH] linux-user: correct argument number for sys_mremap and sys_splice

[Michael Tokarev]

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

Re: [Qemu-devel] [Qemu-trivial] [PATCH] linux-user: correct argument number for sys_mremap and sys_splice

[Peter Maydell]

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

Re: [Qemu-devel] [Qemu-trivial] [PATCH] linux-user: correct argument number for sys_mremap and sys_splice

[Petar Jovanovic]

________________________________________
From: Peter Maydell [peter.maydell@linaro.org]
Sent: Tuesday, July 23, 2013 7:33 PM
To: Michael Tokarev
Cc: Petar Jovanovic; qemu-trivial@nongnu.org; riku.voipio@linaro.org; qemu-devel@nongnu.org; Petar Jovanovic
Subject: Re: [Qemu-devel] [Qemu-trivial] [PATCH] linux-user: correct argument number for sys_mremap and sys_splice

> 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.
>

I did, this is how I came up with these two system calls.

Here are all the differences:

Syscall             Qemu    Kernel

sys_acct            0    1
sys_epoll_wait      3    4
sys_fdatasync       0    1
sys_lookup_dcookie  3    4
sys_mremap          4    5
sys_quotactl        0    4
sys_sigaltstack     2    0
sys_splice          4    6
sys_waitid          4    5


So, we are not fully in sync, but the rest of it will not cause any
problems. In the existing QEMU code in cpu_loop(CPUMIPSState *env)
first four arguments are always passed, so for sys_acct, sys_epoll_wait,
sys_fdatasync, sys_quotactl, sys_lookup_dcookie and sys_sigaltstack
the differences do not matter.

For sys_splice and sys_mremap, the difference is important so I
added this change and came up with test cases.

The last on the list is sys_waitid, the difference could be important yet
it still is not, since the host part does not seem to make use of it.

Considering the turn-around times for code reviews and getting the changes
in, I am guided by "If it ain't broke, don't fix it", otherwise I would
change the other values for clarity and readability, I would also change
tabs/spaces mix in the table mips_syscall_args, etc.

Regards,
Petar

Re: [Qemu-devel] [Qemu-trivial] [PATCH] linux-user: correct argument number for sys_mremap and sys_splice

[Michael Tokarev]

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

Re: [Qemu-devel] [PATCH] linux-user: correct argument number for sys_mremap and sys_splice

[Aurelien Jarno]

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.


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net