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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Viktor,

It is recommended (and probably required, but I can't find exact place
in the rules) to include cover letter if you are sending more that one
patch in series. This will ease up review process, because reviewer will
know what to expect in the series.

Viktor Mitin writes:

> 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>
>
> Note: there is no functional changes introduced by this patch.
This is not completely true, because you change the way how phandle is
retrieved. Also, earlier you said that "fdt_property_interrupts() has
been modified to support both hwdom and domU in one function". This is
the functional change.

>
> 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)
As I said earlier, this formatting contradicts with the coding style.

>  {
>      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)
>  {
> +    void *fdt = kinfo->fdt;
> +
No need for empty line there.

>      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 )


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