* [Qemu-devel] [PATCH] ppc: virtex_ml507: QEMU_OPTION_dtb support for this machine. @ 2013-08-13 11:09 Efimov Vasily 2013-08-14 9:51 ` Alexander Graf 0 siblings, 1 reply; 9+ messages in thread From: Efimov Vasily @ 2013-08-13 11:09 UTC (permalink / raw) To: qemu-devel; +Cc: Edgar E. Iglesias, qemu-ppc, Efimov Vasily, Alexander Graf Signed-off-by: Efimov Vasily <real@ispras.ru> --- 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; 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) { -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc: virtex_ml507: QEMU_OPTION_dtb support for this machine. 2013-08-13 11:09 [Qemu-devel] [PATCH] ppc: virtex_ml507: QEMU_OPTION_dtb support for this machine Efimov Vasily @ 2013-08-14 9:51 ` Alexander Graf 2013-08-14 9:56 ` Edgar E. Iglesias 2013-08-14 10:34 ` Felix Deichmann 0 siblings, 2 replies; 9+ messages in thread From: Alexander Graf @ 2013-08-14 9:51 UTC (permalink / raw) To: Efimov Vasily; +Cc: Edgar E. Iglesias, qemu-ppc, qemu-devel 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc: virtex_ml507: QEMU_OPTION_dtb support for this machine. 2013-08-14 9:51 ` Alexander Graf @ 2013-08-14 9:56 ` Edgar E. Iglesias 2013-08-14 10:03 ` Alexander Graf 2013-08-14 10:34 ` Felix Deichmann 1 sibling, 1 reply; 9+ messages in thread From: Edgar E. Iglesias @ 2013-08-14 9:56 UTC (permalink / raw) To: Alexander Graf; +Cc: qemu-ppc, Efimov Vasily, qemu-devel 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 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc: virtex_ml507: QEMU_OPTION_dtb support for this machine. 2013-08-14 9:56 ` Edgar E. Iglesias @ 2013-08-14 10:03 ` Alexander Graf 2013-08-14 11:03 ` Edgar E. Iglesias 0 siblings, 1 reply; 9+ messages in thread From: Alexander Graf @ 2013-08-14 10:03 UTC (permalink / raw) To: Edgar E. Iglesias; +Cc: qemu-ppc, Efimov Vasily, qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc: virtex_ml507: QEMU_OPTION_dtb support for this machine. 2013-08-14 10:03 ` Alexander Graf @ 2013-08-14 11:03 ` Edgar E. Iglesias 2013-08-14 11:11 ` Alexander Graf 2013-08-14 11:12 ` Andreas Färber 0 siblings, 2 replies; 9+ messages in thread From: Edgar E. Iglesias @ 2013-08-14 11:03 UTC (permalink / raw) To: Alexander Graf; +Cc: qemu-ppc, Efimov Vasily, qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc: virtex_ml507: QEMU_OPTION_dtb support for this machine. 2013-08-14 11:03 ` Edgar E. Iglesias @ 2013-08-14 11:11 ` Alexander Graf 2013-08-14 11:12 ` Andreas Färber 1 sibling, 0 replies; 9+ messages in thread From: Alexander Graf @ 2013-08-14 11:11 UTC (permalink / raw) To: Edgar E. Iglesias; +Cc: qemu-ppc, Efimov Vasily, qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc: virtex_ml507: QEMU_OPTION_dtb support for this machine. 2013-08-14 11:03 ` Edgar E. Iglesias 2013-08-14 11:11 ` Alexander Graf @ 2013-08-14 11:12 ` Andreas Färber 1 sibling, 0 replies; 9+ messages in thread From: Andreas Färber @ 2013-08-14 11:12 UTC (permalink / raw) To: Edgar E. Iglesias, Alexander Graf; +Cc: qemu-ppc, Efimov Vasily, qemu-devel 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 -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc: virtex_ml507: QEMU_OPTION_dtb support for this machine. 2013-08-14 9:51 ` Alexander Graf 2013-08-14 9:56 ` Edgar E. Iglesias @ 2013-08-14 10:34 ` Felix Deichmann 2013-08-14 10:38 ` Alexander Graf 1 sibling, 1 reply; 9+ messages in thread From: Felix Deichmann @ 2013-08-14 10:34 UTC (permalink / raw) To: Alexander Graf; +Cc: Edgar E. Iglesias, qemu-ppc, Efimov Vasily, qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc: virtex_ml507: QEMU_OPTION_dtb support for this machine. 2013-08-14 10:34 ` Felix Deichmann @ 2013-08-14 10:38 ` Alexander Graf 0 siblings, 0 replies; 9+ messages in thread From: Alexander Graf @ 2013-08-14 10:38 UTC (permalink / raw) To: Felix Deichmann; +Cc: Edgar E. Iglesias, qemu-ppc, Efimov Vasily, qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-08-14 11:12 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-13 11:09 [Qemu-devel] [PATCH] ppc: virtex_ml507: QEMU_OPTION_dtb support for this machine Efimov Vasily 2013-08-14 9:51 ` Alexander Graf 2013-08-14 9:56 ` Edgar E. Iglesias 2013-08-14 10:03 ` Alexander Graf 2013-08-14 11:03 ` Edgar E. Iglesias 2013-08-14 11:11 ` Alexander Graf 2013-08-14 11:12 ` Andreas Färber 2013-08-14 10:34 ` Felix Deichmann 2013-08-14 10:38 ` Alexander Graf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).