[PATCH] monitor: add dumpdtb command only in device-tree-enabled targets

[PATCH] monitor: add dumpdtb command only in device-tree-enabled targets

[Paolo Bonzini]

Remove the command altogether from targets that do not have device tree support,
instead of leaving it nonfunctional.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build        | 2 --
 qapi/machine.json  | 2 +-
 hmp-commands.hx    | 2 +-
 system/meson.build | 2 +-
 4 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/meson.build b/meson.build
index de8e5ad67dc..6bf7f3eb685 100644
--- a/meson.build
+++ b/meson.build
@@ -3126,8 +3126,6 @@ if fdt_required.length() > 0 or get_option('fdt').enabled()
   endif
 endif
 
-config_host_data.set('CONFIG_FDT', fdt.found())
-
 vhost_user = not_found
 if host_os == 'linux' and have_vhost_user
   libvhost_user = subproject('libvhost-user')
diff --git a/qapi/machine.json b/qapi/machine.json
index b6d634b30d5..c5e40857ba4 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1828,4 +1828,4 @@
 ##
 { 'command': 'dumpdtb',
   'data': { 'filename': 'str' },
-  'if': 'CONFIG_FDT' }
+  'if': 'TARGET_NEEDS_FDT' }
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 17b5ea839d9..f7263957240 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1819,7 +1819,7 @@ ERST
         .flags      = "p",
     },
 
-#if defined(CONFIG_FDT)
+#if defined(TARGET_NEED_FDT)
     {
         .name       = "dumpdtb",
         .args_type  = "filename:F",
diff --git a/system/meson.build b/system/meson.build
index 25e21172505..4e54f5d1a4b 100644
--- a/system/meson.build
+++ b/system/meson.build
@@ -32,7 +32,7 @@ if have_tpm
 endif
 
 system_ss.add(when: seccomp, if_true: files('qemu-seccomp.c'))
-system_ss.add(when: fdt, if_true: files('device_tree.c'))
+system_ss.add(when: 'TARGET_NEED_FDT', if_true: [fdt, files('device_tree.c')])
 if host_os == 'linux'
   system_ss.add(files('async-teardown.c'))
 endif
-- 
2.43.0

Re: [PATCH] monitor: add dumpdtb command only in device-tree-enabled targets

[Thomas Huth]

On 22/01/2024 10.24, Paolo Bonzini wrote:
> Remove the command altogether from targets that do not have device tree support,
> instead of leaving it nonfunctional.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   meson.build        | 2 --
>   qapi/machine.json  | 2 +-
>   hmp-commands.hx    | 2 +-
>   system/meson.build | 2 +-
>   4 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index de8e5ad67dc..6bf7f3eb685 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3126,8 +3126,6 @@ if fdt_required.length() > 0 or get_option('fdt').enabled()
>     endif
>   endif
>   
> -config_host_data.set('CONFIG_FDT', fdt.found())
> -
>   vhost_user = not_found
>   if host_os == 'linux' and have_vhost_user
>     libvhost_user = subproject('libvhost-user')
> diff --git a/qapi/machine.json b/qapi/machine.json
> index b6d634b30d5..c5e40857ba4 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1828,4 +1828,4 @@
>   ##
>   { 'command': 'dumpdtb',
>     'data': { 'filename': 'str' },
> -  'if': 'CONFIG_FDT' }
> +  'if': 'TARGET_NEEDS_FDT' }
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 17b5ea839d9..f7263957240 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1819,7 +1819,7 @@ ERST
>           .flags      = "p",
>       },
>   
> -#if defined(CONFIG_FDT)
> +#if defined(TARGET_NEED_FDT)
>       {
>           .name       = "dumpdtb",
>           .args_type  = "filename:F",
> diff --git a/system/meson.build b/system/meson.build
> index 25e21172505..4e54f5d1a4b 100644
> --- a/system/meson.build
> +++ b/system/meson.build
> @@ -32,7 +32,7 @@ if have_tpm
>   endif
>   
>   system_ss.add(when: seccomp, if_true: files('qemu-seccomp.c'))
> -system_ss.add(when: fdt, if_true: files('device_tree.c'))
> +system_ss.add(when: 'TARGET_NEED_FDT', if_true: [fdt, files('device_tree.c')])

IIUC this does not work. For example, I applied your patch and tried to 
"make qemu-system-xtensa", I got:

/usr/bin/ld: libcommon.fa.p/hw_core_guest-loader.c.o: in function 
`loader_insert_platform_data':
/home/thuth/tmp/qemu-build/../../devel/qemu/hw/core/guest-loader.c:55: 
undefined reference to `qemu_fdt_add_subnode'
/usr/bin/ld: 
/home/thuth/tmp/qemu-build/../../devel/qemu/hw/core/guest-loader.c:56: 
undefined reference to `qemu_fdt_setprop'
/usr/bin/ld: 
/home/thuth/tmp/qemu-build/../../devel/qemu/hw/core/guest-loader.c:60: 
undefined reference to `qemu_fdt_setprop_string_array'
/usr/bin/ld: 
/home/thuth/tmp/qemu-build/../../devel/qemu/hw/core/guest-loader.c:67: 
undefined reference to `qemu_fdt_setprop_string'
/usr/bin/ld: 
/home/thuth/tmp/qemu-build/../../devel/qemu/hw/core/guest-loader.c:73: 
undefined reference to `qemu_fdt_setprop_string_array'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

system_ss is a common set, so you must not use a TARGET_* switch for 
deciding what goes into that set, or did I get that wrong? I.e. I think you 
have to drop the system/meson.build change here?

Also, there is another #ifdef CONFIG_FDT in hw/xtensa/xtfpga.c which likely 
needs some treatment, too?

  Thomas

Re: [PATCH] monitor: add dumpdtb command only in device-tree-enabled targets

[Paolo Bonzini]

On Mon, Jan 22, 2024 at 2:40 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 22/01/2024 10.24, Paolo Bonzini wrote:
> > Remove the command altogether from targets that do not have device tree support,
> > instead of leaving it nonfunctional.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   meson.build        | 2 --
> >   qapi/machine.json  | 2 +-
> >   hmp-commands.hx    | 2 +-
> >   system/meson.build | 2 +-
> >   4 files changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/meson.build b/meson.build
> > index de8e5ad67dc..6bf7f3eb685 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -3126,8 +3126,6 @@ if fdt_required.length() > 0 or get_option('fdt').enabled()
> >     endif
> >   endif
> >
> > -config_host_data.set('CONFIG_FDT', fdt.found())
> > -
> >   vhost_user = not_found
> >   if host_os == 'linux' and have_vhost_user
> >     libvhost_user = subproject('libvhost-user')
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index b6d634b30d5..c5e40857ba4 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -1828,4 +1828,4 @@
> >   ##
> >   { 'command': 'dumpdtb',
> >     'data': { 'filename': 'str' },
> > -  'if': 'CONFIG_FDT' }
> > +  'if': 'TARGET_NEEDS_FDT' }
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index 17b5ea839d9..f7263957240 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -1819,7 +1819,7 @@ ERST
> >           .flags      = "p",
> >       },
> >
> > -#if defined(CONFIG_FDT)
> > +#if defined(TARGET_NEED_FDT)
> >       {
> >           .name       = "dumpdtb",
> >           .args_type  = "filename:F",
> > diff --git a/system/meson.build b/system/meson.build
> > index 25e21172505..4e54f5d1a4b 100644
> > --- a/system/meson.build
> > +++ b/system/meson.build
> > @@ -32,7 +32,7 @@ if have_tpm
> >   endif
> >
> >   system_ss.add(when: seccomp, if_true: files('qemu-seccomp.c'))
> > -system_ss.add(when: fdt, if_true: files('device_tree.c'))
> > +system_ss.add(when: 'TARGET_NEED_FDT', if_true: [fdt, files('device_tree.c')])
>
> IIUC this does not work. For example, I applied your patch and tried to
> "make qemu-system-xtensa", I got:

xtensa is covered by the other patch I sent, but I (incorrectly)
thought these two would be independent and didn't send them as a
single thread.

> /usr/bin/ld: libcommon.fa.p/hw_core_guest-loader.c.o: in function
> `loader_insert_platform_data':
> /home/thuth/tmp/qemu-build/../../devel/qemu/hw/core/guest-loader.c:55:
> undefined reference to `qemu_fdt_add_subnode'
> /usr/bin/ld:
> /home/thuth/tmp/qemu-build/../../devel/qemu/hw/core/guest-loader.c:56:
> undefined reference to `qemu_fdt_setprop'
> /usr/bin/ld:
> /home/thuth/tmp/qemu-build/../../devel/qemu/hw/core/guest-loader.c:60:
> undefined reference to `qemu_fdt_setprop_string_array'
> /usr/bin/ld:
> /home/thuth/tmp/qemu-build/../../devel/qemu/hw/core/guest-loader.c:67:
> undefined reference to `qemu_fdt_setprop_string'
> /usr/bin/ld:
> /home/thuth/tmp/qemu-build/../../devel/qemu/hw/core/guest-loader.c:73:
> undefined reference to `qemu_fdt_setprop_string_array'
> collect2: error: ld returned 1 exit status
> ninja: build stopped: subcommand failed.
>
> system_ss is a common set, so you must not use a TARGET_* switch for
> deciding what goes into that set, or did I get that wrong?

You can use it; it's compiled once, and then (based on the per-target
configuration) the executable will or will not include the files that
depend on it, similar to devices.

Perhaps however it makes sense to turn it into a Kconfig symbol, as
that would make the use more clear. Or based on Phillippe's
suggestion, enabling device tree everywhere might make sense too.

Paolo