| Message ID | 1376392181-30110-1-git-send-email-real@ispras.ru |
|---|---|
| State | New |
| Headers | show |
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? Also, you should abort if the user specified a dtb through -dtb and that file could not get loaded. Alex > if (!fdt) { > path = qemu_find_file(QEMU_FILE_TYPE_BIOS, BINARY_DEVICE_TREE_FILE); > if (path) { > -- > 1.7.10.4 >
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. Thanks, Edgar > > Also, you should abort if the user specified a dtb through -dtb and that file could not get loaded. > > > Alex > > > if (!fdt) { > > path = qemu_find_file(QEMU_FILE_TYPE_BIOS, BINARY_DEVICE_TREE_FILE); > > if (path) { > > -- > > 1.7.10.4 > > >
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(); } } } Alex
2013/8/14 Alexander Graf <agraf@suse.de>: >> - void *fdt; >> + void *fdt = 0; > > This should be NULL. NULL doesn't have to be 0 according to C IIRC. The last statement is wrong here, NULL is always the same as 0 language-wise. Although the above code is always correct, some will consider it better style to use NULL when dealing with pointer context. What you probably meant is that the *internal representation* of a null pointer is not guaranteed to be all-0-bits, in contrast to the conceptual null pointer constant (== 0) understood and taken care of by the compiler. But the internal representation is irrelevant here. http://c-faq.com/null/ Felix
On 14.08.2013, at 12:34, Felix Deichmann wrote: > 2013/8/14 Alexander Graf <agraf@suse.de>: >>> - void *fdt; >>> + void *fdt = 0; >> >> This should be NULL. NULL doesn't have to be 0 according to C IIRC. > > The last statement is wrong here, NULL is always the same as 0 > language-wise. Although the above code is always correct, some will > consider it better style to use NULL when dealing with pointer > context. > What you probably meant is that the *internal representation* of a > null pointer is not guaranteed to be all-0-bits, in contrast to the > conceptual null pointer constant (== 0) understood and taken care of > by the compiler. But the internal representation is irrelevant here. > > http://c-faq.com/null/ Ah, very nice page explaining everything and more I ever wanted to know about NULL ;). Alex
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
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 <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. 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
Am 14.08.2013 13:03, schrieb Edgar E. Iglesias: > On Wed, Aug 14, 2013 at 12:03:34PM +0200, Alexander Graf wrote: >> 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. Don't abort() on user-supplied arguments, they're for programmer errors. Also since Alex has been on vacation, please note that we have a shiny error_report() function that should be preferred over any fprintf(stderr, ...). :) Only downside is it seems to require its own #include. :/ Andreas
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; 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); + } if (!fdt) { path = qemu_find_file(QEMU_FILE_TYPE_BIOS, BINARY_DEVICE_TREE_FILE); if (path) {
Signed-off-by: Efimov Vasily <real@ispras.ru> --- hw/ppc/virtex_ml507.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)