Re: [PATCH 3/3] semihosting: Restrict to TCG

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

ping :)

On 12/6/24 15:12, Philippe Mathieu-Daudé wrote:
> Hi Paolo,
> 
> On 30/5/24 15:58, Philippe Mathieu-Daudé wrote:
>> On 30/5/24 09:31, Paolo Bonzini wrote:
>>> On Thu, May 30, 2024 at 9:22 AM Philippe Mathieu-Daudé
>>> <philmd@linaro.org> wrote:
>>>>
>>>> On 30/5/24 08:02, Paolo Bonzini wrote:
>>>>> On Wed, May 29, 2024 at 5:56 PM Philippe Mathieu-Daudé
>>>>> <philmd@linaro.org> wrote:
>>>>>> It is pointless to build semihosting when TCG is not available.
>>>>>
>>>>> Why? I would have naively assumed that a suitable semihosting API
>>>>> could be implemented by KVM. The justification (and thus the commit
>>>>> message) needs to be different for each architecture if it's a matter
>>>>> of instruction set or insufficient KVM userspace API.
>>>>
>>>> I wasn't sure where semihosting could be used so asked on IRC and
>>>> Alex told me TCG only. Maybe the current implementation is TCG
>>>> only, and I can reword. It certainly need some refactor to work
>>>> on KVM, because currently semihosting end calling the TCG probe_access
>>>> API, which I'm trying to restrict to TCG in order to ease linking
>>>> multiple libtcg for the single binary (see
>>>> https://lore.kernel.org/qemu-devel/20240529155918.6221-1-philmd@linaro.org/).
>>>
>>> Ok, that goes in the commit message though.
>>>
>>> "Semihosting currently uses the TCG probe_access API. It is pointless
>>> to have it in the binary when TCG isn't".
>>>
>>> and in the first two patches:
>>>
>>> "Semihosting currently uses the TCG probe_access API. To prepare for
>>> encoding the TCG dependency in Kconfig, do not enable it unless TCG is
>>> available".
>>>
>>> But then, "select FOO if TCG" mean that it can be compiled out; so
>>> perhaps "imply SEMIHOSTING if TCG" is better? Same for RISC-V's
>>> "select ARM_COMPATIBLE_SEMIHOSTING if TCG".
> 
> Building qemu-system-mips configured with --without-default-devices:
> 
> Undefined symbols for architecture arm64:
>    "_qemu_semihosting_console_write", referenced from:
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>    "_semihost_sys_close", referenced from:
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>    "_uaccess_strlen_user", referenced from:
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>    ...
> 
> So this one has to use "select".
> 
> Similarly m68k:
> 
> Undefined symbols for architecture arm64:
>    "_semihost_sys_close", referenced from:
>        _do_m68k_semihosting in target_m68k_m68k-semi.c.o
>    ...
> 
> I can link m68k using semihosting stubs but I'm not sure it is right:
> 
> -- >8 --
> diff --git a/semihosting/stubs-target-all.c 
> b/semihosting/stubs-target-all.c
> new file mode 100644
> index 0000000000..1f33173f43
> --- /dev/null
> +++ b/semihosting/stubs-target-all.c
> @@ -0,0 +1,97 @@
> +/*
> + * Semihosting Stubs
> + *
> + * Copyright (c) 2024 Linaro Ltd
> + *
> + * Stubs for semihosting targets that don't actually do semihosting.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "exec/exec-all.h"
> +#include "semihosting/syscalls.h"
> +
> +void semihost_sys_open(CPUState *cs, gdb_syscall_complete_cb complete,
> +                       target_ulong fname, target_ulong fname_len,
> +                       int gdb_flags, int mode)
> +{
> +}
> +
> +void semihost_sys_close(CPUState *cs, gdb_syscall_complete_cb complete, 
> int fd)
> +{
> +}
> +
> +void semihost_sys_read_gf(CPUState *cs, gdb_syscall_complete_cb complete,
> +                          GuestFD *gf, target_ulong buf, target_ulong len)
> +{
> +}
> +
> +void semihost_sys_read(CPUState *cs, gdb_syscall_complete_cb complete,
> +                       int fd, target_ulong buf, target_ulong len)
> +{
> +}
> +
> +void semihost_sys_write_gf(CPUState *cs, gdb_syscall_complete_cb complete,
> +                           GuestFD *gf, target_ulong buf, target_ulong 
> len)
> +{
> +}
> +
> +void semihost_sys_write(CPUState *cs, gdb_syscall_complete_cb complete,
> +                        int fd, target_ulong buf, target_ulong len)
> +{
> +}
> +
> +void semihost_sys_lseek(CPUState *cs, gdb_syscall_complete_cb complete,
> +                        int fd, int64_t off, int gdb_whence)
> +{
> +}
> +
> +void semihost_sys_isatty(CPUState *cs, gdb_syscall_complete_cb 
> complete, int fd)
> +{
> +}
> +
> +void semihost_sys_flen(CPUState *cs, gdb_syscall_complete_cb fstat_cb,
> +                       gdb_syscall_complete_cb flen_cb, int fd,
> +                       target_ulong fstat_addr)
> +{
> +}
> +
> +void semihost_sys_fstat(CPUState *cs, gdb_syscall_complete_cb complete,
> +                        int fd, target_ulong addr)
> +{
> +}
> +
> +void semihost_sys_stat(CPUState *cs, gdb_syscall_complete_cb complete,
> +                       target_ulong fname, target_ulong fname_len,
> +                       target_ulong addr)
> +{
> +}
> +
> +void semihost_sys_remove(CPUState *cs, gdb_syscall_complete_cb complete,
> +                         target_ulong fname, target_ulong fname_len)
> +{
> +}
> +
> +void semihost_sys_rename(CPUState *cs, gdb_syscall_complete_cb complete,
> +                         target_ulong oname, target_ulong oname_len,
> +                         target_ulong nname, target_ulong nname_len)
> +{
> +}
> +
> +void semihost_sys_system(CPUState *cs, gdb_syscall_complete_cb complete,
> +                         target_ulong cmd, target_ulong cmd_len)
> +{
> +}
> +
> +void semihost_sys_gettimeofday(CPUState *cs, gdb_syscall_complete_cb 
> complete,
> +                               target_ulong tv_addr, target_ulong tz_addr)
> +{
> +}
> +
> +#ifndef CONFIG_USER_ONLY
> +void semihost_sys_poll_one(CPUState *cs, gdb_syscall_complete_cb complete,
> +                           int fd, GIOCondition cond, int timeout)
> +{
> +}
> +#endif
> diff --git a/semihosting/meson.build b/semihosting/meson.build
> index 34933e5a19..aa8b7a9913 100644
> --- a/semihosting/meson.build
> +++ b/semihosting/meson.build
> @@ -7,7 +7,7 @@ specific_ss.add(when: ['CONFIG_SEMIHOSTING', 
> 'CONFIG_SYSTEM_ONLY'], if_true: fil
>     'config.c',
>     'console.c',
>     'uaccess.c',
> -))
> +), if_false: files('stubs-target-all.c'))
> 
>   common_ss.add(when: ['CONFIG_SEMIHOSTING', 'CONFIG_SYSTEM_ONLY'], 
> if_false: files('stubs-all.c'))
>   system_ss.add(when: ['CONFIG_SEMIHOSTING'], if_false: 
> files('stubs-system.c'))
> ---
> 
> For mips more stubs are needed:
> 
> Undefined symbols for architecture arm64:
>    "_qemu_semihosting_console_write", referenced from:
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>    "_uaccess_lock_user", referenced from:
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>        _uhi_fstat_cb in target_mips_tcg_sysemu_mips-semi.c.o
>    "_uaccess_lock_user_string", referenced from:
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>        _mips_semihosting.cold.6 in target_mips_tcg_sysemu_mips-semi.c.o
>        _mips_semihosting.cold.6 in target_mips_tcg_sysemu_mips-semi.c.o
>    "_uaccess_strlen_user", referenced from:
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>    "_uaccess_unlock_user", referenced from:
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>        _mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
>        _uhi_fstat_cb in target_mips_tcg_sysemu_mips-semi.c.o
>        ...
>