Re: [Xen-devel] [PATCH] xen/arm: extend fdt_property_interrupts

[Julien Grall <julien.grall@xxxxxxx>]

Hi Viktor,

Thank you for the patch. In addition to Volodymyr's review, few comments 
below on the process and the patch.
On 7/26/19 10:01 AM, Viktor Mitin wrote:
> xen/arm: extend fdt_property_interrupts to deal with other
> domain than the hwdom
As mentioned by Volodymyr, the commit message should provide enough 
information for anyone to understand why this is done.
Additionally this patch standalone is not entirely useful. This is a 
preparatory for the patch "xen/arm: merge make_timer_node and 
make_timer_domU_node". So it would make sense to create a series with 
the 2 patches. This has the advantage to give some context to the 
reviewer how this is going to be used.
If you haven't done it yet, I would recommend to have a look through the 
wiki page [1]. This will give you some guidelines how to prepare a patch 
series. Feel free to ask any question if you have any doubt :).
> Tested with aarch64 qemu dom0 and dom0less boot.
While testing is important this is not entirely useful to retain in the 
commit message. If you want to specify your setup, then I would 
recommend to add after the --- so it get strips when the patch is applied.
> Suggested-by: Julien Grall <julien.grall@xxxxxxx>
> Signed-off-by: Viktor Mitin <viktor_mitin@xxxxxxxx>
>
> ---
> See discussion in mail:
> [PATCH] xen/arm: merge make_timer_node and make_timer_domU_node
Finding a patch just with the subject can sometimes be tricky. So it is 
common to specify the Message-ID of the e-mail you refer too. For this 
e-mail, the Message-ID is:
<20190620103805.927-1-viktor.mitin.19@xxxxxxxxx>

This is an uniq identify so it is easily to lookup in your inbox or 
online archives if you don't have the e-mail.
Additionally, you can provide a link to the archived e-mail.

>   xen/arch/arm/domain_build.c | 22 +++++++++++++---------
>   1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4c8404155a..5f68603392 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -621,17 +621,19 @@ static void __init set_interrupt(gic_interrupt_t 
> interrupt,
>    *  "interrupts": contains the list of interrupts
>    *  "interrupt-parent": link to the GIC
>    */
> -static int __init fdt_property_interrupts(void *fdt, gic_interrupt_t *intr,
> -                                          unsigned num_irq)
> +static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
> +                            gic_interrupt_t *intr, unsigned num_irq)
>   {
>       int res;
> +    uint32_t phandle = is_hardware_domain(kinfo->d) ?
> +                       dt_interrupt_controller->phandle : GUEST_PHANDLE_GIC;
>   
> -    res = fdt_property(fdt, "interrupts", intr, sizeof (intr[0]) * num_irq);
> +    res = fdt_property(kinfo->fdt, "interrupts",
> +                       intr, sizeof (intr[0]) * num_irq);
>       if ( res )
>           return res;
>   
> -    res = fdt_property_cell(fdt, "interrupt-parent",
> -                            dt_interrupt_controller->phandle);
> +    res = fdt_property_cell(kinfo->fdt, "interrupt-parent", phandle);
>   
>       return res;
>   }
> @@ -733,7 +735,7 @@ static int __init make_hypervisor_node(struct domain *d,
>        *  TODO: Handle properly the cpumask;
>        */
>       set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> -    res = fdt_property_interrupts(fdt, &intr, 1);
> +    res = fdt_property_interrupts(kinfo, &intr, 1);
>       if ( res )
>           return res;
>   
> @@ -960,8 +962,10 @@ static int __init make_gic_node(const struct domain *d, void *fdt,
>       return res;
>   }
>   
> -static int __init make_timer_node(const struct domain *d, void *fdt)
> +static int __init make_timer_node(const struct kernel_info *kinfo)
You wan to explain in your commit message why the prototype has been 
modified.
>   {
> +    void *fdt = kinfo->fdt;
> +
>       static const struct dt_device_match timer_ids[] __initconst =
>       {
>           DT_MATCH_COMPATIBLE("arm,armv7-timer"),
> @@ -1016,7 +1020,7 @@ static int __init make_timer_node(const struct domain *d, 
> void *fdt)
>       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_interrupts(kinfo, intrs, 3);
>       if ( res )
>           return res;
>   
> @@ -1377,7 +1381,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
>       if ( device_get_class(node) == DEVICE_GIC )
>           return make_gic_node(d, kinfo->fdt, node);
>       if ( dt_match_node(timer_matches, node) )
> -        return make_timer_node(d, kinfo->fdt);
> +        return make_timer_node(kinfo);
>   
>       /* Skip nodes used by Xen */
>       if ( dt_device_used_by(node) == DOMID_XEN )
Cheers,

--
Julien Grall

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