Peter, Daniel offers two ways to fix this bug (see below). Got a
preference?
Daniel Henrique Barboza <dbarb...@ventanamicro.com> writes:
> On 3/23/23 03:29, Markus Armbruster wrote:
>> Watch this:
>>
>> $ gdb --args ../qemu/bld/qemu-system-aarch64 -S -M virt -display none
>> -qmp stdio
>> [...]
>> (gdb) r
>> [...]
>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 7},
>> "package": "v7.2.0-2331-gda89f78a7d"}, "capabilities": ["oob"]}}
>> [New Thread 0x7fffed62c6c0 (LWP 1021967)]
>> {"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}
>> {"return": {}}
>> {"execute": "dumpdtb", "arguments": {"filename": "fdt.dtb"}}
>>
>> Thread 1 "qemu-system-aar" received signal SIGSEGV, Segmentation fault.
>> qmp_dumpdtb (filename=0x5555581c5170 "fdt.dtb",
>> errp=errp@entry=0x7fffffffdae8)
>> at ../softmmu/device_tree.c:661
>> 661 size = fdt_totalsize(current_machine->fdt);
>>
>> current_machine->fdt is non-null here. The crash is within
>> fdt_totalsize().
>
>
> Back when I added this command [1] one of the patches of this series was:
>
> [PATCH v8 03/16] hw/arm: do not free machine->fdt in arm_load_dtb()
>
> https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg04201.html
>
> The patch aimed to address what you're seeing here. machine->fdt is not NULL,
> but it was freed in arm_load_dtb() without assigning it to NULL and it
> contains
> junk.
>
> Back then this patch got no acks/reviews and got left behind. If I apply it
> now
> at current master your example works.
>
>>
>> I suspect ...
>>
>> void qmp_dumpdtb(const char *filename, Error **errp)
>> {
>> g_autoptr(GError) err = NULL;
>> uint32_t size;
>>
>> if (!current_machine->fdt) {
>> error_setg(errp, "This machine doesn't have a FDT");
>> return;
>> }
>>
>> ... we're missing an "FDT isn't ready" guard here.
>
>
> There might be a case where a guard would be needed, but for all ARM machines
> that
> uses arm_load_dtb() I can say that the dumpdtb is broken regardless of
> whether you
> attempt it during early boot or not.
>
> One solution is to just apply the patch I mentioned above. Another solution
> is to
> make a g_free(fdt) in arm_load_dtb and also do a ms->fdt = NULL to tell
> dumpdtb()
> that there is no fdt available.
>
> I don't see any particular reason why arm machines can't support this
> command, so
> let me know and I can re-send that patch.
>
>
>
> Thanks,
>
>
> Daniel
>
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg04190.html
>
>>
>> size = fdt_totalsize(current_machine->fdt);
>>
>> g_assert(size > 0);
>>
>> if (!g_file_set_contents(filename, current_machine->fdt, size,
>> &err)) {
>> error_setg(errp, "Error saving FDT to file %s: %s",
>> filename, err->message);
>> }
>> }
>>
>> Also, I think the error message "does not have a FDT" should say "an
>> FDT".
>>
