Re: [Xen-devel] [PATCH v5 2/2] xen/arm: consolidate make_timer_node and make_timer_domU_node

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Julien,

Julien Grall writes:

> Hi,
>
> On 01/08/2019 14:49, Volodymyr Babchuk wrote:
>>
>> Viktor Mitin writes:
>>
>>> Functions make_timer_node and make_timer_domU_node are quite similar.
>>> So it is better to consolidate them to avoid discrepancy.
>>> The main difference between the functions is the timer interrupts used.
>>>
>>> Keep the domU version for the compatible as it is simpler.
>>> Mean do not copy platform's 'compatible' property into hwdom
>>> device tree, instead set either arm,armv7-timer or arm,armv8-timer,
>>> depending on the platform type.
>> It is hard to parse the last sentence. What about "Keep the domU version
>> for the compatible as it is simpler: do not copy platform's
>> 'compatible' property into hwdom device tree, instead set either
>> arm,armv7-timer or arm,armv8-timer, depending on the platform type." ?
>>
>>
>>> Keep the hw version for the clock as it is relevant for the both cases.
>>>
>>> The new function has changed prototype due to fdt_property_interrupts
>>>    make_timer_node(const struct kernel_info *kinfo)
>>>
>>> Suggested-by: Julien Grall <julien.grall@xxxxxxx>
>>> Signed-off-by: Viktor Mitin <viktor_mitin@xxxxxxxx>
>>> ---
>>> v4 updates:
>>>     updated "Keep the domU version for the compatible as it is simpler"
>>>
>>> v5 updates:
>>>      - changed 'kept' to 'keep', etc.
>>>      - removed empty line
>>>      - updated indentation of parameters in functions calls
>>>      - fixed NITs
>>>      - updated commit message
>>> ---
>>>   xen/arch/arm/domain_build.c | 106 +++++++++++++-----------------------
>>>   1 file changed, 39 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index bc7d17dd2c..58542130ca 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -973,10 +973,8 @@ static int __init make_timer_node(const struct 
>>> kernel_info *kinfo)
>>>           { /* sentinel */ },
>>>       };
>>>       struct dt_device_node *dev;
>>> -    u32 len;
>>> -    const void *compatible;
>>>       int res;
>>> -    unsigned int irq;
>>> +    unsigned int irq[MAX_TIMER_PPI];
>> As I said in the previous version, you are wasting stack space
>> there. Also, this is misleading. When I see array of 4 items, I expect
>> that all 4 items will be used. But you are using only 3, so it leads to
>> two conclusions: either you made a mistake, or I don't understand what
>> it happening. Either option is bad.
>
> 4 byte on a stack of 16KB... that's not really a waste worth an
> argument. The more the stack is pretty empty as this is boot. So yes,
> you will not use the last index because you don't expose hypervisor
> timer to guest yet! (Imagine nested virt). But at least it avoids
> hardcoding a number of index and match the enum.
Yes, but then it should be documented at least. Comment above will be
fine. In this case we also can declare and use intrs[] in the same way.

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel