riscv: Generate devicetree only after machine initialization is complete

[PATCH] riscv: Generate devicetree only after machine initialization is complete

Posted by Guenter Roeck 10 months ago

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);
+    }
+
     /*
      * 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;
-- 
2.39.2

Re: [PATCH] riscv: Generate devicetree only after machine initialization is complete

Posted by Daniel Henrique Barboza 10 months ago

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;

Re: [PATCH] riscv: Generate devicetree only after machine initialization is complete

Posted by Guenter Roeck 10 months ago

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

Re: [PATCH] riscv: Generate devicetree only after machine initialization is complete

Posted by Daniel Henrique Barboza 10 months ago


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
>

Re: [PATCH] riscv: Generate devicetree only after machine initialization is complete

Posted by Philippe Mathieu-Daudé 10 months ago

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;

Re: [PATCH] riscv: Generate devicetree only after machine initialization is complete

Posted by Guenter Roeck 10 months ago

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;
>

Re: [PATCH] riscv: Generate devicetree only after machine initialization is complete

Posted by Daniel Henrique Barboza 10 months ago


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;
>