| Message ID | 20230703034614.3909079-1-linux@roeck-us.net |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: Generate devicetree only after machine initialization is complete | expand |
On 3/7/23 05:46, Guenter Roeck wrote: > If the devicetree is created before machine initialization is complete, > it misses dynamic devices. Specifically, the tpm device is not added > to the devicetree file and is therefore not instantiated in Linux. > Create devicetree in virt_machine_done() to solve the problem. This makes sense, but what about the other archs/machines? Shouldn't we fix this generically? > Cc: Alistair Francis <alistair23@gmail.com> > Fixes: 325b7c4e75 hw/riscv: Enable TPM backends > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > hw/riscv/virt.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index ed4c27487e..08876284f5 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -1248,6 +1248,11 @@ static void virt_machine_done(Notifier *notifier, void *data) > uint64_t kernel_entry = 0; > BlockBackend *pflash_blk0; > > + /* create devicetree if not provided */ > + if (!machine->dtb) { > + create_fdt(s, memmap); > + } > + > /* > * Only direct boot kernel is currently supported for KVM VM, > * so the "-bios" parameter is not supported when KVM is enabled. > @@ -1508,15 +1513,13 @@ static void virt_machine_init(MachineState *machine) > } > virt_flash_map(s, system_memory); > > - /* load/create device tree */ > + /* load device tree */ > if (machine->dtb) { > machine->fdt = load_device_tree(machine->dtb, &s->fdt_size); > if (!machine->fdt) { > error_report("load_device_tree() failed"); > exit(1); > } > - } else { > - create_fdt(s, memmap); > } > > s->machine_done.notify = virt_machine_done;
On 7/3/23 00:46, Philippe Mathieu-Daudé wrote: > On 3/7/23 05:46, Guenter Roeck wrote: >> If the devicetree is created before machine initialization is complete, >> it misses dynamic devices. Specifically, the tpm device is not added >> to the devicetree file and is therefore not instantiated in Linux. >> Create devicetree in virt_machine_done() to solve the problem. > > This makes sense, but what about the other archs/machines? > Shouldn't we fix this generically? > I had a brief look into various implementations. To me the code looks all very machine specific. I don't think it can be solved generically (if other machines are even affected), but then I am not a qemu expert and may be wrong. Guenter >> Cc: Alistair Francis <alistair23@gmail.com> >> Fixes: 325b7c4e75 hw/riscv: Enable TPM backends >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> hw/riscv/virt.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c >> index ed4c27487e..08876284f5 100644 >> --- a/hw/riscv/virt.c >> +++ b/hw/riscv/virt.c >> @@ -1248,6 +1248,11 @@ static void virt_machine_done(Notifier *notifier, void *data) >> uint64_t kernel_entry = 0; >> BlockBackend *pflash_blk0; >> + /* create devicetree if not provided */ >> + if (!machine->dtb) { >> + create_fdt(s, memmap); >> + } >> + >> /* >> * Only direct boot kernel is currently supported for KVM VM, >> * so the "-bios" parameter is not supported when KVM is enabled. >> @@ -1508,15 +1513,13 @@ static void virt_machine_init(MachineState *machine) >> } >> virt_flash_map(s, system_memory); >> - /* load/create device tree */ >> + /* load device tree */ >> if (machine->dtb) { >> machine->fdt = load_device_tree(machine->dtb, &s->fdt_size); >> if (!machine->fdt) { >> error_report("load_device_tree() failed"); >> exit(1); >> } >> - } else { >> - create_fdt(s, memmap); >> } >> s->machine_done.notify = virt_machine_done; >
On 7/3/23 04:46, Philippe Mathieu-Daudé wrote: > On 3/7/23 05:46, Guenter Roeck wrote: >> If the devicetree is created before machine initialization is complete, >> it misses dynamic devices. Specifically, the tpm device is not added >> to the devicetree file and is therefore not instantiated in Linux. >> Create devicetree in virt_machine_done() to solve the problem. > > This makes sense, but what about the other archs/machines? > Shouldn't we fix this generically? As far as other archs goes I can say that ARM isn't affected by it because the fdt creation is done by arm_load_dtb() during virt_machine_done time. I'm not aware of how x86 handles TPM. And pseries/ppc64 does a completely different thing (per usual). Inside hw/riscv the only TPM capable board is 'virt'. So I think this patch has an adequate scope. Thanks, Daniel > >> Cc: Alistair Francis <alistair23@gmail.com> >> Fixes: 325b7c4e75 hw/riscv: Enable TPM backends >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> hw/riscv/virt.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c >> index ed4c27487e..08876284f5 100644 >> --- a/hw/riscv/virt.c >> +++ b/hw/riscv/virt.c >> @@ -1248,6 +1248,11 @@ static void virt_machine_done(Notifier *notifier, void *data) >> uint64_t kernel_entry = 0; >> BlockBackend *pflash_blk0; >> + /* create devicetree if not provided */ >> + if (!machine->dtb) { >> + create_fdt(s, memmap); >> + } >> + >> /* >> * Only direct boot kernel is currently supported for KVM VM, >> * so the "-bios" parameter is not supported when KVM is enabled. >> @@ -1508,15 +1513,13 @@ static void virt_machine_init(MachineState *machine) >> } >> virt_flash_map(s, system_memory); >> - /* load/create device tree */ >> + /* load device tree */ >> if (machine->dtb) { >> machine->fdt = load_device_tree(machine->dtb, &s->fdt_size); >> if (!machine->fdt) { >> error_report("load_device_tree() failed"); >> exit(1); >> } >> - } else { >> - create_fdt(s, memmap); >> } >> s->machine_done.notify = virt_machine_done; >
On 7/3/23 00:46, Guenter Roeck wrote: > If the devicetree is created before machine initialization is complete, > it misses dynamic devices. Specifically, the tpm device is not added > to the devicetree file and is therefore not instantiated in Linux. > Create devicetree in virt_machine_done() to solve the problem. > > Cc: Alistair Francis <alistair23@gmail.com> > Fixes: 325b7c4e75 hw/riscv: Enable TPM backends > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > hw/riscv/virt.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index ed4c27487e..08876284f5 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -1248,6 +1248,11 @@ static void virt_machine_done(Notifier *notifier, void *data) > uint64_t kernel_entry = 0; > BlockBackend *pflash_blk0; > > + /* create devicetree if not provided */ > + if (!machine->dtb) { > + create_fdt(s, memmap); > + } > + I suggest moving the entire load/create DT code from virt_machine_init() to the start of virt_machine_done(): /* load/create device tree */ if (machine->dtb) { machine->fdt = load_device_tree(machine->dtb, &s->fdt_size); if (!machine->fdt) { error_report("load_device_tree() failed"); exit(1); } } else { create_fdt(s, memmap); } This way we don't have to look in to 2 different functions to wonder what happens in case machine->dtb is NULL. Thanks, Daniel > /* > * Only direct boot kernel is currently supported for KVM VM, > * so the "-bios" parameter is not supported when KVM is enabled. > @@ -1508,15 +1513,13 @@ static void virt_machine_init(MachineState *machine) > } > virt_flash_map(s, system_memory); > > - /* load/create device tree */ > + /* load device tree */ > if (machine->dtb) { > machine->fdt = load_device_tree(machine->dtb, &s->fdt_size); > if (!machine->fdt) { > error_report("load_device_tree() failed"); > exit(1); > } > - } else { > - create_fdt(s, memmap); > } > > s->machine_done.notify = virt_machine_done;
On 7/3/23 12:25, Daniel Henrique Barboza wrote: > On 7/3/23 00:46, Guenter Roeck wrote: >> If the devicetree is created before machine initialization is complete, >> it misses dynamic devices. Specifically, the tpm device is not added >> to the devicetree file and is therefore not instantiated in Linux. >> Create devicetree in virt_machine_done() to solve the problem. >> >> Cc: Alistair Francis <alistair23@gmail.com> >> Fixes: 325b7c4e75 hw/riscv: Enable TPM backends >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> hw/riscv/virt.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c >> index ed4c27487e..08876284f5 100644 >> --- a/hw/riscv/virt.c >> +++ b/hw/riscv/virt.c >> @@ -1248,6 +1248,11 @@ static void virt_machine_done(Notifier *notifier, void *data) >> uint64_t kernel_entry = 0; >> BlockBackend *pflash_blk0; >> + /* create devicetree if not provided */ >> + if (!machine->dtb) { >> + create_fdt(s, memmap); >> + } >> + > > I suggest moving the entire load/create DT code from virt_machine_init() to > the start of virt_machine_done(): > > /* load/create device tree */ > if (machine->dtb) { > machine->fdt = load_device_tree(machine->dtb, &s->fdt_size); > if (!machine->fdt) { > error_report("load_device_tree() failed"); > exit(1); > } > } else { > create_fdt(s, memmap); > } > > This way we don't have to look in to 2 different functions to wonder what happens > in case machine->dtb is NULL. > I can do that, but I don't know how to test it. Is there a working dtb/machine combination for riscv which would let me test loading a devicetree file ? Guenter
On 7/3/23 18:18, Guenter Roeck wrote: > On 7/3/23 12:25, Daniel Henrique Barboza wrote: >> On 7/3/23 00:46, Guenter Roeck wrote: >>> If the devicetree is created before machine initialization is complete, >>> it misses dynamic devices. Specifically, the tpm device is not added >>> to the devicetree file and is therefore not instantiated in Linux. >>> Create devicetree in virt_machine_done() to solve the problem. >>> >>> Cc: Alistair Francis <alistair23@gmail.com> >>> Fixes: 325b7c4e75 hw/riscv: Enable TPM backends >>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>> --- >>> hw/riscv/virt.c | 9 ++++++--- >>> 1 file changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c >>> index ed4c27487e..08876284f5 100644 >>> --- a/hw/riscv/virt.c >>> +++ b/hw/riscv/virt.c >>> @@ -1248,6 +1248,11 @@ static void virt_machine_done(Notifier *notifier, void *data) >>> uint64_t kernel_entry = 0; >>> BlockBackend *pflash_blk0; >>> + /* create devicetree if not provided */ >>> + if (!machine->dtb) { >>> + create_fdt(s, memmap); >>> + } >>> + >> >> I suggest moving the entire load/create DT code from virt_machine_init() to >> the start of virt_machine_done(): >> >> /* load/create device tree */ >> if (machine->dtb) { >> machine->fdt = load_device_tree(machine->dtb, &s->fdt_size); >> if (!machine->fdt) { >> error_report("load_device_tree() failed"); >> exit(1); >> } >> } else { >> create_fdt(s, memmap); >> } >> >> This way we don't have to look in to 2 different functions to wonder what happens >> in case machine->dtb is NULL. >> > > I can do that, but I don't know how to test it. Is there a working dtb/machine > combination for riscv which would let me test loading a devicetree file ? I recommend using your own setup with TPM (I'm assuming you're using a TPM setup), generate a .dtb from it, and then launch it using '-dtb'. First you need the patch applied (otherwise there won't be a TPM in the FDT). After that, relaunch the same machine again but appending in the end of the command line: -machine dumpdtb=file.dtb This will create a 'file.dtb' file in the working dir and exit. After that re-launch the machine again but now append: -dtb file.dtb And you should be able to boot a 'virt' machine with TPM support. Thanks, Daniel > > Guenter >
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index ed4c27487e..08876284f5 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -1248,6 +1248,11 @@ static void virt_machine_done(Notifier *notifier, void *data) uint64_t kernel_entry = 0; BlockBackend *pflash_blk0; + /* create devicetree if not provided */ + if (!machine->dtb) { + create_fdt(s, memmap); + } + /* * Only direct boot kernel is currently supported for KVM VM, * so the "-bios" parameter is not supported when KVM is enabled. @@ -1508,15 +1513,13 @@ static void virt_machine_init(MachineState *machine) } virt_flash_map(s, system_memory); - /* load/create device tree */ + /* load device tree */ if (machine->dtb) { machine->fdt = load_device_tree(machine->dtb, &s->fdt_size); if (!machine->fdt) { error_report("load_device_tree() failed"); exit(1); } - } else { - create_fdt(s, memmap); } s->machine_done.notify = virt_machine_done;
If the devicetree is created before machine initialization is complete, it misses dynamic devices. Specifically, the tpm device is not added to the devicetree file and is therefore not instantiated in Linux. Create devicetree in virt_machine_done() to solve the problem. Cc: Alistair Francis <alistair23@gmail.com> Fixes: 325b7c4e75 hw/riscv: Enable TPM backends Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- hw/riscv/virt.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)