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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

You should have enough characters in the title to explain what you are 
extending. Something like:
xen/arm: Extend fdt_property_interrupts to support domU

On 31/07/2019 11:28, Viktor Mitin wrote:
> Extend fdt_property_interrupts to deal with other domain than the hwdom.
>
> The prototype of fdt_property_interrupts() has been modified
> to support both hwdom and domU in one function.
>
> This is a preparatory for the patch "xen/arm: merge make_timer_node and
> make_timer_domU_node". Original goal is to merge make_timer_node and
> make_timer_domU_node functions. See discussion in e-mail, the Message-ID is:
> <20190620103805.927-1-viktor.mitin.19@xxxxxxxxx>
The commit message should not point to discussion but summarize it. If you want 
to point to the discussion, then please do it after ---.
Also, this is a bit risky to write down the title of a patch that does not 
preceded it (or been committed). Image we decide to rename it... Instead, it is 
common to say "A follow-up patch will need to create the interrupts for either 
dom0 or domU".
> Note: there is no functional changes introduced by this patch.
You are expanding the function to this is technically a functional changes.

> Suggested-by: Julien Grall <julien.grall@xxxxxxx>
> Signed-off-by: Viktor Mitin <viktor_mitin@xxxxxxxx>
> ---
>   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..d04a1c3aec 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)
Please align the section line with the first argument.

>   {
>       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)
This change is still not explained in the commit message.

>   {
> +    void *fdt = kinfo->fdt;
> +
You add the newline here but drop it in the next patch. In general, it is 
strongly recommended to not add code this is removed in the same series unless 
there are a reason to.
>       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