| Message ID | 20201026212853.92880-2-keithp@keithp.com |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: Add semihosting support [v10] | expand |
On Mon, Oct 26, 2020 at 2:34 PM Keith Packard via <qemu-devel@nongnu.org> wrote: > > This commit renames two files which provide ARM semihosting support so > that they can be shared by other architectures: > > 1. target/arm/arm-semi.c -> hw/semihosting/common-semi.c > 2. linux-user/arm/semihost.c -> linux-user/semihost.c > > The build system was modified to reflect this change, but the contents > of the two files are unchanged. > > Signed-off-by: Keith Packard <keithp@keithp.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/arm/arm-semi.c => hw/semihosting/common-semi.c | 0 > hw/semihosting/meson.build | 2 ++ > linux-user/arm/meson.build | 3 --- > linux-user/meson.build | 2 ++ > linux-user/{arm => }/semihost.c | 0 > target/arm/meson.build | 2 -- > 6 files changed, 4 insertions(+), 5 deletions(-) > rename target/arm/arm-semi.c => hw/semihosting/common-semi.c (100%) > rename linux-user/{arm => }/semihost.c (100%) > > diff --git a/target/arm/arm-semi.c b/hw/semihosting/common-semi.c > similarity index 100% > rename from target/arm/arm-semi.c > rename to hw/semihosting/common-semi.c > diff --git a/hw/semihosting/meson.build b/hw/semihosting/meson.build > index f40ac574c4..fbd2841e59 100644 > --- a/hw/semihosting/meson.build > +++ b/hw/semihosting/meson.build > @@ -2,3 +2,5 @@ specific_ss.add(when: 'CONFIG_SEMIHOSTING', if_true: files( > 'config.c', > 'console.c', > )) > + > +specific_ss.add(when: 'CONFIG_TCG', if_true: files ('common-semi.c')) > diff --git a/linux-user/arm/meson.build b/linux-user/arm/meson.build > index 432984b58e..5a93c925cf 100644 > --- a/linux-user/arm/meson.build > +++ b/linux-user/arm/meson.build > @@ -1,6 +1,3 @@ > -linux_user_ss.add(when: 'TARGET_AARCH64', if_true: files('semihost.c')) > -linux_user_ss.add(when: 'TARGET_ARM', if_true: files('semihost.c')) > - > subdir('nwfpe') > > syscall_nr_generators += { > diff --git a/linux-user/meson.build b/linux-user/meson.build > index 2b94e4ba24..2fdd12cee5 100644 > --- a/linux-user/meson.build > +++ b/linux-user/meson.build > @@ -17,6 +17,8 @@ linux_user_ss.add(rt) > linux_user_ss.add(when: 'TARGET_HAS_BFLT', if_true: files('flatload.c')) > linux_user_ss.add(when: 'TARGET_I386', if_true: files('vm86.c')) > > +linux_user_ss.add(when: 'TARGET_AARCH64', if_true: files('semihost.c')) > +linux_user_ss.add(when: 'TARGET_ARM', if_true: files('semihost.c')) > > syscall_nr_generators = {} > > diff --git a/linux-user/arm/semihost.c b/linux-user/semihost.c > similarity index 100% > rename from linux-user/arm/semihost.c > rename to linux-user/semihost.c > diff --git a/target/arm/meson.build b/target/arm/meson.build > index f5de2a77b8..15b936c101 100644 > --- a/target/arm/meson.build > +++ b/target/arm/meson.build > @@ -32,8 +32,6 @@ arm_ss.add(files( > )) > arm_ss.add(zlib) > > -arm_ss.add(when: 'CONFIG_TCG', if_true: files('arm-semi.c')) > - > arm_ss.add(when: 'CONFIG_KVM', if_true: files('kvm.c', 'kvm64.c'), if_false: files('kvm-stub.c')) > > arm_ss.add(when: 'TARGET_AARCH64', if_true: files( > -- > 2.28.0 > >
On Mon, 26 Oct 2020 at 21:29, Keith Packard <keithp@keithp.com> wrote: > > This commit renames two files which provide ARM semihosting support so > that they can be shared by other architectures: > > 1. target/arm/arm-semi.c -> hw/semihosting/common-semi.c > 2. linux-user/arm/semihost.c -> linux-user/semihost.c > > The build system was modified to reflect this change, but the contents > of the two files are unchanged. > > Signed-off-by: Keith Packard <keithp@keithp.com> > --- > diff --git a/target/arm/arm-semi.c b/hw/semihosting/common-semi.c > similarity index 100% > rename from target/arm/arm-semi.c > rename to hw/semihosting/common-semi.c > diff --git a/hw/semihosting/meson.build b/hw/semihosting/meson.build > index f40ac574c4..fbd2841e59 100644 > --- a/hw/semihosting/meson.build > +++ b/hw/semihosting/meson.build > @@ -2,3 +2,5 @@ specific_ss.add(when: 'CONFIG_SEMIHOSTING', if_true: files( > 'config.c', > 'console.c', > )) > + > +specific_ss.add(when: 'CONFIG_TCG', if_true: files ('common-semi.c')) I think this adds this file to the compilation for all TCG targets; you only want it for targets which have Arm-semihosting-ABI compatible semihosting. (Various other targets either don't have semihosting or have their own ABI.) thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: >> +specific_ss.add(when: 'CONFIG_TCG', if_true: files ('common-semi.c')) > > I think this adds this file to the compilation for all TCG targets; > you only want it for targets which have Arm-semihosting-ABI compatible > semihosting. (Various other targets either don't have semihosting > or have their own ABI.) Right. These should have been target-specific source sets so that the file was only added to those targets: arm_ss.add(when: 'CONFIG_TCG', if_true: files ('common-semi.c')) riscv_ss.add(files('common-semi.c')) This appears to work in my testing (building arm, risc-v and x86_64 configs).
On Tue, 27 Oct 2020 at 21:56, Keith Packard <keithp@keithp.com> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > >> +specific_ss.add(when: 'CONFIG_TCG', if_true: files ('common-semi.c')) > > > > I think this adds this file to the compilation for all TCG targets; > > you only want it for targets which have Arm-semihosting-ABI compatible > > semihosting. (Various other targets either don't have semihosting > > or have their own ABI.) > > Right. These should have been target-specific source sets so that the > file was only added to those targets: > > arm_ss.add(when: 'CONFIG_TCG', if_true: files ('common-semi.c')) > riscv_ss.add(files('common-semi.c')) > > This appears to work in my testing (building arm, risc-v and x86_64 > configs). I'm not a kconfig expert but it might be preferable to have a new CONFIG_ for arm-semihosting-ABI which the relevant targets enable. Somebody else may be able to advise. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: >> arm_ss.add(when: 'CONFIG_TCG', if_true: files ('common-semi.c')) >> riscv_ss.add(files('common-semi.c')) >> >> This appears to work in my testing (building arm, risc-v and x86_64 >> configs). > > I'm not a kconfig expert but it might be preferable to have > a new CONFIG_ for arm-semihosting-ABI which the relevant > targets enable. Somebody else may be able to advise. The change above makes this do exactly what the old code did -- add this file to the arm_ss sourceset whenever CONFIG_TCG is true. arm_ss is only used to build ARM targets, so this file gets added only for those targets. Here's what the patch to target/arm/meson.build and hw/semihosting/meson.build looks like now. diff --git a/target/arm/meson.build b/target/arm/meson.build index f5de2a77b8..15b936c101 100644 --- a/target/arm/meson.build +++ b/target/arm/meson.build @@ -32,8 +32,6 @@ arm_ss.add(files( )) arm_ss.add(zlib) -arm_ss.add(when: 'CONFIG_TCG', if_true: files('arm-semi.c')) - arm_ss.add(when: 'CONFIG_KVM', if_true: files('kvm.c', 'kvm64.c'), if_false: files('kvm-stub.c')) arm_ss.add(when: 'TARGET_AARCH64', if_true: files( diff --git a/hw/semihosting/meson.build b/hw/semihosting/meson.build index f40ac574c4..26538e81e7 100644 --- a/hw/semihosting/meson.build +++ b/hw/semihosting/meson.build @@ -2,3 +2,5 @@ specific_ss.add(when: 'CONFIG_SEMIHOSTING', if_true: files( 'config.c', 'console.c', )) + +arm_ss.add(when: 'CONFIG_TCG', if_true: files ('common-semi.c'))
Alex Bennée <alex.bennee@linaro.org> writes: > I don't think we want arm specific adds in the common code. I think what > Peter was suggesting is a new config symbol that only ARM and RISC > define, e.g something like: > > specific_ss.add(when: 'CONFIG_ARM_STYLE_SEMIHOSTING', > if_true: files ('common-semi.c')) > > or some other suitably descriptive symbol. Otherwise you have to keep > adding an foo_ss.add line for each architecture that wants it. Sure, that will also work, just a few lines of additional config file stuff.
Alex Bennée <alex.bennee@linaro.org> writes: > specific_ss.add(when: 'CONFIG_ARM_STYLE_SEMIHOSTING', > if_true: files ('common-semi.c')) I've sent another version of the series using this plan. It does look a bit nicer as the only changes required when adding support to another target is to place this option in the config file. Thanks for the suggestion, and thanks for helping review this patch!
diff --git a/target/arm/arm-semi.c b/hw/semihosting/common-semi.c similarity index 100% rename from target/arm/arm-semi.c rename to hw/semihosting/common-semi.c diff --git a/hw/semihosting/meson.build b/hw/semihosting/meson.build index f40ac574c4..fbd2841e59 100644 --- a/hw/semihosting/meson.build +++ b/hw/semihosting/meson.build @@ -2,3 +2,5 @@ specific_ss.add(when: 'CONFIG_SEMIHOSTING', if_true: files( 'config.c', 'console.c', )) + +specific_ss.add(when: 'CONFIG_TCG', if_true: files ('common-semi.c')) diff --git a/linux-user/arm/meson.build b/linux-user/arm/meson.build index 432984b58e..5a93c925cf 100644 --- a/linux-user/arm/meson.build +++ b/linux-user/arm/meson.build @@ -1,6 +1,3 @@ -linux_user_ss.add(when: 'TARGET_AARCH64', if_true: files('semihost.c')) -linux_user_ss.add(when: 'TARGET_ARM', if_true: files('semihost.c')) - subdir('nwfpe') syscall_nr_generators += { diff --git a/linux-user/meson.build b/linux-user/meson.build index 2b94e4ba24..2fdd12cee5 100644 --- a/linux-user/meson.build +++ b/linux-user/meson.build @@ -17,6 +17,8 @@ linux_user_ss.add(rt) linux_user_ss.add(when: 'TARGET_HAS_BFLT', if_true: files('flatload.c')) linux_user_ss.add(when: 'TARGET_I386', if_true: files('vm86.c')) +linux_user_ss.add(when: 'TARGET_AARCH64', if_true: files('semihost.c')) +linux_user_ss.add(when: 'TARGET_ARM', if_true: files('semihost.c')) syscall_nr_generators = {} diff --git a/linux-user/arm/semihost.c b/linux-user/semihost.c similarity index 100% rename from linux-user/arm/semihost.c rename to linux-user/semihost.c diff --git a/target/arm/meson.build b/target/arm/meson.build index f5de2a77b8..15b936c101 100644 --- a/target/arm/meson.build +++ b/target/arm/meson.build @@ -32,8 +32,6 @@ arm_ss.add(files( )) arm_ss.add(zlib) -arm_ss.add(when: 'CONFIG_TCG', if_true: files('arm-semi.c')) - arm_ss.add(when: 'CONFIG_KVM', if_true: files('kvm.c', 'kvm64.c'), if_false: files('kvm-stub.c')) arm_ss.add(when: 'TARGET_AARCH64', if_true: files(
This commit renames two files which provide ARM semihosting support so that they can be shared by other architectures: 1. target/arm/arm-semi.c -> hw/semihosting/common-semi.c 2. linux-user/arm/semihost.c -> linux-user/semihost.c The build system was modified to reflect this change, but the contents of the two files are unchanged. Signed-off-by: Keith Packard <keithp@keithp.com> --- target/arm/arm-semi.c => hw/semihosting/common-semi.c | 0 hw/semihosting/meson.build | 2 ++ linux-user/arm/meson.build | 3 --- linux-user/meson.build | 2 ++ linux-user/{arm => }/semihost.c | 0 target/arm/meson.build | 2 -- 6 files changed, 4 insertions(+), 5 deletions(-) rename target/arm/arm-semi.c => hw/semihosting/common-semi.c (100%) rename linux-user/{arm => }/semihost.c (100%)