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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Julien Grall writes:

> Hi Volodymyr,
>
> On 01/08/2019 15:07, Volodymyr Babchuk wrote:
>>
>> 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@arm.com>
>>>>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
>>>>> ---
>>>>> 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.
>
> I don't really see the problem with the current code... This is
> similar to when we use a structure but not all the field in certain
> circumstance (see dt_device_match for instance). So I would not force
> the contributor to do it.
Okay, then.

>> In this case we also can declare and use intrs[] in the same way.
>
> There is no guarantee the index in irq will match intrs[...]. So you
> need to keep them hardcoded in the latter case.
Oh, right.

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