Re: [PATCH 1/1] plugins: use int64_t for the syscall filter return value

["Alex Bennée" <alex.bennee@linaro.org>]

Ziyang Zhang <functioner@sjtu.edu.cn> writes:

> The syscall return value passed back through the syscall filter
> callback is semantically signed: negative values encode errno codes.
> Declaring the sysret pointer as uint64_t * is therefore misleading and
> forces callers to launder the value through an unsigned temporary.
>
> Change the sysret pointer to int64_t * across the public plugin API
> typedef (qemu_plugin_vcpu_syscall_filter_cb_t), the internal
> qemu_plugin_vcpu_syscall_filter() prototypes and stub, its
> implementation in plugins/core.c, the linux-user caller, and the
> in-tree example plugins.
>
> Signed-off-by: Ziyang Zhang <functioner@sjtu.edu.cn>
> ---
>  include/plugins/qemu-plugin.h | 2 +-
>  include/qemu/plugin.h         | 4 ++--
>  linux-user/syscall.c          | 2 +-
>  plugins/core.c                | 2 +-
>  tests/tcg/plugins/setpc.c     | 2 +-
>  tests/tcg/plugins/syscall.c   | 2 +-
>  6 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/include/plugins/qemu-plugin.h b/include/plugins/qemu-plugin.h
> index 4eb1d2cd85..8eb10b1f48 100644
> --- a/include/plugins/qemu-plugin.h
> +++ b/include/plugins/qemu-plugin.h
> @@ -870,7 +870,7 @@ typedef bool
>                                          int64_t num, uint64_t a1, uint64_t a2,
>                                          uint64_t a3, uint64_t a4, uint64_t a5,
>                                          uint64_t a6, uint64_t a7, uint64_t a8,
> -                                        uint64_t *sysret);
> +                                        int64_t *sysret);
>  
>  /**
>   * typedef qemu_plugin_vcpu_syscall_ret_cb_t - vCPU syscall return callback
> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
> index ddd77bd82c..1ce4b281c1 100644
> --- a/include/qemu/plugin.h
> +++ b/include/qemu/plugin.h
> @@ -174,7 +174,7 @@ bool
>  qemu_plugin_vcpu_syscall_filter(CPUState *cpu, int64_t num, uint64_t a1,
>                                  uint64_t a2, uint64_t a3, uint64_t a4,
>                                  uint64_t a5, uint64_t a6, uint64_t a7,
> -                                uint64_t a8, uint64_t *sysret);
> +                                uint64_t a8, int64_t *sysret);
>  
>  void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
>                               uint64_t value_low,
> @@ -290,7 +290,7 @@ static inline bool
>  qemu_plugin_vcpu_syscall_filter(CPUState *cpu, int64_t num, uint64_t a1,
>                                  uint64_t a2, uint64_t a3, uint64_t a4,
>                                  uint64_t a5, uint64_t a6, uint64_t a7,
> -                                uint64_t a8, uint64_t *sysret)
> +                                uint64_t a8, int64_t *sysret)
>  {
>      return false;
>  }
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index f4b74ad350..63c0a5f8f3 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -14378,7 +14378,7 @@ static bool send_through_syscall_filters(CPUState *cpu, int num,
>                                           abi_long arg7, abi_long arg8,
>                                           abi_long *sysret)
>  {
> -    uint64_t sysret64 = 0;
> +    int64_t sysret64 = 0;
>      bool filtered = qemu_plugin_vcpu_syscall_filter(cpu, num, arg1, arg2,
>                                                      arg3, arg4, arg5, arg6,
>                                                      arg7, arg8,
>  &sysret64);

All of the arguments here are abi_long which go to int32_t or
target_long->int64_t so perhaps we should be using that for all args to
ensure signedness is correct?

> diff --git a/plugins/core.c b/plugins/core.c
> index 2324bbffa3..58f293462a 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -596,7 +596,7 @@ bool
>  qemu_plugin_vcpu_syscall_filter(CPUState *cpu, int64_t num, uint64_t a1,
>                                  uint64_t a2, uint64_t a3, uint64_t a4,
>                                  uint64_t a5, uint64_t a6, uint64_t a7,
> -                                uint64_t a8, uint64_t *sysret)
> +                                uint64_t a8, int64_t *sysret)
>  {
>      struct qemu_plugin_cb *cb, *next;
>      enum qemu_plugin_event ev = QEMU_PLUGIN_EV_VCPU_SYSCALL_FILTER;
> diff --git a/tests/tcg/plugins/setpc.c b/tests/tcg/plugins/setpc.c
> index 8f2d025e24..23862eaaf0 100644
> --- a/tests/tcg/plugins/setpc.c
> +++ b/tests/tcg/plugins/setpc.c
> @@ -27,7 +27,7 @@ static bool vcpu_syscall_filter(qemu_plugin_id_t id, unsigned int vcpu_index,
>                                  int64_t num, uint64_t a1, uint64_t a2,
>                                  uint64_t a3, uint64_t a4, uint64_t a5,
>                                  uint64_t a6, uint64_t a7, uint64_t a8,
> -                                uint64_t *sysret)
> +                                int64_t *sysret)
>  {
>      if (num == MAGIC_SYSCALL) {
>          if (a1 == SETPC) {
> diff --git a/tests/tcg/plugins/syscall.c b/tests/tcg/plugins/syscall.c
> index 5658f83087..76d52b98aa 100644
> --- a/tests/tcg/plugins/syscall.c
> +++ b/tests/tcg/plugins/syscall.c
> @@ -174,7 +174,7 @@ static bool vcpu_syscall_filter(qemu_plugin_id_t id, unsigned int vcpu_index,
>                                  int64_t num, uint64_t a1, uint64_t a2,
>                                  uint64_t a3, uint64_t a4, uint64_t a5,
>                                  uint64_t a6, uint64_t a7, uint64_t a8,
> -                                uint64_t *sysret)
> +                                int64_t *sysret)
>  {
>      /* Special syscall to test the filter functionality. */
>      if (num == 4096 && a1 == 0x66CCFF) {

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro