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

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

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 <real@ispras.ru>
> >> 
> >> 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.

Best regards,
Edgar