Re: [Qemu-devel] [PATCH] ppc: virtex_ml507: QEMU_OPTION_dtb support for this machine.

[Alexander Graf]

On 14.08.2013, at 13:03, Edgar E. Iglesias wrote:

> On Wed, Aug 14, 2013 at 12:03:34PM +0200, Alexander Graf wrote:
>> 
>> On 14.08.2013, at 11:56, Edgar E. Iglesias wrote:
>> 
>>> On Wed, Aug 14, 2013 at 11:51:19AM +0200, Alexander Graf wrote:
>>>> 
>>>> On 13.08.2013, at 13:09, Efimov Vasily wrote:
>>>> 
>>>>> 
>>>>> Signed-off-by: Efimov Vasily <address@hidden>
>>>> 
>>>> Please provide a patch description :).
>>>> 
>>>>> ---
>>>>> hw/ppc/virtex_ml507.c |   13 ++++++++++---
>>>>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>>>> 
>>>>> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
>>>>> index 08e77fb..a00f709 100644
>>>>> --- a/hw/ppc/virtex_ml507.c
>>>>> +++ b/hw/ppc/virtex_ml507.c
>>>>> @@ -141,11 +141,18 @@ static int xilinx_load_device_tree(hwaddr addr,
>>>>> {
>>>>>   char *path;
>>>>>   int fdt_size;
>>>>> -    void *fdt;
>>>>> +    void *fdt = 0;
>>>> 
>>>> This should be NULL. NULL doesn't have to be 0 according to C IIRC.
>>>> 
>>>>>   int r;
>>>>> +    const char *dtb_filename;
>>>>> 
>>>>> -    /* Try the local "ppc.dtb" override.  */
>>>>> -    fdt = load_device_tree("ppc.dtb", &fdt_size);
>>>>> +    dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
>>>>> +    if (dtb_filename) {
>>>>> +        fdt = load_device_tree(dtb_filename, &fdt_size);
>>>>> +    }
>>>>> +    if (!fdt) {
>>>>> +        /* Try the local "ppc.dtb" override.  */
>>>>> +        fdt = load_device_tree("ppc.dtb", &fdt_size);
>>>>> +    }
>>>> 
>>>> Could you please just remove the ppc.dtb override option? It's superfluous 
>>>> once we have proper -dtb support.
>>>> 
>>>> Edgar, any objections?
>>> 
>>> Hi,
>>> 
>>> No objections from my side, I tested the patch and it works fine.
>>> I'd prefer to keep the ppc.dtb fallback for backwards compatibility,
>>> for example the test image on the wiki relies on it.
>> 
>> Ah, ok. Then let's make the logic work like this:
>> 
>> if (user provided -dtb) {
>>  if (load_dtb(user provided dtb)) {
>>    abort();
>>  }
>> } else {
>>  if (load_dtb("ppc.dtb")) {
>>    if (load_dtb(find_file(BINARY_DEVICE_TREE_FILE))) {
>>      abort();
>>    }
>>  }
>> }
> 
> Hi Alex,
> 
> I think that we should only abort() on the -dtb arg case. The other
> cases are for backwards compatibility.
> The dtb used to be optional (useful when for example when running
> kernels with a builtin dtb) hence the lack of aborts.
> 
> Except from that, your suggestion sounds good to me.

Ah, ok. Works for me then.

Eventually the machine should really be converted to generate its device tree 
dynamically instead of loading a blob, like e500 and spapr do it.


Alex