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

[Viktor Mitin <viktor.mitin.19@xxxxxxxxx>]

Hi Julien,

On Mon, Jul 15, 2019 at 9:01 PM Julien Grall <julien.grall@xxxxxxx> wrote:
>
> 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.
>
Ok, will change it in the next version of the patch.

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

Both Dom0 and DomU (dom0less) support has been tested.
There are no the clock-frequency property in your DT timer node.
Is it possible to test it anyway somehow?

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

Ok, will use "if ( is_hardware_domain(...) )" approach you mentioned below.

> > +    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.

Ok, let's keep domU version for the compatible.

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

Ok, will use this approach.

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

Do you mean adding one more parameter, for example interrupt_parent_val?

static int __init fdt_property_interrupts(void *fdt, gic_interrupt_t
*intr,
                                          unsigned num_irq, dt_phandle
interrupt_parent_val)

>
> > +    if (res)
>
> NIT: Coding style
>
> if ( ... )

You are right, will fix this NIT as well.

Thanks

> Cheers,
>
> --
> Julien Grall

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