[Xen-devel] [PATCH v7 0/2] xen/arm: Consolidate make_timer_node and make_timer_domU_node

[Xen-devel] [PATCH v7 0/2] xen/arm: Consolidate make_timer_node and make_timer_domU_node

[Viktor Mitin]

Functions make_timer_node and make_timer_domU_node are quite similar,
so it is better to consolidate them to avoid discrepancy.

This patch series achives this goal in two steps:
- Extend fdt_property_interrupts to deal with other domain than the hwdom.
- Consolidate make_timer_node and make_timer_domU_node into one function:

Viktor Mitin (2):
  xen/arm: extend fdt_property_interrupts to support DomU
  xen/arm: consolidate make_timer_node and make_timer_domU_node

 xen/arch/arm/domain_build.c | 118 ++++++++++++++----------------------
 1 file changed, 44 insertions(+), 74 deletions(-)

-- 
2.17.1


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

[Xen-devel] [PATCH v7 1/2] xen/arm: extend fdt_property_interrupts to support DomU

[Viktor Mitin]

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 next patch which consolidates
make_timer_node and make_timer_domU_node".
Original goal is to consolidate make_timer functions.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
v7: removed extra space after sizeof in fdt_property_interrupts()
---
 xen/arch/arm/domain_build.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4c8404155a..26cd0ae12c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -621,17 +621,20 @@ 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,
+static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
+                                          gic_interrupt_t *intr,
                                           unsigned num_irq)
 {
     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 +736,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 +963,9 @@ 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;
     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 )
-- 
2.17.1


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

[Xen-devel] [PATCH v7 2/2] xen/arm: consolidate make_timer_node and make_timer_domU_node

[Viktor Mitin]

Functions make_timer_node and make_timer_domU_node are quite similar.
So it is better to consolidate them to avoid discrepancy.
The main difference between the functions is the timer interrupts used.

Keep the domU version for the compatible as it is simpler:
do not copy platform's 'compatible' property into hwdom device tree,
instead set either arm,armv7-timer or arm,armv8-timer,
depending on the platform type.

Keep the hw version for the clock as it is relevant for the both cases.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
---
v4 updates:
   updated "Keep the domU version for the compatible as it is simpler"

v5 updates:
    - changed 'kept' to 'keep', etc.
    - removed empty line
    - updated indentation of parameters in functions calls
    - fixed NITs
    - updated commit message

v6 updates:
	- move if out of outer "if"
    - add full stop at the end of the last sentence
    - minor rephrase of commit message

v7 updates:
    - removed extra braces according to Coding style rule:
      Braces should be omitted for blocks with a single statement.
---
 xen/arch/arm/domain_build.c | 96 ++++++++++++-------------------------
 1 file changed, 31 insertions(+), 65 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 26cd0ae12c..4e51e22927 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -973,10 +973,8 @@ static int __init make_timer_node(const struct kernel_info *kinfo)
         { /* sentinel */ },
     };
     struct dt_device_node *dev;
-    u32 len;
-    const void *compatible;
     int res;
-    unsigned int irq;
+    unsigned int irq[MAX_TIMER_PPI];
     gic_interrupt_t intrs[3];
     u32 clock_frequency;
     bool clock_valid;
@@ -990,35 +988,43 @@ static int __init make_timer_node(const struct kernel_info *kinfo)
         return -FDT_ERR_XEN(ENOENT);
     }
 
-    compatible = dt_get_property(dev, "compatible", &len);
-    if ( !compatible )
-    {
-        dprintk(XENLOG_ERR, "Can't find compatible property for timer node\n");
-        return -FDT_ERR_XEN(ENOENT);
-    }
-
     res = fdt_begin_node(fdt, "timer");
     if ( res )
         return res;
 
-    res = fdt_property(fdt, "compatible", compatible, len);
+    if ( !is_64bit_domain(kinfo->d) )
+        res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
+    else
+        res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
     if ( res )
         return res;
 
-    /* The timer IRQ is emulated by Xen. It always exposes an active-low
-     * level-sensitive interrupt */
-
-    irq = timer_get_irq(TIMER_PHYS_SECURE_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);
-    dt_dprintk("  Non secure interrupt %u\n", irq);
-    set_interrupt(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
+    /*
+     * The timer IRQ is emulated by Xen.
+     * It always exposes an active-low level-sensitive interrupt.
+     */
 
-    irq = timer_get_irq(TIMER_VIRT_PPI);
-    dt_dprintk("  Virt interrupt %u\n", irq);
-    set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
+    if ( is_hardware_domain(kinfo->d) )
+    {
+        irq[TIMER_PHYS_SECURE_PPI] = timer_get_irq(TIMER_PHYS_SECURE_PPI);
+        irq[TIMER_PHYS_NONSECURE_PPI] =
+                                    timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
+        irq[TIMER_VIRT_PPI] = timer_get_irq(TIMER_VIRT_PPI);
+    }
+    else
+    {
+        irq[TIMER_PHYS_SECURE_PPI] = GUEST_TIMER_PHYS_S_PPI;
+        irq[TIMER_PHYS_NONSECURE_PPI] = GUEST_TIMER_PHYS_NS_PPI;
+        irq[TIMER_VIRT_PPI] = GUEST_TIMER_VIRT_PPI;
+    }
+    dt_dprintk("  Secure interrupt %u\n", irq[TIMER_PHYS_SECURE_PPI]);
+    set_interrupt(intrs[0], irq[TIMER_PHYS_SECURE_PPI],
+                  0xf, DT_IRQ_TYPE_LEVEL_LOW);
+    dt_dprintk("  Non secure interrupt %u\n", irq[TIMER_PHYS_NONSECURE_PPI]);
+    set_interrupt(intrs[1], irq[TIMER_PHYS_NONSECURE_PPI],
+                  0xf, DT_IRQ_TYPE_LEVEL_LOW);
+    dt_dprintk("  Virt interrupt %u\n", irq[TIMER_VIRT_PPI]);
+    set_interrupt(intrs[2], irq[TIMER_VIRT_PPI], 0xf, DT_IRQ_TYPE_LEVEL_LOW);
 
     res = fdt_property_interrupts(kinfo, intrs, 3);
     if ( res )
@@ -1603,46 +1609,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)
 {
@@ -1748,7 +1714,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(kinfo);
     if ( ret )
         goto err;
 
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v7 0/2] xen/arm: Consolidate make_timer_node and make_timer_domU_node

[Julien Grall]

Hi,

Thank you for adding the cover letter. Although, one more request :).

Please CC reviewers on your cover letter. Otherwise, it will likely land in a 
different folder and be missed.

The script scripts/add_maintainers.pl should do the job for you.

Cheers,

On 07/08/2019 11:10, Viktor Mitin wrote:
> Functions make_timer_node and make_timer_domU_node are quite similar,
> so it is better to consolidate them to avoid discrepancy.
> 
> This patch series achives this goal in two steps:
> - Extend fdt_property_interrupts to deal with other domain than the hwdom.
> - Consolidate make_timer_node and make_timer_domU_node into one function:
> 
> Viktor Mitin (2):
>    xen/arm: extend fdt_property_interrupts to support DomU
>    xen/arm: consolidate make_timer_node and make_timer_domU_node
> 
>   xen/arch/arm/domain_build.c | 118 ++++++++++++++----------------------
>   1 file changed, 44 insertions(+), 74 deletions(-)
> 

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v7 1/2] xen/arm: extend fdt_property_interrupts to support DomU

[Julien Grall]

Hi Viktor,

On 07/08/2019 11:10, 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.

To be pedantic, the current prototype for fdt_property_interrupts() can already 
deal with either hwdom and DomU. What your patch does is passing kinfo, so you 
only use parameter rather than two. So how about:

"The domain and fdt can be found in the structure kinfo. Rather than adding a an 
extra argument for the domain, pass directly kinfo. This also requires to adapt 
fdt_property_interrupts() prototype."

> 
> This is a preparatory for the next patch which consolidates
> make_timer_node and make_timer_domU_node".

In v3, I pointed out that it is a bit risky to write down the title of a patch 
that does not preceded it (or been committed). Imagine we can decide to rename 
it. Furthermore, "next patch" implies they are committed one after the other.

It is fairly common to apply part of the series if the rest needs some rework. 
So a better (and safer wording) is "A follow-up patch will need to create the 
interrupts for either Dom0 or DomU".

> Original goal is to consolidate make_timer functions.

With my suggestion above, this sentence can be dropped.

The rest of the patch looks good to me. I am happy to update the commit message 
while committing it.

Let me know your preference.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v7 2/2] xen/arm: consolidate make_timer_node and make_timer_domU_node

[Julien Grall]

Hi,
On 07/08/2019 11:10, Viktor Mitin wrote:
> Functions make_timer_node and make_timer_domU_node are quite similar.
> So it is better to consolidate them to avoid discrepancy.
> The main difference between the functions is the timer interrupts used.
> 
> Keep the domU version for the compatible as it is simpler:
> do not copy platform's 'compatible' property into hwdom device tree,
> instead set either arm,armv7-timer or arm,armv8-timer,
> depending on the platform type.

I am afraid this comment does not match the code below. The compatible is set 
based on the domain created not the platform.

Also, I think it would be worth mentioning what's the implication of for dom0 as 
this change behavior.

> 
> Keep the hw version for the clock as it is relevant for the both cases.

I guess you mean "dom0"/"hwdom" version? But then it is not clear what "clock" 
mean here. Did you intend to refer to "timer frequency"?

The code below looks good to me and v8 (actually v9 now...) for such series a 
bit too much... So let's discuss about the commit message here.

I would suggest the following commit message:

"
xen/arm: domain_build: Consolidate make_timer_node() and make_timer_domU_node()

At the moment, the hwdom and domUs are creating the timer node differently.

Technically the timer exposed the same way for any domain, the only difference 
should be the interrupts used. The two current other differences are:
    - compatible: The hwdom DT will use the same as the one provided by the host 
provided. The domUs DT will use "arm,armv7-timer" for 32-bit domain and 
"arm,armv8-timer" for 64-bit domain. The latter matches the behavior of libxl 
when guests are created from userspace.

    - clock-frequency: The property is used on platform with broken firmware to 
indicate the clock frequency. This should be used by all the domains, however 
this is not yet the case for domUs created by Xen.

To avoid more discrepancy the two functions are now consolidated into one place 
make_timer_node().

For simplicity, the compatible will now be based on the bitness even for the 
hwdom. This means the compatible exposed for the hwdom may differ. This should 
only have an impact on 32-bit hwdom booting on Armv8 hardware.
"

> 
> Suggested-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
> ---
> v4 updates:
>     updated "Keep the domU version for the compatible as it is simpler"
> 
> v5 updates:
>      - changed 'kept' to 'keep', etc.
>      - removed empty line
>      - updated indentation of parameters in functions calls
>      - fixed NITs
>      - updated commit message
> 
> v6 updates:
> 	- move if out of outer "if"
>      - add full stop at the end of the last sentence
>      - minor rephrase of commit message
> 
> v7 updates:
>      - removed extra braces according to Coding style rule:
>        Braces should be omitted for blocks with a single statement.
> ---
>   xen/arch/arm/domain_build.c | 96 ++++++++++++-------------------------
>   1 file changed, 31 insertions(+), 65 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 26cd0ae12c..4e51e22927 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -973,10 +973,8 @@ static int __init make_timer_node(const struct kernel_info *kinfo)
>           { /* sentinel */ },
>       };
>       struct dt_device_node *dev;
> -    u32 len;
> -    const void *compatible;
>       int res;
> -    unsigned int irq;
> +    unsigned int irq[MAX_TIMER_PPI];
>       gic_interrupt_t intrs[3];
>       u32 clock_frequency;
>       bool clock_valid;
> @@ -990,35 +988,43 @@ static int __init make_timer_node(const struct kernel_info *kinfo)
>           return -FDT_ERR_XEN(ENOENT);
>       }
>   
> -    compatible = dt_get_property(dev, "compatible", &len);
> -    if ( !compatible )
> -    {
> -        dprintk(XENLOG_ERR, "Can't find compatible property for timer node\n");
> -        return -FDT_ERR_XEN(ENOENT);
> -    }
> -
>       res = fdt_begin_node(fdt, "timer");
>       if ( res )
>           return res;
>   
> -    res = fdt_property(fdt, "compatible", compatible, len);
> +    if ( !is_64bit_domain(kinfo->d) )
> +        res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
> +    else
> +        res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
>       if ( res )
>           return res;
>   
> -    /* The timer IRQ is emulated by Xen. It always exposes an active-low
> -     * level-sensitive interrupt */
> -
> -    irq = timer_get_irq(TIMER_PHYS_SECURE_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);
> -    dt_dprintk("  Non secure interrupt %u\n", irq);
> -    set_interrupt(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +    /*
> +     * The timer IRQ is emulated by Xen.
> +     * It always exposes an active-low level-sensitive interrupt.
> +     */
>   
> -    irq = timer_get_irq(TIMER_VIRT_PPI);
> -    dt_dprintk("  Virt interrupt %u\n", irq);
> -    set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +    if ( is_hardware_domain(kinfo->d) )
> +    {
> +        irq[TIMER_PHYS_SECURE_PPI] = timer_get_irq(TIMER_PHYS_SECURE_PPI);
> +        irq[TIMER_PHYS_NONSECURE_PPI] =
> +                                    timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
> +        irq[TIMER_VIRT_PPI] = timer_get_irq(TIMER_VIRT_PPI);
> +    }
> +    else
> +    {
> +        irq[TIMER_PHYS_SECURE_PPI] = GUEST_TIMER_PHYS_S_PPI;
> +        irq[TIMER_PHYS_NONSECURE_PPI] = GUEST_TIMER_PHYS_NS_PPI;
> +        irq[TIMER_VIRT_PPI] = GUEST_TIMER_VIRT_PPI;
> +    }
> +    dt_dprintk("  Secure interrupt %u\n", irq[TIMER_PHYS_SECURE_PPI]);
> +    set_interrupt(intrs[0], irq[TIMER_PHYS_SECURE_PPI],
> +                  0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +    dt_dprintk("  Non secure interrupt %u\n", irq[TIMER_PHYS_NONSECURE_PPI]);
> +    set_interrupt(intrs[1], irq[TIMER_PHYS_NONSECURE_PPI],
> +                  0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +    dt_dprintk("  Virt interrupt %u\n", irq[TIMER_VIRT_PPI]);
> +    set_interrupt(intrs[2], irq[TIMER_VIRT_PPI], 0xf, DT_IRQ_TYPE_LEVEL_LOW);
>   
>       res = fdt_property_interrupts(kinfo, intrs, 3);
>       if ( res )
> @@ -1603,46 +1609,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)
>   {
> @@ -1748,7 +1714,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(kinfo);
>       if ( ret )
>           goto err;
>   
> 

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v7 2/2] xen/arm: consolidate make_timer_node and make_timer_domU_node

[Viktor Mitin]

Hi Julien,

On Wed, Aug 7, 2019 at 4:05 PM Julien Grall <julien.grall@arm.com> wrote:
>
>
> Hi,
> On 07/08/2019 11:10, Viktor Mitin wrote:
> > Functions make_timer_node and make_timer_domU_node are quite similar.
> > So it is better to consolidate them to avoid discrepancy.
> > The main difference between the functions is the timer interrupts used.
> >
> > Keep the domU version for the compatible as it is simpler:
> > do not copy platform's 'compatible' property into hwdom device tree,
> > instead set either arm,armv7-timer or arm,armv8-timer,
> > depending on the platform type.
>
> I am afraid this comment does not match the code below. The compatible is set
> based on the domain created not the platform.
>
> Also, I think it would be worth mentioning what's the implication of for dom0 as
> this change behavior.
>
> >
> > Keep the hw version for the clock as it is relevant for the both cases.
>
> I guess you mean "dom0"/"hwdom" version? But then it is not clear what "clock"
> mean here. Did you intend to refer to "timer frequency"?
>
> The code below looks good to me and v8 (actually v9 now...) for such series a
> bit too much... So let's discuss about the commit message here.
>
> I would suggest the following commit message:
>
> "
> xen/arm: domain_build: Consolidate make_timer_node() and make_timer_domU_node()
>
> At the moment, the hwdom and domUs are creating the timer node differently.
>
> Technically the timer exposed the same way for any domain, the only difference
> should be the interrupts used. The two current other differences are:
>     - compatible: The hwdom DT will use the same as the one provided by the host
> provided. The domUs DT will use "arm,armv7-timer" for 32-bit domain and
> "arm,armv8-timer" for 64-bit domain. The latter matches the behavior of libxl
> when guests are created from userspace.
>
>     - clock-frequency: The property is used on platform with broken firmware to
> indicate the clock frequency. This should be used by all the domains, however
> this is not yet the case for domUs created by Xen.
>
> To avoid more discrepancy the two functions are now consolidated into one place
> make_timer_node().
>
> For simplicity, the compatible will now be based on the bitness even for the
> hwdom. This means the compatible exposed for the hwdom may differ. This should
> only have an impact on 32-bit hwdom booting on Armv8 hardware.
> "

Fine with me.

Thanks

> >
> > Suggested-by: Julien Grall <julien.grall@arm.com>
> > Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
> > ---
> > v4 updates:
> >     updated "Keep the domU version for the compatible as it is simpler"
> >
> > v5 updates:
> >      - changed 'kept' to 'keep', etc.
> >      - removed empty line
> >      - updated indentation of parameters in functions calls
> >      - fixed NITs
> >      - updated commit message
> >
> > v6 updates:
> >       - move if out of outer "if"
> >      - add full stop at the end of the last sentence
> >      - minor rephrase of commit message
> >
> > v7 updates:
> >      - removed extra braces according to Coding style rule:
> >        Braces should be omitted for blocks with a single statement.
> > ---
> >   xen/arch/arm/domain_build.c | 96 ++++++++++++-------------------------
> >   1 file changed, 31 insertions(+), 65 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 26cd0ae12c..4e51e22927 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -973,10 +973,8 @@ static int __init make_timer_node(const struct kernel_info *kinfo)
> >           { /* sentinel */ },
> >       };
> >       struct dt_device_node *dev;
> > -    u32 len;
> > -    const void *compatible;
> >       int res;
> > -    unsigned int irq;
> > +    unsigned int irq[MAX_TIMER_PPI];
> >       gic_interrupt_t intrs[3];
> >       u32 clock_frequency;
> >       bool clock_valid;
> > @@ -990,35 +988,43 @@ static int __init make_timer_node(const struct kernel_info *kinfo)
> >           return -FDT_ERR_XEN(ENOENT);
> >       }
> >
> > -    compatible = dt_get_property(dev, "compatible", &len);
> > -    if ( !compatible )
> > -    {
> > -        dprintk(XENLOG_ERR, "Can't find compatible property for timer node\n");
> > -        return -FDT_ERR_XEN(ENOENT);
> > -    }
> > -
> >       res = fdt_begin_node(fdt, "timer");
> >       if ( res )
> >           return res;
> >
> > -    res = fdt_property(fdt, "compatible", compatible, len);
> > +    if ( !is_64bit_domain(kinfo->d) )
> > +        res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
> > +    else
> > +        res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
> >       if ( res )
> >           return res;
> >
> > -    /* The timer IRQ is emulated by Xen. It always exposes an active-low
> > -     * level-sensitive interrupt */
> > -
> > -    irq = timer_get_irq(TIMER_PHYS_SECURE_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);
> > -    dt_dprintk("  Non secure interrupt %u\n", irq);
> > -    set_interrupt(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > +    /*
> > +     * The timer IRQ is emulated by Xen.
> > +     * It always exposes an active-low level-sensitive interrupt.
> > +     */
> >
> > -    irq = timer_get_irq(TIMER_VIRT_PPI);
> > -    dt_dprintk("  Virt interrupt %u\n", irq);
> > -    set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > +    if ( is_hardware_domain(kinfo->d) )
> > +    {
> > +        irq[TIMER_PHYS_SECURE_PPI] = timer_get_irq(TIMER_PHYS_SECURE_PPI);
> > +        irq[TIMER_PHYS_NONSECURE_PPI] =
> > +                                    timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
> > +        irq[TIMER_VIRT_PPI] = timer_get_irq(TIMER_VIRT_PPI);
> > +    }
> > +    else
> > +    {
> > +        irq[TIMER_PHYS_SECURE_PPI] = GUEST_TIMER_PHYS_S_PPI;
> > +        irq[TIMER_PHYS_NONSECURE_PPI] = GUEST_TIMER_PHYS_NS_PPI;
> > +        irq[TIMER_VIRT_PPI] = GUEST_TIMER_VIRT_PPI;
> > +    }
> > +    dt_dprintk("  Secure interrupt %u\n", irq[TIMER_PHYS_SECURE_PPI]);
> > +    set_interrupt(intrs[0], irq[TIMER_PHYS_SECURE_PPI],
> > +                  0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > +    dt_dprintk("  Non secure interrupt %u\n", irq[TIMER_PHYS_NONSECURE_PPI]);
> > +    set_interrupt(intrs[1], irq[TIMER_PHYS_NONSECURE_PPI],
> > +                  0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > +    dt_dprintk("  Virt interrupt %u\n", irq[TIMER_VIRT_PPI]);
> > +    set_interrupt(intrs[2], irq[TIMER_VIRT_PPI], 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> >
> >       res = fdt_property_interrupts(kinfo, intrs, 3);
> >       if ( res )
> > @@ -1603,46 +1609,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)
> >   {
> > @@ -1748,7 +1714,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(kinfo);
> >       if ( ret )
> >           goto err;
> >
> >
>
> --
> Julien Grall

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