Re: [Xen-devel] [PATCH] xen/arm: merge make_timer_node and make_timer_domU_node

[Julien Grall <julien.grall@xxxxxxx>]

Hi Viktor,

On 20/06/2019 11:38, Viktor Mitin wrote:
> Functions make_timer_node and make_timer_domU_node are quite similar.
> The only difference between Dom0 and DomU timer DT node
> is the timer interrupts used.  All the rest code should be the same.
This is a bit confusing to read. First you say there are only "one difference" 
but then the second part leads to think there are more difference.
The commit message should describe what are the differences and which version 
you are keeping.
> So it is better to merge them to avoid discrepancy.
>
> Tested dom0 boot with rcar h3 sk board.
How about domU support? Also, do you have the clock-frequency property in your 
DT timer node?
> Suggested-by: Julien Grall <julien.grall@xxxxxxx>
> Signed-off-by: Viktor Mitin <viktor_mitin@xxxxxxxx>
> ---
>   xen/arch/arm/domain_build.c | 66 ++++++++++++-------------------------
>   1 file changed, 21 insertions(+), 45 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 7fb828cae2..610dd3e8e7 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -976,6 +976,8 @@ static int __init make_timer_node(const struct domain *d, 
> void *fdt)
>       gic_interrupt_t intrs[3];
>       u32 clock_frequency;
>       bool clock_valid;
> +    bool d0 = is_hardware_domain(d);
Please avoid to use the term "d0"/"dom0" whenever it is possible. While in 
practice the hardware domain is always dom0 on Arm, I want to avoid the mixing.
> +    uint32_t ip_val;
>   
>       dt_dprintk("Create timer node\n");
I am not sure where to comment, but the compatible will be different for DomU 
now. I would actually prefer if we keep the domU version for the compatible as 
it is simpler.
>   
> @@ -1004,22 +1006,36 @@ static int __init make_timer_node(const struct domain *d, void *fdt)
>       /* The timer IRQ is emulated by Xen. It always exposes an active-low
>        * level-sensitive interrupt */
>   
> -    irq = timer_get_irq(TIMER_PHYS_SECURE_PPI);
> +    irq = d0
> +        ? timer_get_irq(TIMER_PHYS_SECURE_PPI)
> +        : GUEST_TIMER_PHYS_S_PPI;
Rather than using ternary everywhere, how about introducing an array where the 
value will be stored.
So the code would look like:

if ( is_hardware_domain(...) )
{
  timer_irq[...] = ...;
  timer_irq[...] = ...;
}
else
{
}

[....]

set_interrupt(timer_irq[...]);

Have a look at time.c as we define handy value (enum timer_ppi and 
MAX_TIMER_PPI).

>       dt_dprintk("  Secure interrupt %u\n", irq);
>       set_interrupt(intrs[0], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
>   
> -    irq = timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
> +    irq = d0
> +        ? timer_get_irq(TIMER_PHYS_NONSECURE_PPI)
> +        : GUEST_TIMER_PHYS_NS_PPI;
>       dt_dprintk("  Non secure interrupt %u\n", irq);
>       set_interrupt(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
>   
> -    irq = timer_get_irq(TIMER_VIRT_PPI);
> +    irq = d0
> +        ? timer_get_irq(TIMER_VIRT_PPI)
> +        : GUEST_TIMER_VIRT_PPI;
>       dt_dprintk("  Virt interrupt %u\n", irq);
>       set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
>   
> -    res = fdt_property_interrupts(fdt, intrs, 3);
> +    res = fdt_property(fdt, "interrupts", intrs, sizeof (intrs[0]) * 3);
>       if ( res )
>           return res;
>   
> +    ip_val = d0
> +           ? dt_interrupt_controller->phandle
> +           : GUEST_PHANDLE_GIC;
> +
> +    res = fdt_property_cell(fdt, "interrupt-parent", ip_val);
I would actually prefer if we extend fdt_property_interrupts to deal with other 
domain than the hwdom. This would avoid the function.
> +    if (res)
NIT: Coding style

if ( ... )

Cheers,

--
Julien Grall

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