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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,
On 07/08/2019 11:10, Viktor Mitin wrote:
> 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:
> 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.
I am afraid this comment does not match the code below. The compatible is set 
based on the domain created not the platform.
Also, I think it would be worth mentioning what's the implication of for dom0 as 
this change behavior.
> Keep the hw version for the clock as it is relevant for the both cases.
I guess you mean "dom0"/"hwdom" version? But then it is not clear what "clock" 
mean here. Did you intend to refer to "timer frequency"?
The code below looks good to me and v8 (actually v9 now...) for such series a 
bit too much... So let's discuss about the commit message here.
I would suggest the following commit message:

"
xen/arm: domain_build: Consolidate make_timer_node() and make_timer_domU_node()

At the moment, the hwdom and domUs are creating the timer node differently.

Technically the timer exposed the same way for any domain, the only difference 
should be the interrupts used. The two current other differences are:
   - compatible: The hwdom DT will use the same as the one provided by the host 
provided. The domUs DT will use "arm,armv7-timer" for 32-bit domain and 
"arm,armv8-timer" for 64-bit domain. The latter matches the behavior of libxl 
when guests are created from userspace.
   - clock-frequency: The property is used on platform with broken firmware to 
indicate the clock frequency. This should be used by all the domains, however 
this is not yet the case for domUs created by Xen.
To avoid more discrepancy the two functions are now consolidated into one place 
make_timer_node().
For simplicity, the compatible will now be based on the bitness even for the 
hwdom. This means the compatible exposed for the hwdom may differ. This should 
only have an impact on 32-bit hwdom booting on Armv8 hardware.
"

> 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
>
> v6 updates:
>         - move if out of outer "if"
>      - add full stop at the end of the last sentence
>      - minor rephrase of commit message
>
> v7 updates:
>      - removed extra braces according to Coding style rule:
>        Braces should be omitted for blocks with a single statement.
> ---
>   xen/arch/arm/domain_build.c | 96 ++++++++++++-------------------------
>   1 file changed, 31 insertions(+), 65 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 26cd0ae12c..4e51e22927 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];
>       gic_interrupt_t intrs[3];
>       u32 clock_frequency;
>       bool clock_valid;
> @@ -990,35 +988,43 @@ static int __init make_timer_node(const struct 
> kernel_info *kinfo)
>           return -FDT_ERR_XEN(ENOENT);
>       }
>   
> -    compatible = dt_get_property(dev, "compatible", &len);
> -    if ( !compatible )
> -    {
> -        dprintk(XENLOG_ERR, "Can't find compatible property for timer node\n");
> -        return -FDT_ERR_XEN(ENOENT);
> -    }
> -
>       res = fdt_begin_node(fdt, "timer");
>       if ( res )
>           return res;
>   
> -    res = fdt_property(fdt, "compatible", compatible, len);
> +    if ( !is_64bit_domain(kinfo->d) )
> +        res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
> +    else
> +        res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
>       if ( res )
>           return res;
>   
> -    /* The timer IRQ is emulated by Xen. It always exposes an active-low
> -     * level-sensitive interrupt */
> -
> -    irq = timer_get_irq(TIMER_PHYS_SECURE_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);
> -    dt_dprintk("  Non secure interrupt %u\n", irq);
> -    set_interrupt(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +    /*
> +     * The timer IRQ is emulated by Xen.
> +     * It always exposes an active-low level-sensitive interrupt.
> +     */
>   
> -    irq = timer_get_irq(TIMER_VIRT_PPI);
> -    dt_dprintk("  Virt interrupt %u\n", irq);
> -    set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +    if ( is_hardware_domain(kinfo->d) )
> +    {
> +        irq[TIMER_PHYS_SECURE_PPI] = timer_get_irq(TIMER_PHYS_SECURE_PPI);
> +        irq[TIMER_PHYS_NONSECURE_PPI] =
> +                                    timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
> +        irq[TIMER_VIRT_PPI] = timer_get_irq(TIMER_VIRT_PPI);
> +    }
> +    else
> +    {
> +        irq[TIMER_PHYS_SECURE_PPI] = GUEST_TIMER_PHYS_S_PPI;
> +        irq[TIMER_PHYS_NONSECURE_PPI] = GUEST_TIMER_PHYS_NS_PPI;
> +        irq[TIMER_VIRT_PPI] = GUEST_TIMER_VIRT_PPI;
> +    }
> +    dt_dprintk("  Secure interrupt %u\n", irq[TIMER_PHYS_SECURE_PPI]);
> +    set_interrupt(intrs[0], irq[TIMER_PHYS_SECURE_PPI],
> +                  0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +    dt_dprintk("  Non secure interrupt %u\n", irq[TIMER_PHYS_NONSECURE_PPI]);
> +    set_interrupt(intrs[1], irq[TIMER_PHYS_NONSECURE_PPI],
> +                  0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +    dt_dprintk("  Virt interrupt %u\n", irq[TIMER_VIRT_PPI]);
> +    set_interrupt(intrs[2], irq[TIMER_VIRT_PPI], 0xf, DT_IRQ_TYPE_LEVEL_LOW);
>   
>       res = fdt_property_interrupts(kinfo, intrs, 3);
>       if ( res )
> @@ -1603,46 +1609,6 @@ static int __init make_gic_domU_node(const struct domain 
> *d, void *fdt)
>       }
>   }
>   
> -static int __init make_timer_domU_node(const struct domain *d, void *fdt)
> -{
> -    int res;
> -    gic_interrupt_t intrs[3];
> -
> -    res = fdt_begin_node(fdt, "timer");
> -    if ( res )
> -        return res;
> -
> -    if ( !is_64bit_domain(d) )
> -    {
> -        res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
> -        if ( res )
> -            return res;
> -    }
> -    else
> -    {
> -        res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
> -        if ( res )
> -            return res;
> -    }
> -
> -    set_interrupt(intrs[0], GUEST_TIMER_PHYS_S_PPI, 0xf, 
> DT_IRQ_TYPE_LEVEL_LOW);
> -    set_interrupt(intrs[1], GUEST_TIMER_PHYS_NS_PPI, 0xf, 
> DT_IRQ_TYPE_LEVEL_LOW);
> -    set_interrupt(intrs[2], GUEST_TIMER_VIRT_PPI, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> -
> -    res = fdt_property(fdt, "interrupts", intrs, sizeof (intrs[0]) * 3);
> -    if ( res )
> -        return res;
> -
> -    res = fdt_property_cell(fdt, "interrupt-parent",
> -                            GUEST_PHANDLE_GIC);
> -    if (res)
> -        return res;
> -
> -    res = fdt_end_node(fdt);
> -
> -    return res;
> -}
> -
>   #ifdef CONFIG_SBSA_VUART_CONSOLE
>   static int __init make_vpl011_uart_node(const struct domain *d, void *fdt)
>   {
> @@ -1748,7 +1714,7 @@ static int __init prepare_dtb_domU(struct domain *d, 
> struct kernel_info *kinfo)
>       if ( ret )
>           goto err;
>   
> -    ret = make_timer_domU_node(d, kinfo->fdt);
> +    ret = make_timer_node(kinfo);
>       if ( ret )
>           goto err;
>   
--
Julien Grall

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