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

[Julien Grall <julien.grall@xxxxxxx>]

On 16/07/2019 14:56, Viktor Mitin wrote:
> Hi Julien,
Hi,

> 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?
Yes by adding the property in your device-tree. The value should match the timer 
frequency read from CNTFREQ to avoid any big issues.
> > > 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)
I would prefer if you hide out that completely and infer from the domain (or 
kernel_info). A potential prototype would be:
int fdt_property_interrupts(struct kernel_info *kinfo, gic_interrupt_t *intr);

With the implementation looking like (pseudo-code):
   if ( is_hardware_domain(kinfo->d) )
     /* Use host phandle */
   else
     /* Use GUEST_PHANDLE_GIC */

Ideally the prototype change should be done in a separate patch

Cheers,

--
Julien Grall

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