Re: [PATCH v5 05/25] xen/arm: check for multiboot nodes only under /chosen

[Julien Grall <julien.grall@arm.com>]

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@xilinx.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel