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

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

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