[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v5 05/25] xen/arm: check for multiboot nodes only under /chosen
Hi Stefano,
On 10/27/18 1:42 AM, Stefano Stabellini wrote:
On Fri, 26 Oct 2018, Julien Grall wrote:
On 10/26/18 10:27 PM, Julien Grall wrote:
Hi,
On 10/26/18 10:12 PM, Stefano Stabellini wrote:
On Fri, 26 Oct 2018, Julien Grall wrote:
Hi Stefano,
On 10/23/18 3:02 AM, Stefano Stabellini wrote:
Make sure to only look for multiboot compatible nodes only under
/chosen, not under any other paths (depth <= 3).
Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
---
Changes in v5:
- add patch
- add check on return value of fdt_get_path
---
xen/arch/arm/bootfdt.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 8eba42c..a314aca 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -173,7 +173,14 @@ static void __init process_multiboot_node(const
void
*fdt, int node,
bootmodule_kind kind;
paddr_t start, size;
const char *cmdline;
- int len;
+ int len = sizeof("/chosen");
NIT, I am not convince you win anything with that trick. strlen could be
optimized by the compiler (we use __builtin_strlen on Arm64).
I could use strlen if you prefer, but given that the string is known at
compile time, it should hopefully be resolved to "8" at compile time
using sizeof. What's wrong with it?
__builtin_strlen should get optimized at compile-time. Although, I don't
seem to be the case when I tried. Not sure why.
Never mind then.
Actually I just tried to use __builtin_stlren rather than strlen. And
GCC is computing the size at compiler time.
Somehow, I thought arm64 was using __builtin_strlen but it seems to implement
there own. Most likely we would want to use __builtin_strlen wor constant
string. But that's another story.
In that case, I would prefer if you use __builtin_strlen. This is easier to
understand over sizeof.
sizeof should be easier than __builtin_strlen for most: sizeof is part
of the C language, and it is taught in C classes. I expect everybody
should know what it is. While __builtin_strlen is a compiler construct,
which is certainly not for beginners. I think it is strange you find it
easier, probably a side of effect of years of Xen/kernel programming :-)
I think it is pretty much the invert :). When I see "strlen" in the name
I can imagine what you are trying to achieve. I.e computing the length
of the string. If it has __builtin, my,... in front it just tells you
that it is a different implementation.
Sizeof takes a bit more time because you have to wonder why strlen is
not "suitable" here.
To be honest, this is a micro-optimization. It is very likely not going
to make Xen booting much faster as that function might just get called
~30 times at most.
I would rather keep it as is.
Then please stay consistent. You hardcode the value in one place, and
the other you use sizeof("/chosen"). Either way works for me.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
