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.
So it is better to merge them to avoid discrepancy.

Tested dom0 boot with rcar h3 sk board.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
---
 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);
+    uint32_t ip_val;
 
     dt_dprintk("Create timer node\n");
 
@@ -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;
     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);
+    if (res)
+        return res;
+
     clock_valid = dt_property_read_u32(dev, "clock-frequency",
                                        &clock_frequency);
     if ( clock_valid )
@@ -1581,46 +1597,6 @@ static int __init make_gic_domU_node(const struct domain *d, void *fdt)
     }
 }
 
-static int __init make_timer_domU_node(const struct domain *d, void *fdt)
-{
-    int res;
-    gic_interrupt_t intrs[3];
-
-    res = fdt_begin_node(fdt, "timer");
-    if ( res )
-        return res;
-
-    if ( !is_64bit_domain(d) )
-    {
-        res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
-        if ( res )
-            return res;
-    }
-    else
-    {
-        res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
-        if ( res )
-            return res;
-    }
-
-    set_interrupt(intrs[0], GUEST_TIMER_PHYS_S_PPI, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
-    set_interrupt(intrs[1], GUEST_TIMER_PHYS_NS_PPI, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
-    set_interrupt(intrs[2], GUEST_TIMER_VIRT_PPI, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
-
-    res = fdt_property(fdt, "interrupts", intrs, sizeof (intrs[0]) * 3);
-    if ( res )
-        return res;
-
-    res = fdt_property_cell(fdt, "interrupt-parent",
-                            GUEST_PHANDLE_GIC);
-    if (res)
-        return res;
-
-    res = fdt_end_node(fdt);
-
-    return res;
-}
-
 #ifdef CONFIG_SBSA_VUART_CONSOLE
 static int __init make_vpl011_uart_node(const struct domain *d, void *fdt)
 {
@@ -1726,7 +1702,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     if ( ret )
         goto err;
 
-    ret = make_timer_domU_node(d, kinfo->fdt);
+    ret = make_timer_node(d, kinfo->fdt);
     if ( ret )
         goto err;
 
-- 
2.17.1


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

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.

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

> 
> Suggested-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
> ---
>   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.

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

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

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

> +    if (res)

NIT: Coding style

if ( ... )

Cheers,

-- 
Julien Grall

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

Hi Julien,

On Mon, Jul 15, 2019 at 9:01 PM Julien Grall <julien.grall@arm.com> 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@arm.com>
> > Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
> > ---
> >   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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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@arm.com> 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@arm.com>
>>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
>>> ---
>>>    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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel