| Message ID | 20240529155548.5878-4-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | semihosting: Restrict to TCG | expand |
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. Paolo > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > semihosting/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/semihosting/Kconfig b/semihosting/Kconfig > index eaf3a20ef5..fbe6ac87f9 100644 > --- a/semihosting/Kconfig > +++ b/semihosting/Kconfig > @@ -1,6 +1,7 @@ > > config SEMIHOSTING > bool > + depends on TCG > > config ARM_COMPATIBLE_SEMIHOSTING > bool > -- > 2.41.0 >
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/). > Paolo > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> semihosting/Kconfig | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/semihosting/Kconfig b/semihosting/Kconfig >> index eaf3a20ef5..fbe6ac87f9 100644 >> --- a/semihosting/Kconfig >> +++ b/semihosting/Kconfig >> @@ -1,6 +1,7 @@ >> >> config SEMIHOSTING >> bool >> + depends on TCG >> >> config ARM_COMPATIBLE_SEMIHOSTING >> bool >> -- >> 2.41.0 >> >
Paolo Bonzini <pbonzini@redhat.com> writes: > 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. For Arm at least the HLT instruction is an external debug feature and as such not trappable by KVM. > > Paolo > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> semihosting/Kconfig | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/semihosting/Kconfig b/semihosting/Kconfig >> index eaf3a20ef5..fbe6ac87f9 100644 >> --- a/semihosting/Kconfig >> +++ b/semihosting/Kconfig >> @@ -1,6 +1,7 @@ >> >> config SEMIHOSTING >> bool >> + depends on TCG >> >> config ARM_COMPATIBLE_SEMIHOSTING >> bool >> -- >> 2.41.0 >>
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". Paolo > > Paolo > > > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > >> --- > >> semihosting/Kconfig | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/semihosting/Kconfig b/semihosting/Kconfig > >> index eaf3a20ef5..fbe6ac87f9 100644 > >> --- a/semihosting/Kconfig > >> +++ b/semihosting/Kconfig > >> @@ -1,6 +1,7 @@ > >> > >> config SEMIHOSTING > >> bool > >> + depends on TCG > >> > >> config ARM_COMPATIBLE_SEMIHOSTING > >> bool > >> -- > >> 2.41.0 > >> > > >
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". You are right. Thanks for reminding me Kconfig "imply" :) Regards, Phil.
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 ...
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 > ... >
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?
On 18/6/24 15:56, Alex Bennée wrote: > 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? No, this is still unresolved :/ (this is a ping for Paolo :p)
On Wed, Jun 12, 2024 at 3:13 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > 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 > ... Go ahead and use "select" for these. The file to be stubbed would have to be target/m68k/m68k-semi.c (for do_m68k_semihosting) and target/mips/tcg/sysemu/mips-semi.c (for mips_semihosting), not the common code. Paolo
On 19/6/24 09:49, Paolo Bonzini wrote: > On Wed, Jun 12, 2024 at 3:13 PM Philippe Mathieu-Daudé > <philmd@linaro.org> wrote: >> 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 >> ... > > The file to be stubbed would have > to be target/m68k/m68k-semi.c (for do_m68k_semihosting) and > target/mips/tcg/sysemu/mips-semi.c (for mips_semihosting), not the > common code. Yes! Thank you, I'm was to blind to realize that...
diff --git a/semihosting/Kconfig b/semihosting/Kconfig index eaf3a20ef5..fbe6ac87f9 100644 --- a/semihosting/Kconfig +++ b/semihosting/Kconfig @@ -1,6 +1,7 @@ config SEMIHOSTING bool + depends on TCG config ARM_COMPATIBLE_SEMIHOSTING bool
It is pointless to build semihosting when TCG is not available. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- semihosting/Kconfig | 1 + 1 file changed, 1 insertion(+)