[PATCH 0/3] target/arm: disable-tcg and without-default-devices fixes

[PATCH 0/3] target/arm: disable-tcg and without-default-devices fixes

[Fabiano Rosas]

Here's the fix for the cdrom test failure that we discussed in the
list, plus 2 fixes for the ---without-default-devices build.

When I moved the boards CONFIGs from default.mak to Kconfig, it became
possible (due to --without-default-devices) to disable the CONFIGs for
all the boards that require ARM_V7M. That breaks the build because
ARM_V7M is required to be always set.

Fabiano Rosas (3):
  target/arm: Use CONFIG_SEMIHOSTING instead of TCG for semihosting
  target/arm: Add CONFIG_ARM_V7M back to default.mak
  tests/qtest: Don't run cdrom tests if no accelerator is present

 configs/devices/arm-softmmu/default.mak | 1 +
 target/arm/helper.c                     | 4 ++--
 target/arm/tcg/m_helper.c               | 2 +-
 tests/qtest/cdrom-test.c                | 5 +++++
 4 files changed, 9 insertions(+), 3 deletions(-)

-- 
2.35.3

[PATCH 1/3] target/arm: Use CONFIG_SEMIHOSTING instead of TCG for semihosting

[Fabiano Rosas]

When building --without-default-devices, the semihosting code will not
be available, so check the proper config.

Fixes: 29d9efca16 ("arm/Kconfig: Do not build TCG-only boards on a KVM-only build")
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 target/arm/helper.c       | 4 ++--
 target/arm/tcg/m_helper.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2297626bfb..24bb7efb34 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10972,7 +10972,7 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
  * We only see semihosting exceptions in TCG only as they are not
  * trapped to the hypervisor in KVM.
  */
-#ifdef CONFIG_TCG
+#ifdef CONFIG_SEMIHOSTING
 static void tcg_handle_semihosting(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
@@ -11033,7 +11033,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
      * that caused the exception, not the target exception level, so
      * must be handled here.
      */
-#ifdef CONFIG_TCG
+#ifdef CONFIG_SEMIHOSTING
     if (cs->exception_index == EXCP_SEMIHOST) {
         tcg_handle_semihosting(cs);
         return;
diff --git a/target/arm/tcg/m_helper.c b/target/arm/tcg/m_helper.c
index 9758f225d6..4261f1bb1e 100644
--- a/target/arm/tcg/m_helper.c
+++ b/target/arm/tcg/m_helper.c
@@ -2345,7 +2345,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         qemu_log_mask(CPU_LOG_INT,
                       "...handling as semihosting call 0x%x\n",
                       env->regs[0]);
-#ifdef CONFIG_TCG
+#ifdef CONFIG_SEMIHOSTING
         do_common_semihosting(cs);
 #else
         g_assert_not_reached();
-- 
2.35.3

Re: [PATCH 1/3] target/arm: Use CONFIG_SEMIHOSTING instead of TCG for semihosting

[Paolo Bonzini]

On 5/3/23 21:38, Fabiano Rosas wrote:
> When building --without-default-devices, the semihosting code will not
> be available, so check the proper config.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

for this change; however, there are two more related issues:

1) you still want to leave out the code if !TCG, because KVM is not able
to exit to userspace on semihosting calls as far as I understand

2) I am not sure why CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y appears in
config/targets/{arm,riscv32,riscv64}-softmmu/default.mak.

Putting things together you also need something like

diff --git a/semihosting/Kconfig b/semihosting/Kconfig
index eaf3a20ef5b2..671020a33426 100644
--- a/semihosting/Kconfig
+++ b/semihosting/Kconfig
@@ -4,4 +4,5 @@ config SEMIHOSTING
  
  config ARM_COMPATIBLE_SEMIHOSTING
         bool
+       default y if (ARM && TCG) || RISCV32 || RISCV64
         select SEMIHOSTING
diff --git a/configs/devices/arm-softmmu/default.mak b/configs/devices/arm-softmmu/default.mak
index 1b49a7830c7e..5e7a17d05bf8 100644
--- a/configs/devices/arm-softmmu/default.mak
+++ b/configs/devices/arm-softmmu/default.mak
@@ -41,5 +41,4 @@ CONFIG_FSL_IMX25=y
  CONFIG_FSL_IMX7=y
  CONFIG_FSL_IMX6UL=y
  CONFIG_SEMIHOSTING=y
-CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
  CONFIG_ALLWINNER_H3=y
diff --git a/configs/devices/riscv32-softmmu/default.mak b/configs/devices/riscv32-softmmu/default.mak
index d847bd5692ec..94a236c9c25b 100644
--- a/configs/devices/riscv32-softmmu/default.mak
+++ b/configs/devices/riscv32-softmmu/default.mak
@@ -3,8 +3,6 @@
  # Uncomment the following lines to disable these optional devices:
  #
  #CONFIG_PCI_DEVICES=n
-CONFIG_SEMIHOSTING=y
-CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
  
  # Boards:
  #
diff --git a/configs/devices/riscv64-softmmu/default.mak b/configs/devices/riscv64-softmmu/default.mak
index bc69301fa4a6..3f6805944849 100644
--- a/configs/devices/riscv64-softmmu/default.mak
+++ b/configs/devices/riscv64-softmmu/default.mak
@@ -3,8 +3,6 @@
  # Uncomment the following lines to disable these optional devices:
  #
  #CONFIG_PCI_DEVICES=n
-CONFIG_SEMIHOSTING=y
-CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
  
  # Boards:
  #

Paolo

> Fixes: 29d9efca16 ("arm/Kconfig: Do not build TCG-only boards on a KVM-only build")
> Signed-off-by: Fabiano Rosas<farosas@suse.de>
> ---
>   target/arm/helper.c       | 4 ++--
>   target/arm/tcg/m_helper.c | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)

Re: [PATCH 1/3] target/arm: Use CONFIG_SEMIHOSTING instead of TCG for semihosting

[Peter Maydell]

On Thu, 4 May 2023 at 08:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 5/3/23 21:38, Fabiano Rosas wrote:
> > When building --without-default-devices, the semihosting code will not
> > be available, so check the proper config.

I think the changes to the ifdeffery are conceptually
fine (only do semihosting if it was configured in), but
it sounds like there's a separate problem here.
Whether we need semihosting depends on the accelerator (ie
"is it TCG or not"), not on what set of devices we're building.
So the problem seems to me to be that --without-default-devices
is causing the semihosting code not to be built in.

> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>
> for this change; however, there are two more related issues:
>
> 1) you still want to leave out the code if !TCG, because KVM is not able
> to exit to userspace on semihosting calls as far as I understand
>
> 2) I am not sure why CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y appears in
> config/targets/{arm,riscv32,riscv64}-softmmu/default.mak.

Because those are the architectures which have
"arm-compatible" semihosting ABIs ?

-- PMM

Re: [PATCH 1/3] target/arm: Use CONFIG_SEMIHOSTING instead of TCG for semihosting

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]

Il gio 4 mag 2023, 10:59 Peter Maydell <peter.maydell@linaro.org> ha
scritto:

> On Thu, 4 May 2023 at 08:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 5/3/23 21:38, Fabiano Rosas wrote:
> > > When building --without-default-devices, the semihosting code will not
> > > be available, so check the proper config.
>
> I think the changes to the ifdeffery are conceptually
> fine (only do semihosting if it was configured in), but
> it sounds like there's a separate problem here.
> Whether we need semihosting depends on the accelerator (ie
> "is it TCG or not"), not on what set of devices we're building.
> So the problem seems to me to be that --without-default-devices
> is causing the semihosting code not to be built in.
>
> > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> >
> > for this change; however, there are two more related issues:
> >
> > 1) you still want to leave out the code if !TCG, because KVM is not able
> > to exit to userspace on semihosting calls as far as I understand
> >
> > 2) I am not sure why CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y appears in
> > config/targets/{arm,riscv32,riscv64}-softmmu/default.mak.
>
> Because those are the architectures which have
> "arm-compatible" semihosting ABIs ?
>

Yes but is there a reason to do it in configs/ where all the other symbols
are boards, or was it just overlooked and a "default y" (as I suggested in
the previous reply) or "imply" is better?

The latter would be

config ARM_COMPATIBLE_SEMIHOSTING
    bool
    depends on TCG

and under "config ARM/RISCV32/RISCV64"

imply ARM_COMPATIBLE_SEMIHOSTING

Paolo

>
> -- PMM
>
>

[-- Attachment #2: Type: text/html, Size: 2789 bytes --]

Re: [PATCH 1/3] target/arm: Use CONFIG_SEMIHOSTING instead of TCG for semihosting

[Fabiano Rosas]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il gio 4 mag 2023, 10:59 Peter Maydell <peter.maydell@linaro.org> ha
> scritto:
>
>> On Thu, 4 May 2023 at 08:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> >
>> > On 5/3/23 21:38, Fabiano Rosas wrote:
>> > > When building --without-default-devices, the semihosting code will not
>> > > be available, so check the proper config.
>>
>> I think the changes to the ifdeffery are conceptually
>> fine (only do semihosting if it was configured in), but
>> it sounds like there's a separate problem here.
>> Whether we need semihosting depends on the accelerator (ie
>> "is it TCG or not"), not on what set of devices we're building.
>> So the problem seems to me to be that --without-default-devices
>> is causing the semihosting code not to be built in.
>>
>> > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>> >
>> > for this change; however, there are two more related issues:
>> >
>> > 1) you still want to leave out the code if !TCG, because KVM is not able
>> > to exit to userspace on semihosting calls as far as I understand
>> >
>> > 2) I am not sure why CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y appears in
>> > config/targets/{arm,riscv32,riscv64}-softmmu/default.mak.
>>
>> Because those are the architectures which have
>> "arm-compatible" semihosting ABIs ?
>>
>
> Yes but is there a reason to do it in configs/ where all the other symbols
> are boards, or was it just overlooked and a "default y" (as I suggested in
> the previous reply) or "imply" is better?

For arm it has been taken out of configs/ and moved into
target/arm/Kconfig:

...
# This config exists just so we can make SEMIHOSTING default when TCG
# is selected without also changing it for other architectures.
config ARM_SEMIHOSTING
    bool
    default y if TCG && ARM
    select ARM_COMPATIBLE_SEMIHOSTING

So I guess we'd need a similar change to what you suggested for ARM_V7M:

config ARM
    bool
    select ARM_V7M if TCG
    select ARM_COMPATIBLE_SEMIHOSTING if TCG

Re: [PATCH 1/3] target/arm: Use CONFIG_SEMIHOSTING instead of TCG for semihosting

[Paolo Bonzini]

On 5/4/23 15:01, Fabiano Rosas wrote:
> ...
> # This config exists just so we can make SEMIHOSTING default when TCG
> # is selected without also changing it for other architectures.
> config ARM_SEMIHOSTING
>      bool
>      default y if TCG && ARM
>      select ARM_COMPATIBLE_SEMIHOSTING

This can be replaced by "imply ARM_COMPATIBLE_SEMIHOSTING if TCG" placed 
under "config ARM" (and also RISCV32/RISCV64)".

Paolo

Re: [PATCH 1/3] target/arm: Use CONFIG_SEMIHOSTING instead of TCG for semihosting

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 5/3/23 21:38, Fabiano Rosas wrote:
>> When building --without-default-devices, the semihosting code will not
>> be available, so check the proper config.
>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>
> for this change; however, there are two more related issues:
>
> 1) you still want to leave out the code if !TCG, because KVM is not able
> to exit to userspace on semihosting calls as far as I understand

Correct.

> 2) I am not sure why CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y appears in
> config/targets/{arm,riscv32,riscv64}-softmmu/default.mak.

I think we need to be clearer in the development guide when things are
triggered by config/devices/ and when they are triggered by default
rules in Kconfig.

After Fabino's series the delta between
config/aarch64-softmmu/minimal.mk and config/aarch64-softmmu/default.mk
is pretty small.

I guess minimal.mk should be renamed to virt.mk, sbsa-ref dropped from
it (as you can't run it under KVM) and what controls which devices the
virt platform can use?

Should it be:

CONFIG_PCI_DEVICES=y
CONFIG_VIRTIO_PCI=y

to ensure we can plug and play all the various VirtIO bits we need for
the KVM/Xen use case?

>
> Putting things together you also need something like
>
> diff --git a/semihosting/Kconfig b/semihosting/Kconfig
> index eaf3a20ef5b2..671020a33426 100644
> --- a/semihosting/Kconfig
> +++ b/semihosting/Kconfig
> @@ -4,4 +4,5 @@ config SEMIHOSTING
>    config ARM_COMPATIBLE_SEMIHOSTING
>         bool
> +       default y if (ARM && TCG) || RISCV32 || RISCV64
>         select SEMIHOSTING
> diff --git a/configs/devices/arm-softmmu/default.mak b/configs/devices/arm-softmmu/default.mak
> index 1b49a7830c7e..5e7a17d05bf8 100644
> --- a/configs/devices/arm-softmmu/default.mak
> +++ b/configs/devices/arm-softmmu/default.mak
> @@ -41,5 +41,4 @@ CONFIG_FSL_IMX25=y
>  CONFIG_FSL_IMX7=y
>  CONFIG_FSL_IMX6UL=y
>  CONFIG_SEMIHOSTING=y
> -CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
>  CONFIG_ALLWINNER_H3=y
> diff --git a/configs/devices/riscv32-softmmu/default.mak b/configs/devices/riscv32-softmmu/default.mak
> index d847bd5692ec..94a236c9c25b 100644
> --- a/configs/devices/riscv32-softmmu/default.mak
> +++ b/configs/devices/riscv32-softmmu/default.mak
> @@ -3,8 +3,6 @@
>  # Uncomment the following lines to disable these optional devices:
>  #
>  #CONFIG_PCI_DEVICES=n
> -CONFIG_SEMIHOSTING=y
> -CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
>    # Boards:
>  #
> diff --git a/configs/devices/riscv64-softmmu/default.mak b/configs/devices/riscv64-softmmu/default.mak
> index bc69301fa4a6..3f6805944849 100644
> --- a/configs/devices/riscv64-softmmu/default.mak
> +++ b/configs/devices/riscv64-softmmu/default.mak
> @@ -3,8 +3,6 @@
>  # Uncomment the following lines to disable these optional devices:
>  #
>  #CONFIG_PCI_DEVICES=n
> -CONFIG_SEMIHOSTING=y
> -CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
>    # Boards:
>  #
>
> Paolo
>
>> Fixes: 29d9efca16 ("arm/Kconfig: Do not build TCG-only boards on a KVM-only build")
>> Signed-off-by: Fabiano Rosas<farosas@suse.de>
>> ---
>>   target/arm/helper.c       | 4 ++--
>>   target/arm/tcg/m_helper.c | 2 +-
>>   2 files changed, 3 insertions(+), 3 deletions(-)


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 2/3] target/arm: Add CONFIG_ARM_V7M back to default.mak

[Fabiano Rosas]

We cannot allow this config to be disabled at the moment as not all of
the relevant code is protected by it.

Commit 29d9efca16 ("arm/Kconfig: Do not build TCG-only boards on a
KVM-only build") moved the CONFIGs of several boards to Kconfig, so it
is now possible that nothing selects ARM_V7M (e.g. when doing a
--without-default-devices build).

Return the CONFIG_ARM_V7M entry to default.mak while we don't enable
the compilation without it. Note that this goes against the intention
of commit cd43648a44 ("hw/arm: move CONFIG_V7M out of
default-devices"), but at this point this is the smallest change we
can do.

Fixes: 29d9efca16 ("arm/Kconfig: Do not build TCG-only boards on a KVM-only build")
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 configs/devices/arm-softmmu/default.mak | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/devices/arm-softmmu/default.mak b/configs/devices/arm-softmmu/default.mak
index 647fbce88d..0c2b24d6bb 100644
--- a/configs/devices/arm-softmmu/default.mak
+++ b/configs/devices/arm-softmmu/default.mak
@@ -4,3 +4,4 @@
 # CONFIG_TEST_DEVICES=n
 
 CONFIG_ARM_VIRT=y
+CONFIG_ARM_V7M=y
-- 
2.35.3

Re: [PATCH 2/3] target/arm: Add CONFIG_ARM_V7M back to default.mak

[Paolo Bonzini]

On 5/3/23 21:38, Fabiano Rosas wrote:
> We cannot allow this config to be disabled at the moment as not all of
> the relevant code is protected by it.
> 
> Commit 29d9efca16 ("arm/Kconfig: Do not build TCG-only boards on a
> KVM-only build") moved the CONFIGs of several boards to Kconfig, so it
> is now possible that nothing selects ARM_V7M (e.g. when doing a
> --without-default-devices build).
> 
> Return the CONFIG_ARM_V7M entry to default.mak while we don't enable
> the compilation without it. Note that this goes against the intention
> of commit cd43648a44 ("hw/arm: move CONFIG_V7M out of
> default-devices"), but at this point this is the smallest change we
> can do.

If this is a dependency of target/arm/tcg/translate.c on ARM_V7M, it 
should be written as

diff --git a/target/arm/Kconfig b/target/arm/Kconfig
index 3f3394a22b23..498bdba1e139 100644
--- a/target/arm/Kconfig
+++ b/target/arm/Kconfig
@@ -1,5 +1,6 @@
  config ARM
      bool
+    select ARM_V7M if TCG

  config AARCH64
      bool


Paolo

> Fixes: 29d9efca16 ("arm/Kconfig: Do not build TCG-only boards on a KVM-only build")
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   configs/devices/arm-softmmu/default.mak | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/configs/devices/arm-softmmu/default.mak b/configs/devices/arm-softmmu/default.mak
> index 647fbce88d..0c2b24d6bb 100644
> --- a/configs/devices/arm-softmmu/default.mak
> +++ b/configs/devices/arm-softmmu/default.mak
> @@ -4,3 +4,4 @@
>   # CONFIG_TEST_DEVICES=n
>   
>   CONFIG_ARM_VIRT=y
> +CONFIG_ARM_V7M=y

[PATCH 3/3] tests/qtest: Don't run cdrom tests if no accelerator is present

[Fabiano Rosas]

On a build configured with: --disable-tcg --enable-xen it is possible
to produce a QEMU binary with no TCG nor KVM support. Skip the test if
that's the case.

Fixes: 0c1ae3ff9d ("tests/qtest: Fix tests when no KVM or TCG are present")
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/cdrom-test.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
index 26a2400181..09655e6ff0 100644
--- a/tests/qtest/cdrom-test.c
+++ b/tests/qtest/cdrom-test.c
@@ -205,6 +205,11 @@ int main(int argc, char **argv)
 
     g_test_init(&argc, &argv, NULL);
 
+    if (!qtest_has_accel("tcg") && !qtest_has_accel("kvm")) {
+        g_test_skip("No KVM or TCG accelerator available");
+        return 0;
+    }
+
     if (exec_genisoimg(genisocheck)) {
         /* genisoimage not available - so can't run tests */
         return g_test_run();
-- 
2.35.3

Re: [PATCH 3/3] tests/qtest: Don't run cdrom tests if no accelerator is present

[Paolo Bonzini]

On 5/3/23 21:38, Fabiano Rosas wrote:
> On a build configured with: --disable-tcg --enable-xen it is possible
> to produce a QEMU binary with no TCG nor KVM support. Skip the test if
> that's the case.
> 
> Fixes: 0c1ae3ff9d ("tests/qtest: Fix tests when no KVM or TCG are present")
> Signed-off-by: Fabiano Rosas<farosas@suse.de>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>