[PATCH 0/5] semihosting: Reduce target specific code

[PATCH 0/5] semihosting: Reduce target specific code

[Philippe Mathieu-Daudé]

This series makes semihosting config.c and console.c
target agnostic, building them once, removing symbol
collision of the following functions in the single
binary:

 - qemu_semihosting_chardev_init
 - qemu_semihosting_config_options
 - qemu_semihosting_config_opts
 - qemu_semihosting_enable
 - semihosting_arg_fallback
 - semihosting_enabled
 - semihosting_get_argc
 - semihosting_get_target

This function is still problematic, being built for
each target:

 - qemu_semihosting_guestfd_init

Note, it depends on CONFIG_ARM_COMPATIBLE_SEMIHOSTING
which is target specific, so doesn't scale in a
heterogeneous setup like the ZynqMP machine, having
ARM cores with CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y and
MicroBlaze ones with CONFIG_ARM_COMPATIBLE_SEMIHOSTING=n.

I suppose the semihosting API needs rework to consider
the CPUClass? I'll let that investigation for the
maintainer ;)

Regards,

Phil.

Philippe Mathieu-Daudé (5):
  semihosting/syscalls: Include missing 'exec/cpu-defs.h' header
  semihosting/uaccess: Include missing 'exec/cpu-all.h' header
  semihosting/arm-compat: Include missing 'cpu.h' header
  semihosting/console: Avoid including 'cpu.h'
  semihosting/meson: Build config.o and console.o once

 include/semihosting/console.h  | 2 --
 include/semihosting/syscalls.h | 1 +
 semihosting/arm-compat-semi.c  | 1 +
 semihosting/console.c          | 3 ++-
 semihosting/uaccess.c          | 1 +
 semihosting/meson.build        | 9 ++++++---
 6 files changed, 11 insertions(+), 6 deletions(-)

-- 
2.47.1

[PATCH 1/5] semihosting/syscalls: Include missing 'exec/cpu-defs.h' header

[Philippe Mathieu-Daudé]

target_ulong is defined in each target "cpu-param.h",
itself included by "exec/cpu-defs.h".
Include the latter in order to avoid when refactoring:

  include/semihosting/syscalls.h:26:24: error: unknown type name 'target_ulong'
     26 |                        target_ulong fname, target_ulong fname_len,
        |                        ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/semihosting/syscalls.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/semihosting/syscalls.h b/include/semihosting/syscalls.h
index b5937c619a6..6627c45fb28 100644
--- a/include/semihosting/syscalls.h
+++ b/include/semihosting/syscalls.h
@@ -9,6 +9,7 @@
 #ifndef SEMIHOSTING_SYSCALLS_H
 #define SEMIHOSTING_SYSCALLS_H
 
+#include "exec/cpu-defs.h"
 #include "gdbstub/syscalls.h"
 
 /*
-- 
2.47.1

[PATCH 2/5] semihosting/uaccess: Include missing 'exec/cpu-all.h' header

[Philippe Mathieu-Daudé]

TLB_INVALID_MASK is defined in "exec/cpu-all.h".
Include it in order to avoid when refactoring:

  ../semihosting/uaccess.c:41:21: error: use of undeclared identifier 'TLB_INVALID_MASK'
     41 |         if (flags & TLB_INVALID_MASK) {
        |                     ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 semihosting/uaccess.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
index dc587d73bc4..382a366ce31 100644
--- a/semihosting/uaccess.c
+++ b/semihosting/uaccess.c
@@ -8,6 +8,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "exec/cpu-all.h"
 #include "exec/exec-all.h"
 #include "semihosting/uaccess.h"
 
-- 
2.47.1

[PATCH 3/5] semihosting/arm-compat: Include missing 'cpu.h' header

[Philippe Mathieu-Daudé]

ARM semihosting implementations in "common-semi-target.h"
must de-reference the target CPUArchState, which is declared
in each target "cpu.h" header. Include it in order to avoid
when refactoring:

  In file included from ../../semihosting/arm-compat-semi.c:169:
  ../target/riscv/common-semi-target.h:16:5: error: use of undeclared identifier 'RISCVCPU'
     16 |     RISCVCPU *cpu = RISCV_CPU(cs);
        |     ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 semihosting/arm-compat-semi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index d78c6428b90..86e5260e504 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -166,6 +166,7 @@ static LayoutInfo common_semi_find_bases(CPUState *cs)
 
 #endif
 
+#include "cpu.h"
 #include "common-semi-target.h"
 
 /*
-- 
2.47.1

[PATCH 4/5] semihosting/console: Avoid including 'cpu.h'

[Philippe Mathieu-Daudé]

The CPUState structure is declared in "hw/core/cpu.h",
the EXCP_HALTED definition in "exec/cpu-common.h".
Both headers are indirectly include by "cpu.h". In
order to remove "cpu.h" from "semihosting/console.h",
explicitly include them in console.c, otherwise we'd
get:

  ../semihosting/console.c:88:11: error: incomplete definition of type 'struct CPUState'
     88 |         cs->exception_index = EXCP_HALTED;
        |         ~~^
  ../semihosting/console.c:88:31: error: use of undeclared identifier 'EXCP_HALTED'
     88 |         cs->exception_index = EXCP_HALTED;
        |                               ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/semihosting/console.h | 2 --
 semihosting/console.c         | 3 ++-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/semihosting/console.h b/include/semihosting/console.h
index bd78e5f03fc..1c12e178ee3 100644
--- a/include/semihosting/console.h
+++ b/include/semihosting/console.h
@@ -9,8 +9,6 @@
 #ifndef SEMIHOST_CONSOLE_H
 #define SEMIHOST_CONSOLE_H
 
-#include "cpu.h"
-
 /**
  * qemu_semihosting_console_read:
  * @cs: CPUState
diff --git a/semihosting/console.c b/semihosting/console.c
index 60102bbab66..c3683a15668 100644
--- a/semihosting/console.c
+++ b/semihosting/console.c
@@ -18,14 +18,15 @@
 #include "qemu/osdep.h"
 #include "semihosting/semihost.h"
 #include "semihosting/console.h"
+#include "exec/cpu-common.h"
 #include "exec/gdbstub.h"
-#include "exec/exec-all.h"
 #include "qemu/log.h"
 #include "chardev/char.h"
 #include "chardev/char-fe.h"
 #include "qemu/main-loop.h"
 #include "qapi/error.h"
 #include "qemu/fifo8.h"
+#include "hw/core/cpu.h"
 
 /* Access to this structure is protected by the BQL */
 typedef struct SemihostingConsole {
-- 
2.47.1

[PATCH 5/5] semihosting/meson: Build config.o and console.o once

[Philippe Mathieu-Daudé]

config.c and console.c don't use any target specific
headers anymore, move them from specific_ss[] to
system_ss[] so they are built once, but will also be
linked once, removing global symbol clash in a single
QEMU binary.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 semihosting/meson.build | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/semihosting/meson.build b/semihosting/meson.build
index 34933e5a195..86f5004bed7 100644
--- a/semihosting/meson.build
+++ b/semihosting/meson.build
@@ -4,13 +4,16 @@ specific_ss.add(when: 'CONFIG_SEMIHOSTING', if_true: files(
 ))
 
 specific_ss.add(when: ['CONFIG_SEMIHOSTING', 'CONFIG_SYSTEM_ONLY'], if_true: files(
-  'config.c',
-  'console.c',
   'uaccess.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'))
+system_ss.add(when: ['CONFIG_SEMIHOSTING'], if_true: files(
+  'config.c',
+  'console.c',
+), if_false: files(
+  'stubs-system.c',
+))
 
 specific_ss.add(when: ['CONFIG_ARM_COMPATIBLE_SEMIHOSTING'],
 		if_true: files('arm-compat-semi.c'))
-- 
2.47.1

Re: [PATCH 0/5] semihosting: Reduce target specific code

[Richard Henderson]

On 1/3/25 09:10, Philippe Mathieu-Daudé wrote:
> Philippe Mathieu-Daudé (5):
>    semihosting/syscalls: Include missing 'exec/cpu-defs.h' header
>    semihosting/uaccess: Include missing 'exec/cpu-all.h' header
>    semihosting/arm-compat: Include missing 'cpu.h' header
>    semihosting/console: Avoid including 'cpu.h'
>    semihosting/meson: Build config.o and console.o once

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 0/5] semihosting: Reduce target specific code

[Alex Bennée]

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

> This series makes semihosting config.c and console.c
> target agnostic, building them once, removing symbol
> collision of the following functions in the single
> binary:

Queued to semihosting/next, thanks.

>  - qemu_semihosting_chardev_init
>  - qemu_semihosting_config_options
>  - qemu_semihosting_config_opts
>  - qemu_semihosting_enable
>  - semihosting_arg_fallback
>  - semihosting_enabled
>  - semihosting_get_argc
>  - semihosting_get_target
>
> This function is still problematic, being built for
> each target:
>
>  - qemu_semihosting_guestfd_init
>
> Note, it depends on CONFIG_ARM_COMPATIBLE_SEMIHOSTING
> which is target specific, so doesn't scale in a
> heterogeneous setup like the ZynqMP machine, having
> ARM cores with CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y and
> MicroBlaze ones with CONFIG_ARM_COMPATIBLE_SEMIHOSTING=n.

Does MicroBlaze even do semihosting?

> I suppose the semihosting API needs rework to consider
> the CPUClass? I'll let that investigation for the
> maintainer ;)

Hmm most of it is already handled as EXCP_SEMIHOST exceptions are dealt
with withing the target specific exception handlers.
do_common_semihosting could be renamed though - do_armc_semihosting()
maybe?

If we have the full list of CPUs at qemu_semihosting_chardev_init() time
we could then selectively do the bits of qemu_semihosting_guestfd_init()
depending on what combination we have. For normal open/read/write stuff
I think they could co-exist.

Two independent cores could still write to stdout (0) though. Fixing
that would need a per-cpu semihosting config.

>
> Regards,
>
> Phil.
>
> Philippe Mathieu-Daudé (5):
>   semihosting/syscalls: Include missing 'exec/cpu-defs.h' header
>   semihosting/uaccess: Include missing 'exec/cpu-all.h' header
>   semihosting/arm-compat: Include missing 'cpu.h' header
>   semihosting/console: Avoid including 'cpu.h'
>   semihosting/meson: Build config.o and console.o once
>
>  include/semihosting/console.h  | 2 --
>  include/semihosting/syscalls.h | 1 +
>  semihosting/arm-compat-semi.c  | 1 +
>  semihosting/console.c          | 3 ++-
>  semihosting/uaccess.c          | 1 +
>  semihosting/meson.build        | 9 ++++++---
>  6 files changed, 11 insertions(+), 6 deletions(-)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 0/5] semihosting: Reduce target specific code

[Richard Henderson]

On 1/8/25 07:26, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> This series makes semihosting config.c and console.c
>> target agnostic, building them once, removing symbol
>> collision of the following functions in the single
>> binary:
> 
> Queued to semihosting/next, thanks.
> 
>>   - qemu_semihosting_chardev_init
>>   - qemu_semihosting_config_options
>>   - qemu_semihosting_config_opts
>>   - qemu_semihosting_enable
>>   - semihosting_arg_fallback
>>   - semihosting_enabled
>>   - semihosting_get_argc
>>   - semihosting_get_target
>>
>> This function is still problematic, being built for
>> each target:
>>
>>   - qemu_semihosting_guestfd_init
>>
>> Note, it depends on CONFIG_ARM_COMPATIBLE_SEMIHOSTING
>> which is target specific, so doesn't scale in a
>> heterogeneous setup like the ZynqMP machine, having
>> ARM cores with CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y and
>> MicroBlaze ones with CONFIG_ARM_COMPATIBLE_SEMIHOSTING=n.
> 
> Does MicroBlaze even do semihosting?
> 
>> I suppose the semihosting API needs rework to consider
>> the CPUClass? I'll let that investigation for the
>> maintainer ;)
> 
> Hmm most of it is already handled as EXCP_SEMIHOST exceptions are dealt
> with withing the target specific exception handlers.
> do_common_semihosting could be renamed though - do_armc_semihosting()
> maybe?
> 
> If we have the full list of CPUs at qemu_semihosting_chardev_init() time
> we could then selectively do the bits of qemu_semihosting_guestfd_init()
> depending on what combination we have. For normal open/read/write stuff
> I think they could co-exist.
> 
> Two independent cores could still write to stdout (0) though. Fixing
> that would need a per-cpu semihosting config.

None of the semihosting stuff is smp safe.

The assumption in the homogeneous cpu case is that the guest uses it's own mutexes to 
protect the semihosting calls.  This is obviously more complicated in the heterogeneous 
case, but it *still* should not be qemu's problem.


r~

Re: [PATCH 0/5] semihosting: Reduce target specific code

[Philippe Mathieu-Daudé]

+Paolo & Marc-André Lureau for chardev backend.

On 8/1/25 23:53, Richard Henderson wrote:
> On 1/8/25 07:26, Alex Bennée wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>
>>> This series makes semihosting config.c and console.c
>>> target agnostic, building them once, removing symbol
>>> collision of the following functions in the single
>>> binary:
>>
>> Queued to semihosting/next, thanks.
>>
>>>   - qemu_semihosting_chardev_init
>>>   - qemu_semihosting_config_options
>>>   - qemu_semihosting_config_opts
>>>   - qemu_semihosting_enable
>>>   - semihosting_arg_fallback
>>>   - semihosting_enabled
>>>   - semihosting_get_argc
>>>   - semihosting_get_target
>>>
>>> This function is still problematic, being built for
>>> each target:
>>>
>>>   - qemu_semihosting_guestfd_init
>>>
>>> Note, it depends on CONFIG_ARM_COMPATIBLE_SEMIHOSTING
>>> which is target specific, so doesn't scale in a
>>> heterogeneous setup like the ZynqMP machine, having
>>> ARM cores with CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y and
>>> MicroBlaze ones with CONFIG_ARM_COMPATIBLE_SEMIHOSTING=n.
>>
>> Does MicroBlaze even do semihosting?
>>
>>> I suppose the semihosting API needs rework to consider
>>> the CPUClass? I'll let that investigation for the
>>> maintainer ;)
>>
>> Hmm most of it is already handled as EXCP_SEMIHOST exceptions are dealt
>> with withing the target specific exception handlers.
>> do_common_semihosting could be renamed though - do_armc_semihosting()
>> maybe?
>>
>> If we have the full list of CPUs at qemu_semihosting_chardev_init() time
>> we could then selectively do the bits of qemu_semihosting_guestfd_init()
>> depending on what combination we have. For normal open/read/write stuff
>> I think they could co-exist.
>>
>> Two independent cores could still write to stdout (0) though. Fixing
>> that would need a per-cpu semihosting config.

What I'd expect here is one VC per semihosting context stdout. If we
want to mux, we use the chardev mux.

Anyhow this in particular is not a blocker, it was just an opened
question.

> None of the semihosting stuff is smp safe.
> 
> The assumption in the homogeneous cpu case is that the guest uses it's 
> own mutexes to protect the semihosting calls.  This is obviously more 
> complicated in the heterogeneous case, but it *still* should not be 
> qemu's problem.

FYI the use case requested is $n Hexagon cores writing to $n semihosting
file descriptors, and a user-mode process on an ARM core able to read
each of these FDs.

Regards,

Phil.