hw/xtensa: require libfdt

[PATCH] hw/xtensa: require libfdt

Posted by Paolo Bonzini 10 months, 1 week ago

Always allow -dtb in qemu-system-xtensa.  Basically all other targets require
it if it can be used (including for example i386/x86_64).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 configs/targets/xtensa-softmmu.mak   | 1 +
 configs/targets/xtensaeb-softmmu.mak | 1 +
 hw/xtensa/xtfpga.c                   | 9 ---------
 3 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/configs/targets/xtensa-softmmu.mak b/configs/targets/xtensa-softmmu.mak
index f075557bfa9..c394df73034 100644
--- a/configs/targets/xtensa-softmmu.mak
+++ b/configs/targets/xtensa-softmmu.mak
@@ -1,2 +1,3 @@
 TARGET_ARCH=xtensa
 TARGET_SUPPORTS_MTTCG=y
+TARGET_NEED_FDT=y
diff --git a/configs/targets/xtensaeb-softmmu.mak b/configs/targets/xtensaeb-softmmu.mak
index b02e11b8200..517b4c3e12d 100644
--- a/configs/targets/xtensaeb-softmmu.mak
+++ b/configs/targets/xtensaeb-softmmu.mak
@@ -1,3 +1,4 @@
 TARGET_ARCH=xtensa
 TARGET_BIG_ENDIAN=y
 TARGET_SUPPORTS_MTTCG=y
+TARGET_NEED_FDT=y
diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index fbad1c83a3f..3c93cfffbaa 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -357,7 +357,6 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
             cur_tagptr = put_tag(cur_tagptr, BP_TAG_COMMAND_LINE,
                                  strlen(kernel_cmdline) + 1, kernel_cmdline);
         }
-#ifdef CONFIG_FDT
         if (dtb_filename) {
             int fdt_size;
             void *fdt = load_device_tree(dtb_filename, &fdt_size);
@@ -374,14 +373,6 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
             cur_lowmem = QEMU_ALIGN_UP(cur_lowmem + fdt_size, 4 * KiB);
             g_free(fdt);
         }
-#else
-        if (dtb_filename) {
-            error_report("could not load DTB '%s': "
-                         "FDT support is not configured in QEMU",
-                         dtb_filename);
-            exit(EXIT_FAILURE);
-        }
-#endif
         if (initrd_filename) {
             BpMemInfo initrd_location = { 0 };
             int initrd_size = load_ramdisk(initrd_filename, cur_lowmem,
-- 
2.43.0

Re: [PATCH] hw/xtensa: require libfdt

Posted by Thomas Huth 10 months, 1 week ago

On 22/01/2024 10.42, Paolo Bonzini wrote:
> Always allow -dtb in qemu-system-xtensa.  Basically all other targets require
> it if it can be used (including for example i386/x86_64).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   configs/targets/xtensa-softmmu.mak   | 1 +
>   configs/targets/xtensaeb-softmmu.mak | 1 +
>   hw/xtensa/xtfpga.c                   | 9 ---------
>   3 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/configs/targets/xtensa-softmmu.mak b/configs/targets/xtensa-softmmu.mak
> index f075557bfa9..c394df73034 100644
> --- a/configs/targets/xtensa-softmmu.mak
> +++ b/configs/targets/xtensa-softmmu.mak
> @@ -1,2 +1,3 @@
>   TARGET_ARCH=xtensa
>   TARGET_SUPPORTS_MTTCG=y
> +TARGET_NEED_FDT=y
> diff --git a/configs/targets/xtensaeb-softmmu.mak b/configs/targets/xtensaeb-softmmu.mak
> index b02e11b8200..517b4c3e12d 100644
> --- a/configs/targets/xtensaeb-softmmu.mak
> +++ b/configs/targets/xtensaeb-softmmu.mak
> @@ -1,3 +1,4 @@
>   TARGET_ARCH=xtensa
>   TARGET_BIG_ENDIAN=y
>   TARGET_SUPPORTS_MTTCG=y
> +TARGET_NEED_FDT=y
> diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
> index fbad1c83a3f..3c93cfffbaa 100644
> --- a/hw/xtensa/xtfpga.c
> +++ b/hw/xtensa/xtfpga.c
> @@ -357,7 +357,6 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
>               cur_tagptr = put_tag(cur_tagptr, BP_TAG_COMMAND_LINE,
>                                    strlen(kernel_cmdline) + 1, kernel_cmdline);
>           }
> -#ifdef CONFIG_FDT
>           if (dtb_filename) {
>               int fdt_size;
>               void *fdt = load_device_tree(dtb_filename, &fdt_size);
> @@ -374,14 +373,6 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
>               cur_lowmem = QEMU_ALIGN_UP(cur_lowmem + fdt_size, 4 * KiB);
>               g_free(fdt);
>           }
> -#else
> -        if (dtb_filename) {
> -            error_report("could not load DTB '%s': "
> -                         "FDT support is not configured in QEMU",
> -                         dtb_filename);
> -            exit(EXIT_FAILURE);
> -        }
> -#endif

Agreed, we should do this the same way on all architectures.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] hw/xtensa: require libfdt

Posted by Philippe Mathieu-Daudé 10 months, 1 week ago

Hi Paolo,

On 22/1/24 10:42, Paolo Bonzini wrote:
> Always allow -dtb in qemu-system-xtensa.  Basically all other targets require
> it if it can be used (including for example i386/x86_64).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   configs/targets/xtensa-softmmu.mak   | 1 +
>   configs/targets/xtensaeb-softmmu.mak | 1 +
>   hw/xtensa/xtfpga.c                   | 9 ---------
>   3 files changed, 2 insertions(+), 9 deletions(-)

I've been wondering for some time why not requires libfdt
for all sysemu targets. It gives some pain with MIPS machines,
and I wonder if there is any value in not including it.

Re: [PATCH] hw/xtensa: require libfdt

Posted by Peter Maydell 10 months, 1 week ago

On Mon, 22 Jan 2024 at 11:10, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> On 22/1/24 10:42, Paolo Bonzini wrote:
> > Always allow -dtb in qemu-system-xtensa.  Basically all other targets require
> > it if it can be used (including for example i386/x86_64).

> I've been wondering for some time why not requires libfdt
> for all sysemu targets. It gives some pain with MIPS machines,
> and I wonder if there is any value in not including it.

I think traditionally we wanted to avoid the dependency
for the common case of "I only care about x86 guests",
back when fdt was mostly limited to ppc or arm, but since
these days even x86 sets TARGET_NEED_FDT there's something
to be said for just making it obligatory.

Guest archs which *don't* use fdt:
 alpha cris hppa m68k s390x sh4 sparc tricore xtensa.

Of those, s390x is the only one with KVM support and
so the only one where maybe somebody might care about
the extra dependency for security auditing purposes.
But given it's already in x86 binaries I don't think that's
a very strong argument. The rest are all "minor" architectures.
So if making fdt mandatory makes our lives easier for
development purposes I think we should go ahead and do it.

(mips is a bit odd because only mips64el-softmmu sets
TARGET_NEED_FDT, not the other mips configs. Since upstream
Linux has an arch/mips/boot/dts/mti/malta.dts, presumably
in theory the 32-bit boards also ought to support -dtb and
friends; though CONFIG_MIPS_RAW_APPENDED_DTB makes this
not a problem in practice.)

thanks
-- PMM