[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@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.

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.

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.

Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.