[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





On 01/08/2019 14:57, Julien Grall wrote:
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.

I forgot to mention. @Viktor, it is good to try to reply to each comment at least those you don't plan to address. So the reviewer doesn't have the feeling comments are ignored...

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