Re: [PATCH] linux-user: Add translation for argument of msync()

[Helge Deller <deller@gmx.de>]

On 12/15/22 08:58, Philippe Mathieu-Daudé wrote:
> On 15/12/22 08:27, Helge Deller wrote:
>> msync() uses the flags MS_ASYNC, MS_INVALIDATE and MS_SYNC, which differ
>> between platforms, specifcally on alpha and hppa.
>>
>> Add a target to host translation for those and wire up a nicer strace
>> output.
>>
>> This fixes the testsuite of the macaulay2 debian package with a hppa-linux
>> guest on a x86-64 host.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>>
>> diff --git a/linux-user/alpha/target_mman.h b/linux-user/alpha/target_mman.h
>> index cd6e3d70a6..051544f5ab 100644
>> --- a/linux-user/alpha/target_mman.h
>> +++ b/linux-user/alpha/target_mman.h
>> @@ -3,6 +3,10 @@
>>
>>   #define TARGET_MADV_DONTNEED 6
>>
>> +#define TARGET_MS_ASYNC 1
>> +#define TARGET_MS_SYNC 2
>> +#define TARGET_MS_INVALIDATE 4
>> +
>>   #include "../generic/target_mman.h"
>>
>>   #endif
>> diff --git a/linux-user/generic/target_mman.h b/linux-user/generic/target_mman.h
>> index 1436a3c543..32bf1a52d0 100644
>> --- a/linux-user/generic/target_mman.h
>> +++ b/linux-user/generic/target_mman.h
>> @@ -89,4 +89,17 @@
>>   #define TARGET_MADV_DONTNEED_LOCKED 24
>>   #endif
>>
>> +
>> +#ifndef TARGET_MS_ASYNC
>> +#define TARGET_MS_ASYNC 1
>
> Hmm don't we want to keep the host flag instead?
>
>     #define TARGET_MS_ASYNC MS_ASYNC

Yes, that would be possible, but the value is the same.
In the <arch>/*h files you usually want to have numerical values
which makes it easier to search for conversion bugs.

I'd prefer to keep it as is, it's done for the other
files/values like that.

Helge


>
>> +#endif
>> +
>> +#ifndef TARGET_MS_INVALIDATE
>> +#define TARGET_MS_INVALIDATE 2
>
> Ditto,
>
>> +#endif
>> +
>> +#ifndef TARGET_MS_SYNC
>> +#define TARGET_MS_SYNC 4
>
> ditto.
>
> LGTM otherwise.
>
>> +#endif
>> +
>>   #endif
>> diff --git a/linux-user/hppa/target_mman.h b/linux-user/hppa/target_mman.h
>> index 66dd9f7941..f9b6b97032 100644
>> --- a/linux-user/hppa/target_mman.h
>> +++ b/linux-user/hppa/target_mman.h
>> @@ -10,6 +10,10 @@
>>   #define TARGET_MADV_WIPEONFORK 71
>>   #define TARGET_MADV_KEEPONFORK 72
>>
>> +#define TARGET_MS_SYNC 1
>> +#define TARGET_MS_ASYNC 2
>> +#define TARGET_MS_INVALIDATE 4
>> +
>>   #include "../generic/target_mman.h"
>>
>>   #endif
>> diff --git a/linux-user/strace.list b/linux-user/strace.list
>> index a75101fca1..ac8f872371 100644
>> --- a/linux-user/strace.list
>> +++ b/linux-user/strace.list
>> @@ -650,7 +650,7 @@
>>   { TARGET_NR_msgsnd, "msgsnd" , NULL, NULL, NULL },
>>   #endif
>>   #ifdef TARGET_NR_msync
>> -{ TARGET_NR_msync, "msync" , NULL, NULL, NULL },
>> +{ TARGET_NR_msync, "msync" , "%s(%p,%u,%d)", NULL, NULL },
>>   #endif
>>   #ifdef TARGET_NR_multiplexer
>>   { TARGET_NR_multiplexer, "multiplexer" , NULL, NULL, NULL },
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index d58e9b8d10..e541fbe09a 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -22,6 +22,7 @@
>>   #include "qemu/path.h"
>>   #include "qemu/memfd.h"
>>   #include "qemu/queue.h"
>> +#include "target_mman.h"
>>   #include <elf.h>
>>   #include <endian.h>
>>   #include <grp.h>
>> @@ -7667,6 +7668,14 @@ static inline int target_to_host_mlockall_arg(int arg)
>>   }
>>   #endif
>>
>> +static inline int target_to_host_msync_arg(abi_long arg)
>> +{
>> +    return ((arg & TARGET_MS_ASYNC) ? MS_ASYNC : 0) |
>> +           ((arg & TARGET_MS_INVALIDATE) ? MS_INVALIDATE : 0) |
>> +           ((arg & TARGET_MS_SYNC) ? MS_SYNC : 0) |
>> +           (arg & ~(TARGET_MS_ASYNC | TARGET_MS_INVALIDATE | TARGET_MS_SYNC));
>> +}
>> +
>>   #if (defined(TARGET_NR_stat64) || defined(TARGET_NR_lstat64) ||     \
>>        defined(TARGET_NR_fstat64) || defined(TARGET_NR_fstatat64) ||  \
>>        defined(TARGET_NR_newfstatat))
>> @@ -10163,7 +10172,8 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
>>           /* ??? msync/mlock/munlock are broken for softmmu.  */
>>   #ifdef TARGET_NR_msync
>>       case TARGET_NR_msync:
>> -        return get_errno(msync(g2h(cpu, arg1), arg2, arg3));
>> +        return get_errno(msync(g2h(cpu, arg1), arg2,
>> +                               target_to_host_msync_arg(arg3)));
>>   #endif
>>   #ifdef TARGET_NR_mlock
>>       case TARGET_NR_mlock:
>>
>