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

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

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

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

Oh sorry I was waiting for a re-spin. I should just apply the above to
3/3?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro