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

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

[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 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@gmail.com>

Note: there is no functional changes introduced by this patch.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
---
 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)
 {
     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;
+
     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 v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node

[Viktor Mitin]

Merged make_timer_node and make_timer_domU_node into one function
make_timer_node.

Kept the domU version for the compatible as it is simpler.
Kept 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 "Kept the domU version for the compatible as it is simpler"

 xen/arch/arm/domain_build.c | 109 +++++++++++++-----------------------
 1 file changed, 39 insertions(+), 70 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d04a1c3aec..4d7c3411a6 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -964,8 +964,12 @@ static int __init make_gic_node(const struct domain *d, void *fdt,
 
 static int __init make_timer_node(const struct kernel_info *kinfo)
 {
+    int res;
     void *fdt = kinfo->fdt;
-
+    unsigned int irq[MAX_TIMER_PPI];
+    gic_interrupt_t intrs[3];
+    u32 clock_frequency;
+    bool clock_valid;
     static const struct dt_device_match timer_ids[] __initconst =
     {
         DT_MATCH_COMPATIBLE("arm,armv7-timer"),
@@ -973,15 +977,6 @@ 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;
-    gic_interrupt_t intrs[3];
-    u32 clock_frequency;
-    bool clock_valid;
-
-    dt_dprintk("Create timer node\n");
 
     dev = dt_find_matching_node(NULL, timer_ids);
     if ( !dev )
@@ -990,35 +985,49 @@ 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 ( res )
-        return res;
+    if ( !is_64bit_domain(kinfo->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;
+    }
 
     /* The timer IRQ is emulated by Xen. It always exposes an active-low
      * level-sensitive interrupt */
+    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;
+    }
 
-    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);
+    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);
 
-    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);
+    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);
 
-    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);
+    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 +1612,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 +1717,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 v4 1/2] xen/arm: extend fdt_property_interrupts

[Volodymyr Babchuk]

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

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

[Viktor Mitin]

Hi Volodymyr,

On Wed, Jul 31, 2019 at 3:11 PM Volodymyr Babchuk
<Volodymyr_Babchuk@epam.com> wrote:
>
>
> 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.

There is no such requirement, only recommendation.
I did not put it since this is simple short patch series and both
patches in this series have been discussed previously, so it is known
what it is about.

> 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@gmail.com>
> >
> > 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.

Phandle is retreved the same way:
in case of  hwdom it is dt_interrupt_controller->phandle
in case of domU it is GUEST_PHANDLE_GIC.
Don't see any functional change here.

What is 'functional change' in your opinion?

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

There is no such coding style requirement (not explicitly documented).
Even more, the original code formatted the same way.

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

Why not? Is there any reason or pointers to the documents?

Thanks

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

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

[Volodymyr Babchuk]

Viktor Mitin writes:

> Merged make_timer_node and make_timer_domU_node into one function
> make_timer_node.
It is widely accepted to write commit messages in imperative mood,
e.g. "merge" instead of "merged"

> Kept the domU version for the compatible as it is simpler.
> Kept the hw version for the clock as it is relevant for the both cases.
... or "keep" instead of "kept"

> Suggested-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
> ---
> v4 updates:
>    updated "Kept the domU version for the compatible as it is simpler"
>
>  xen/arch/arm/domain_build.c | 109 +++++++++++++-----------------------
>  1 file changed, 39 insertions(+), 70 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d04a1c3aec..4d7c3411a6 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -964,8 +964,12 @@ static int __init make_gic_node(const struct domain *d, void *fdt,
>
>  static int __init make_timer_node(const struct kernel_info *kinfo)
>  {
> +    int res;
>      void *fdt = kinfo->fdt;
> -
In the previous patch you added this empty string, now you are deleting
it.

> +    unsigned int irq[MAX_TIMER_PPI];
MAX_TIMER_PPI equals to 4, but looks like you are using only first 3
items of the array.

> +    gic_interrupt_t intrs[3];
> +    u32 clock_frequency;
> +    bool clock_valid;
Do you really need to move those declarations?

>      static const struct dt_device_match timer_ids[] __initconst =
>      {
>          DT_MATCH_COMPATIBLE("arm,armv7-timer"),
> @@ -973,15 +977,6 @@ 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;
> -    gic_interrupt_t intrs[3];
> -    u32 clock_frequency;
> -    bool clock_valid;
> -
> -    dt_dprintk("Create timer node\n");
>
>      dev = dt_find_matching_node(NULL, timer_ids);
>      if ( !dev )
> @@ -990,35 +985,49 @@ 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 ( res )
> -        return res;
> +    if ( !is_64bit_domain(kinfo->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;
> +    }
So, previously this code copied "compatible" property from platform
device tree. Please note, that theoretically it would be neither
"arm,armv8-timer" not "arm,armv7-timer". Now you are setting one of the
two values. I'm not sure if this is right thing to do in the first
place. Probably we need comment from Julien. But this change should be
reflected in the commit message.


>      /* The timer IRQ is emulated by Xen. It always exposes an active-low
>       * level-sensitive interrupt */
I'm not demanding this, but you can fix this comment in the next
version. It does not conforms to the coding style. Also, it is partially
misplaced now.

> +    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;
> +    }
>
> -    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);
> +    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);
Strange formatting. As I said earlier, 0xf should be aligned with intrs[0].

> -    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);
> +    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);
The same about formatting.

>
> -    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);
> +    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 +1612,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 +1717,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;


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

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

[Viktor Mitin]

On Wed, Jul 31, 2019 at 3:33 PM Volodymyr Babchuk
<Volodymyr_Babchuk@epam.com> wrote:
>
>
>
> Viktor Mitin writes:
>
> > Merged make_timer_node and make_timer_domU_node into one function
> > make_timer_node.
> It is widely accepted to write commit messages in imperative mood,
> e.g. "merge" instead of "merged"
>
> > Kept the domU version for the compatible as it is simpler.
> > Kept the hw version for the clock as it is relevant for the both cases.
> ... or "keep" instead of "kept"

Well, again, there is no such rule in the coding style document.

> > Suggested-by: Julien Grall <julien.grall@arm.com>
> > Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
> > ---
> > v4 updates:
> >    updated "Kept the domU version for the compatible as it is simpler"
> >
> >  xen/arch/arm/domain_build.c | 109 +++++++++++++-----------------------
> >  1 file changed, 39 insertions(+), 70 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index d04a1c3aec..4d7c3411a6 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -964,8 +964,12 @@ static int __init make_gic_node(const struct domain *d, void *fdt,
> >
> >  static int __init make_timer_node(const struct kernel_info *kinfo)
> >  {
> > +    int res;
> >      void *fdt = kinfo->fdt;
> > -
> In the previous patch you added this empty string, now you are deleting
> it.

Why not? Do not remember why did it, probably it was more convenient
at that moment.
Anyway, why not?

>
> > +    unsigned int irq[MAX_TIMER_PPI];
> MAX_TIMER_PPI equals to 4, but looks like you are using only first 3
> items of the array.

Yes. This is because MAX_TIMER_PPI has been defined, and this
particular example is taken from time.c

>
> > +    gic_interrupt_t intrs[3];
> > +    u32 clock_frequency;
> > +    bool clock_valid;
> Do you really need to move those declarations?

Not really, it has appeared as a result of many code edit iterations.
As I mentioned previously, those patches are changed several times already,
so the final version has another order of the local variables. Why not?

> >      static const struct dt_device_match timer_ids[] __initconst =
> >      {
> >          DT_MATCH_COMPATIBLE("arm,armv7-timer"),
> > @@ -973,15 +977,6 @@ 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;
> > -    gic_interrupt_t intrs[3];
> > -    u32 clock_frequency;
> > -    bool clock_valid;
> > -
> > -    dt_dprintk("Create timer node\n");
> >
> >      dev = dt_find_matching_node(NULL, timer_ids);
> >      if ( !dev )
> > @@ -990,35 +985,49 @@ 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 ( res )
> > -        return res;
> > +    if ( !is_64bit_domain(kinfo->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;
> > +    }
> So, previously this code copied "compatible" property from platform
> device tree. Please note, that theoretically it would be neither
> "arm,armv8-timer" not "arm,armv7-timer". Now you are setting one of the
> two values. I'm not sure if this is right thing to do in the first
> place. Probably we need comment from Julien. But this change should be
> reflected in the commit message.

Well, it is done, because Julien preferred domU variant as more simple one.
Actually I have checked that both variats works well, but kept domU case.

It is in the commit message:
"Kept the domU version for the compatible as it is simpler."
>
>
> >      /* The timer IRQ is emulated by Xen. It always exposes an active-low
> >       * level-sensitive interrupt */
> I'm not demanding this, but you can fix this comment in the next
> version. It does not conforms to the coding style. Also, it is partially
> misplaced now.

The format of this comment has not been changed by me.
Why do you think that it is misplaced now?

> > +    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;
> > +    }
> >
> > -    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);
> > +    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);
> Strange formatting. As I said earlier, 0xf should be aligned with intrs[0].

See the answer in another patch. There is no such formatting rule.

> > -    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);
> > +    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);
> The same about formatting.

If you think it is important to follow this rule, let's add it to the
coding style document explicitly.
I'm ok to format it as you prefer, however, it is important to keep
such things documented explicitly.

Thanks

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

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

[Julien Grall]

Hi,

I am only going to comment on process. I will look at the rest later on.

On 31/07/2019 13:28, Viktor Mitin wrote:
> Hi Volodymyr,
> 
> On Wed, Jul 31, 2019 at 3:11 PM Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com> wrote:
>>
>>
>> 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.
>  > There is no such requirement, only recommendation.

It is a strong recommendation: "If you need to send more than one patches (which 
normally means you're sending a patch series with cover letter),".

> I did not put it since this is simple short patch series and both
> patches in this series have been discussed previously, so it is known
> what it is about.

For a first, if you don't have a cover letter then the threading in e-mail 
client would look weird:
    [PATCH v4 1/2] xen/arm: extend fdt_property_interrupts
       |-> [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node

I tend to hid anything within the thread so I have only one title. For the title 
it is not clear to me what's the purpose of the e-mail.

The cover letter is also used to keep a summary of what was discussed and the 
overall goal. It does not matter if it is just a few lines. This is also a good 
place to have a discussion of the overall series (i.e not specific to a patch).

Lastly, you may have new reviewers that haven't followed the previous 
discussion. You have also reviewers like me which receive a few hundreds e-mails 
per week (just counting my inbox so e-mail I am CCed to). While I have a good 
memory, I can't possibly remember everything single e-mails.

So the cover letter is a good place to explain what changes have been done 
between series. You can also do that per-patch.

Speaking about changelog, I would highly recommend to keep all the changelog 
from v1. This gives us an idea what happen over the review.

> 
>> 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@gmail.com>
>>>
>>> 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.
> 
> Phandle is retreved the same way:
> in case of  hwdom it is dt_interrupt_controller->phandle
> in case of domU it is GUEST_PHANDLE_GIC.
> Don't see any functional change here.
> 
> What is 'functional change' in your opinion?

The function is now extended so technically it has changed. "No functional 
change" is usually used when the code is consolidated/reworked.

> 
>>
>>>
>>> Suggested-by: Julien Grall <julien.grall@arm.com>
>>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
>>> ---
>>>   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.
> 
> There is no such coding style requirement (not explicitly documented).
> Even more, the original code formatted the same way.

What original code? This is formatted with all the parameters aligned. So can 
you please do it.

> 
>>>   {
>>>       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.
> 
> Why not? Is there any reason or pointers to the documents?

I think what Volodymyr means is the newline is not necessary here. So this could 
possibly be removed. The counter-argument to this is why do you need this newline?

In generally, we tend to keep all variables declarations together. But I would 
not argue on this.

Cheers,

-- 
Julien Grall

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

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

[Volodymyr Babchuk]

Viktor Mitin writes:

> On Wed, Jul 31, 2019 at 3:33 PM Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com> wrote:
>>
>>
>>
>> Viktor Mitin writes:
>>
>> > Merged make_timer_node and make_timer_domU_node into one function
>> > make_timer_node.
>> It is widely accepted to write commit messages in imperative mood,
>> e.g. "merge" instead of "merged"
>>
>> > Kept the domU version for the compatible as it is simpler.
>> > Kept the hw version for the clock as it is relevant for the both cases.
>> ... or "keep" instead of "kept"
>
> Well, again, there is no such rule in the coding style document.
Yeah, but this is widely accepted style. It is good to have all commit
messages in the same style, isn't?

>> > Suggested-by: Julien Grall <julien.grall@arm.com>
>> > Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
>> > ---
>> > v4 updates:
>> >    updated "Kept the domU version for the compatible as it is simpler"
>> >
>> >  xen/arch/arm/domain_build.c | 109 +++++++++++++-----------------------
>> >  1 file changed, 39 insertions(+), 70 deletions(-)
>> >
>> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> > index d04a1c3aec..4d7c3411a6 100644
>> > --- a/xen/arch/arm/domain_build.c
>> > +++ b/xen/arch/arm/domain_build.c
>> > @@ -964,8 +964,12 @@ static int __init make_gic_node(const struct domain *d, void *fdt,
>> >
>> >  static int __init make_timer_node(const struct kernel_info *kinfo)
>> >  {
>> > +    int res;
>> >      void *fdt = kinfo->fdt;
>> > -
>> In the previous patch you added this empty string, now you are deleting
>> it.
>
> Why not? Do not remember why did it, probably it was more convenient
> at that moment.
> Anyway, why not?
Because patches should not include unnecessary changes. You are spending
reviewer's mental resources by introducing unneeded changes and then
undoing them in the next patch.

>>
>> > +    unsigned int irq[MAX_TIMER_PPI];
>> MAX_TIMER_PPI equals to 4, but looks like you are using only first 3
>> items of the array.
>
> Yes. This is because MAX_TIMER_PPI has been defined, and this
> particular example is taken from time.c
Yes, but code in time.c uses all four IRQs. In your case you just wasting
extra space on stack.

>>
>> > +    gic_interrupt_t intrs[3];
>> > +    u32 clock_frequency;
>> > +    bool clock_valid;
>> Do you really need to move those declarations?
>
> Not really, it has appeared as a result of many code edit iterations.
> As I mentioned previously, those patches are changed several times already,
> so the final version has another order of the local variables. Why not?
Because patches should do only necessary things. If you for some reason
want to tidy up variable declaration, please do this in the separate patch.

>> >      static const struct dt_device_match timer_ids[] __initconst =
>> >      {
>> >          DT_MATCH_COMPATIBLE("arm,armv7-timer"),
>> > @@ -973,15 +977,6 @@ 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;
>> > -    gic_interrupt_t intrs[3];
>> > -    u32 clock_frequency;
>> > -    bool clock_valid;
>> > -
>> > -    dt_dprintk("Create timer node\n");
>> >
>> >      dev = dt_find_matching_node(NULL, timer_ids);
>> >      if ( !dev )
>> > @@ -990,35 +985,49 @@ 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 ( res )
>> > -        return res;
>> > +    if ( !is_64bit_domain(kinfo->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;
>> > +    }
>> So, previously this code copied "compatible" property from platform
>> device tree. Please note, that theoretically it would be neither
>> "arm,armv8-timer" not "arm,armv7-timer". Now you are setting one of the
>> two values. I'm not sure if this is right thing to do in the first
>> place. Probably we need comment from Julien. But this change should be
>> reflected in the commit message.
>
> Well, it is done, because Julien preferred domU variant as more simple one.
> Actually I have checked that both variats works well, but kept domU case.
My concern is that you are changing function behavior in
non-backward compatible way. Yes, it is working on your platform. But
what about others?

> It is in the commit message:
> "Kept the domU version for the compatible as it is simpler."
This implies that read knows what is "domU version". I'd prefer to see
something like "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".

>>
>> >      /* The timer IRQ is emulated by Xen. It always exposes an active-low
>> >       * level-sensitive interrupt */
>> I'm not demanding this, but you can fix this comment in the next
>> version. It does not conforms to the coding style. Also, it is partially
>> misplaced now.
>
> The format of this comment has not been changed by me.
Yes, this is why I said "I'm not demanding this".

> Why do you think that it is misplaced now?
Because it mentions active-low level sensitive interrupt. But in the
following block of the code I do not see any interrupt level
configuration:

>> > +    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;
>> > +    }
>> >

... interrupt levels are configured there:
>> > -    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);
>> > +    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);
>> Strange formatting. As I said earlier, 0xf should be aligned with intrs[0].
>
> See the answer in another patch. There is no such formatting rule.
>
>> > -    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);
>> > +    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);
>> The same about formatting.
>
> If you think it is important to follow this rule, let's add it to the
> coding style document explicitly.
> I'm ok to format it as you prefer, however, it is important to keep
> such things documented explicitly.
Yes. I'm agree that we need patch to CODING_STYLE. I'll see to it.

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

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

[Julien Grall]

Hi,

You should have enough characters in the title to explain what you are 
extending. Something like:

xen/arm: Extend fdt_property_interrupts to support domU

On 31/07/2019 11:28, 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.
> 
> 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@gmail.com>

The commit message should not point to discussion but summarize it. If you want 
to point to the discussion, then please do it after ---.

Also, this is a bit risky to write down the title of a patch that does not 
preceded it (or been committed). Image we decide to rename it... Instead, it is 
common to say "A follow-up patch will need to create the interrupts for either 
dom0 or domU".

> 
> Note: there is no functional changes introduced by this patch.

You are expanding the function to this is technically a functional changes.

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

Please align the section line with the first argument.

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

This change is still not explained in the commit message.

>   {
> +    void *fdt = kinfo->fdt;
> +

You add the newline here but drop it in the next patch. In general, it is 
strongly recommended to not add code this is removed in the same series unless 
there are a reason to.

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

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi,

NIT: s/merge/consolidate/

On 31/07/2019 11:28, Viktor Mitin wrote:
> Merged make_timer_node and make_timer_domU_node into one function
> make_timer_node.
> 
> Kept the domU version for the compatible as it is simpler.
> Kept the hw version for the clock as it is relevant for the both cases.

The commit message needs a bit of rewording:
  - It is not clear why they the two functions are merged
  - This needs more word around so the commit message looks like a coherent text.

> 
> Suggested-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
> ---
> v4 updates:
>     updated "Kept the domU version for the compatible as it is simpler"
> 
>   xen/arch/arm/domain_build.c | 109 +++++++++++++-----------------------
>   1 file changed, 39 insertions(+), 70 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d04a1c3aec..4d7c3411a6 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -964,8 +964,12 @@ static int __init make_gic_node(const struct domain *d, void *fdt,
>   
>   static int __init make_timer_node(const struct kernel_info *kinfo)
>   {
> +    int res;
>       void *fdt = kinfo->fdt;
> -

Please avoid to add code that you drop in a patch later.

> +    unsigned int irq[MAX_TIMER_PPI];
> +    gic_interrupt_t intrs[3];
> +    u32 clock_frequency;
> +    bool clock_valid;

This is not related to this patch and only increase the complexity of the 
review. If you want to do reshuffling then it should be a separate patch.

But then, I see you real value of the re-ordering here.

>       static const struct dt_device_match timer_ids[] __initconst =
>       {
>           DT_MATCH_COMPATIBLE("arm,armv7-timer"),
> @@ -973,15 +977,6 @@ 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;
> -    gic_interrupt_t intrs[3];
> -    u32 clock_frequency;
> -    bool clock_valid;
> -
> -    dt_dprintk("Create timer node\n");

Why is it dropped?

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Viktor,

I am going to exceptionally top-post.

There are rules that are widely accepted for the coding style yet they are not 
written in CODING_STYLE. Rather than keeping reminding us how everything is 
unwritten, it would be more beneficial if you try to help us making better.

Meanwhile, I see limited value to waste both your time arguing on it. Volodymyr 
is an experienced contributor of Xen Project and I trust him to point out what 
is widely accepted.

Cheers,

On 31/07/2019 14:41, Volodymyr Babchuk wrote:
> 
> Viktor Mitin writes:
> 
>> On Wed, Jul 31, 2019 at 3:33 PM Volodymyr Babchuk
>> <Volodymyr_Babchuk@epam.com> wrote:
>>>
>>>
>>>
>>> Viktor Mitin writes:
>>>
>>>> Merged make_timer_node and make_timer_domU_node into one function
>>>> make_timer_node.
>>> It is widely accepted to write commit messages in imperative mood,
>>> e.g. "merge" instead of "merged"
>>>
>>>> Kept the domU version for the compatible as it is simpler.
>>>> Kept the hw version for the clock as it is relevant for the both cases.
>>> ... or "keep" instead of "kept"
>>
>> Well, again, there is no such rule in the coding style document.
> Yeah, but this is widely accepted style. It is good to have all commit
> messages in the same style, isn't?
> 
>>>> Suggested-by: Julien Grall <julien.grall@arm.com>
>>>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
>>>> ---
>>>> v4 updates:
>>>>     updated "Kept the domU version for the compatible as it is simpler"
>>>>
>>>>   xen/arch/arm/domain_build.c | 109 +++++++++++++-----------------------
>>>>   1 file changed, 39 insertions(+), 70 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>> index d04a1c3aec..4d7c3411a6 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -964,8 +964,12 @@ static int __init make_gic_node(const struct domain *d, void *fdt,
>>>>
>>>>   static int __init make_timer_node(const struct kernel_info *kinfo)
>>>>   {
>>>> +    int res;
>>>>       void *fdt = kinfo->fdt;
>>>> -
>>> In the previous patch you added this empty string, now you are deleting
>>> it.
>>
>> Why not? Do not remember why did it, probably it was more convenient
>> at that moment.
>> Anyway, why not?
> Because patches should not include unnecessary changes. You are spending
> reviewer's mental resources by introducing unneeded changes and then
> undoing them in the next patch.
> 
>>>
>>>> +    unsigned int irq[MAX_TIMER_PPI];
>>> MAX_TIMER_PPI equals to 4, but looks like you are using only first 3
>>> items of the array.
>>
>> Yes. This is because MAX_TIMER_PPI has been defined, and this
>> particular example is taken from time.c
> Yes, but code in time.c uses all four IRQs. In your case you just wasting
> extra space on stack.
> 
>>>
>>>> +    gic_interrupt_t intrs[3];
>>>> +    u32 clock_frequency;
>>>> +    bool clock_valid;
>>> Do you really need to move those declarations?
>>
>> Not really, it has appeared as a result of many code edit iterations.
>> As I mentioned previously, those patches are changed several times already,
>> so the final version has another order of the local variables. Why not?
> Because patches should do only necessary things. If you for some reason
> want to tidy up variable declaration, please do this in the separate patch.
> 
>>>>       static const struct dt_device_match timer_ids[] __initconst =
>>>>       {
>>>>           DT_MATCH_COMPATIBLE("arm,armv7-timer"),
>>>> @@ -973,15 +977,6 @@ 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;
>>>> -    gic_interrupt_t intrs[3];
>>>> -    u32 clock_frequency;
>>>> -    bool clock_valid;
>>>> -
>>>> -    dt_dprintk("Create timer node\n");
>>>>
>>>>       dev = dt_find_matching_node(NULL, timer_ids);
>>>>       if ( !dev )
>>>> @@ -990,35 +985,49 @@ 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 ( res )
>>>> -        return res;
>>>> +    if ( !is_64bit_domain(kinfo->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;
>>>> +    }
>>> So, previously this code copied "compatible" property from platform
>>> device tree. Please note, that theoretically it would be neither
>>> "arm,armv8-timer" not "arm,armv7-timer". Now you are setting one of the
>>> two values. I'm not sure if this is right thing to do in the first
>>> place. Probably we need comment from Julien. But this change should be
>>> reflected in the commit message.
>>
>> Well, it is done, because Julien preferred domU variant as more simple one.
>> Actually I have checked that both variats works well, but kept domU case.
> My concern is that you are changing function behavior in
> non-backward compatible way. Yes, it is working on your platform. But
> what about others?
> 
>> It is in the commit message:
>> "Kept the domU version for the compatible as it is simpler."
> This implies that read knows what is "domU version". I'd prefer to see
> something like "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".
> 
>>>
>>>>       /* The timer IRQ is emulated by Xen. It always exposes an active-low
>>>>        * level-sensitive interrupt */
>>> I'm not demanding this, but you can fix this comment in the next
>>> version. It does not conforms to the coding style. Also, it is partially
>>> misplaced now.
>>
>> The format of this comment has not been changed by me.
> Yes, this is why I said "I'm not demanding this".
> 
>> Why do you think that it is misplaced now?
> Because it mentions active-low level sensitive interrupt. But in the
> following block of the code I do not see any interrupt level
> configuration:
> 
>>>> +    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;
>>>> +    }
>>>>
> 
> ... interrupt levels are configured there:
>>>> -    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);
>>>> +    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);
>>> Strange formatting. As I said earlier, 0xf should be aligned with intrs[0].
>>
>> See the answer in another patch. There is no such formatting rule.
>>
>>>> -    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);
>>>> +    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);
>>> The same about formatting.
>>
>> If you think it is important to follow this rule, let's add it to the
>> coding style document explicitly.
>> I'm ok to format it as you prefer, however, it is important to keep
>> such things documented explicitly.
> Yes. I'm agree that we need patch to CODING_STYLE. I'll see to it.
> 
> --
> Volodymyr Babchuk at EPAM
> 

-- 
Julien Grall

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

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

[Julien Grall]

Hi Volodymyr,

On 31/07/2019 14:41, Volodymyr Babchuk wrote:
> Viktor Mitin writes:
>> On Wed, Jul 31, 2019 at 3:33 PM Volodymyr Babchuk
>> <Volodymyr_Babchuk@epam.com> wrote:
>>> So, previously this code copied "compatible" property from platform
>>> device tree. Please note, that theoretically it would be neither
>>> "arm,armv8-timer" not "arm,armv7-timer". Now you are setting one of the
>>> two values. I'm not sure if this is right thing to do in the first
>>> place. Probably we need comment from Julien. But this change should be
>>> reflected in the commit message.
>>
>> Well, it is done, because Julien preferred domU variant as more simple one.
>> Actually I have checked that both variats works well, but kept domU case.
> My concern is that you are changing function behavior in
> non-backward compatible way. Yes, it is working on your platform. But
> what about others?

There are only official three compatible existing for the arch timer:
    - arm,armv7-timer
    - arm,armv8-timer
    - arm,cortex-a15-timer

The latter should always have also arm,armv7-timer. Any OS running on Xen should 
not assume that a specific property should be there. If it is not the case, then 
fix your OS.

I am also discarding any other property compatible as they are probably 
out-of-tree and therefore not supported.

For 64-bit domain, you can only ever run on Armv8 platform so there are no 
change here.

For 32-bit domain, you can run on either Armv8 and Armv7 platform. It is a grey 
area where you should pass a different property depending on the platform you 
are. Libxl is always passing armv7 so I would prefer to keep like that.

The main difference with this patch will be for 32-bit dom0. They will always 
see Armv7 compatible even when running on Armv8 platform.

Xen 32-bit on Armv8 platform is not supported. Running 32-bit dom0 on Xen 64-bit 
is very unlikely.

So I don't have any major concerns with this change. This has the advantage to 
uniformize the way arch timer is exposed to all the guests.

Cheers,

-- 
Julien Grall

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

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

[Viktor Mitin]

Hi Julien and Volodymyr,

On Wed, Jul 31, 2019 at 3:52 PM Julien Grall <julien.grall@arm.com> wrote:
>
> Hi,
>
> >> 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.
> >  > There is no such requirement, only recommendation.
>
> It is a strong recommendation: "If you need to send more than one patches (which
> normally means you're sending a patch series with cover letter),".
>
> > I did not put it since this is simple short patch series and both
> > patches in this series have been discussed previously, so it is known
> > what it is about.
>
> For a first, if you don't have a cover letter then the threading in e-mail
> client would look weird:
>     [PATCH v4 1/2] xen/arm: extend fdt_property_interrupts
>        |-> [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node
>
> I tend to hid anything within the thread so I have only one title. For the title
> it is not clear to me what's the purpose of the e-mail.
>
> The cover letter is also used to keep a summary of what was discussed and the
> overall goal. It does not matter if it is just a few lines. This is also a good
> place to have a discussion of the overall series (i.e not specific to a patch).
>
> Lastly, you may have new reviewers that haven't followed the previous
> discussion. You have also reviewers like me which receive a few hundreds e-mails
> per week (just counting my inbox so e-mail I am CCed to). While I have a good
> memory, I can't possibly remember everything single e-mails.
>
> So the cover letter is a good place to explain what changes have been done
> between series. You can also do that per-patch.
>
> Speaking about changelog, I would highly recommend to keep all the changelog
> from v1. This gives us an idea what happen over the review.

Thank you for this great and detailed argumentation provided. It makes
sense, so probably Xen patches wiki should be updated with this
information and cover letter should become not a recommendation, but a
rule.

Thanks

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

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

[Julien Grall]

Hi Viktor,

On 01/08/2019 13:26, Viktor Mitin wrote:
> Hi Julien and Volodymyr,
> 
> On Wed, Jul 31, 2019 at 3:52 PM Julien Grall <julien.grall@arm.com> wrote:
>>
>> Hi,
>>
>>>> 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.
>>>   > There is no such requirement, only recommendation.
>>
>> It is a strong recommendation: "If you need to send more than one patches (which
>> normally means you're sending a patch series with cover letter),".
>>
>>> I did not put it since this is simple short patch series and both
>>> patches in this series have been discussed previously, so it is known
>>> what it is about.
>>
>> For a first, if you don't have a cover letter then the threading in e-mail
>> client would look weird:
>>      [PATCH v4 1/2] xen/arm: extend fdt_property_interrupts
>>         |-> [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node
>>
>> I tend to hid anything within the thread so I have only one title. For the title
>> it is not clear to me what's the purpose of the e-mail.
>>
>> The cover letter is also used to keep a summary of what was discussed and the
>> overall goal. It does not matter if it is just a few lines. This is also a good
>> place to have a discussion of the overall series (i.e not specific to a patch).
>>
>> Lastly, you may have new reviewers that haven't followed the previous
>> discussion. You have also reviewers like me which receive a few hundreds e-mails
>> per week (just counting my inbox so e-mail I am CCed to). While I have a good
>> memory, I can't possibly remember everything single e-mails.
>>
>> So the cover letter is a good place to explain what changes have been done
>> between series. You can also do that per-patch.
>>
>> Speaking about changelog, I would highly recommend to keep all the changelog
>> from v1. This gives us an idea what happen over the review.
> 
> Thank you for this great and detailed argumentation provided. It makes
> sense, so probably Xen patches wiki should be updated with this
> information and cover letter should become not a recommendation, but a
> rule.

Update to the wiki are always welcomed.

Cheers,

-- 
Julien Grall

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

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

[Lars Kurth]

[-- Attachment #1.1: Type: text/plain, Size: 2929 bytes --]



> On 1 Aug 2019, at 13:41, Julien Grall <julien.grall@arm.com> wrote:
> 
> Hi Viktor,
> 
> On 01/08/2019 13:26, Viktor Mitin wrote:
>> Hi Julien and Volodymyr,
>> On Wed, Jul 31, 2019 at 3:52 PM Julien Grall <julien.grall@arm.com> wrote:
>>> 
>>> Hi,
>>> 
>>>>> 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.
>>>>  > There is no such requirement, only recommendation.
>>> 
>>> It is a strong recommendation: "If you need to send more than one patches (which
>>> normally means you're sending a patch series with cover letter),".
>>> 
>>>> I did not put it since this is simple short patch series and both
>>>> patches in this series have been discussed previously, so it is known
>>>> what it is about.
>>> 
>>> For a first, if you don't have a cover letter then the threading in e-mail
>>> client would look weird:
>>>     [PATCH v4 1/2] xen/arm: extend fdt_property_interrupts
>>>        |-> [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node
>>> 
>>> I tend to hid anything within the thread so I have only one title. For the title
>>> it is not clear to me what's the purpose of the e-mail.
>>> 
>>> The cover letter is also used to keep a summary of what was discussed and the
>>> overall goal. It does not matter if it is just a few lines. This is also a good
>>> place to have a discussion of the overall series (i.e not specific to a patch).
>>> 
>>> Lastly, you may have new reviewers that haven't followed the previous
>>> discussion. You have also reviewers like me which receive a few hundreds e-mails
>>> per week (just counting my inbox so e-mail I am CCed to). While I have a good
>>> memory, I can't possibly remember everything single e-mails.
>>> 
>>> So the cover letter is a good place to explain what changes have been done
>>> between series. You can also do that per-patch.
>>> 
>>> Speaking about changelog, I would highly recommend to keep all the changelog
>>> from v1. This gives us an idea what happen over the review.
>> Thank you for this great and detailed argumentation provided. It makes
>> sense, so probably Xen patches wiki should be updated with this
>> information and cover letter should become not a recommendation, but a
>> rule.
> 
> Update to the wiki are always welcomed.

I still have an action to rework https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches <https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches> and <https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patchesand> migrate the content to the sphinx docs

@Victor: can you quickly point out where we recommend to use cover letters (if you remember). I thought it was a requirement

Lars


[-- Attachment #1.2: Type: text/html, Size: 7347 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Julien Grall]

Hi Lars,

On 01/08/2019 14:59, Lars Kurth wrote:
> 
> 
>> On 1 Aug 2019, at 13:41, Julien Grall <julien.grall@arm.com 
>> <mailto:julien.grall@arm.com>> wrote:
>>
>> Hi Viktor,
>>
>> On 01/08/2019 13:26, Viktor Mitin wrote:
>>> Hi Julien and Volodymyr,
>>> On Wed, Jul 31, 2019 at 3:52 PM Julien Grall <julien.grall@arm.com 
>>> <mailto:julien.grall@arm.com>> wrote:
>>>>
>>>> Hi,
>>>>
>>>>>> 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.
>>>>>  > There is no such requirement, only recommendation.
>>>>
>>>> It is a strong recommendation: "If you need to send more than one patches (which
>>>> normally means you're sending a patch series with cover letter),".
>>>>
>>>>> I did not put it since this is simple short patch series and both
>>>>> patches in this series have been discussed previously, so it is known
>>>>> what it is about.
>>>>
>>>> For a first, if you don't have a cover letter then the threading in e-mail
>>>> client would look weird:
>>>>     [PATCH v4 1/2] xen/arm: extend fdt_property_interrupts
>>>>        |-> [PATCH v4 2/2] xen/arm: merge make_timer_node and 
>>>> make_timer_domU_node
>>>>
>>>> I tend to hid anything within the thread so I have only one title. For the title
>>>> it is not clear to me what's the purpose of the e-mail.
>>>>
>>>> The cover letter is also used to keep a summary of what was discussed and the
>>>> overall goal. It does not matter if it is just a few lines. This is also a good
>>>> place to have a discussion of the overall series (i.e not specific to a patch).
>>>>
>>>> Lastly, you may have new reviewers that haven't followed the previous
>>>> discussion. You have also reviewers like me which receive a few hundreds e-mails
>>>> per week (just counting my inbox so e-mail I am CCed to). While I have a good
>>>> memory, I can't possibly remember everything single e-mails.
>>>>
>>>> So the cover letter is a good place to explain what changes have been done
>>>> between series. You can also do that per-patch.
>>>>
>>>> Speaking about changelog, I would highly recommend to keep all the changelog
>>>> from v1. This gives us an idea what happen over the review.
>>> Thank you for this great and detailed argumentation provided. It makes
>>> sense, so probably Xen patches wiki should be updated with this
>>> information and cover letter should become not a recommendation, but a
>>> rule.
>>
>> Update to the wiki are always welcomed.
> 
> I still have an action to rework 
> https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches and 
> <https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patchesand> migrate the 
> content to the sphinx docs
> 
> @Victor: can you quickly point out where we recommend to use cover letters (if 
> you remember). I thought it was a requirement

It is not explicitly written in the wiki page. But it is implied in a few places 
such as in the section "Providing a git branch", "Using git send-email alone".
> 
> Lars
> 

-- 
Julien Grall

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

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

[Lars Kurth]

> On 1 Aug 2019, at 15:02, Julien Grall <julien.grall@arm.com> wrote:
> 
> Hi Lars,
> 
> On 01/08/2019 14:59, Lars Kurth wrote:
>>> On 1 Aug 2019, at 13:41, Julien Grall <julien.grall@arm.com <mailto:julien.grall@arm.com>> wrote:
>>> 
>>> Hi Viktor,
>>> 
>>> On 01/08/2019 13:26, Viktor Mitin wrote:
>>>> Hi Julien and Volodymyr,
>>>> On Wed, Jul 31, 2019 at 3:52 PM Julien Grall <julien.grall@arm.com <mailto:julien.grall@arm.com>> wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>>>> 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.
>>>>>>  > There is no such requirement, only recommendation.
>>>>> 
>>>>> It is a strong recommendation: "If you need to send more than one patches (which
>>>>> normally means you're sending a patch series with cover letter),".
>>>>> 
>>>>>> I did not put it since this is simple short patch series and both
>>>>>> patches in this series have been discussed previously, so it is known
>>>>>> what it is about.
>>>>> 
>>>>> For a first, if you don't have a cover letter then the threading in e-mail
>>>>> client would look weird:
>>>>>     [PATCH v4 1/2] xen/arm: extend fdt_property_interrupts
>>>>>        |-> [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node
>>>>> 
>>>>> I tend to hid anything within the thread so I have only one title. For the title
>>>>> it is not clear to me what's the purpose of the e-mail.
>>>>> 
>>>>> The cover letter is also used to keep a summary of what was discussed and the
>>>>> overall goal. It does not matter if it is just a few lines. This is also a good
>>>>> place to have a discussion of the overall series (i.e not specific to a patch).
>>>>> 
>>>>> Lastly, you may have new reviewers that haven't followed the previous
>>>>> discussion. You have also reviewers like me which receive a few hundreds e-mails
>>>>> per week (just counting my inbox so e-mail I am CCed to). While I have a good
>>>>> memory, I can't possibly remember everything single e-mails.
>>>>> 
>>>>> So the cover letter is a good place to explain what changes have been done
>>>>> between series. You can also do that per-patch.
>>>>> 
>>>>> Speaking about changelog, I would highly recommend to keep all the changelog
>>>>> from v1. This gives us an idea what happen over the review.
>>>> Thank you for this great and detailed argumentation provided. It makes
>>>> sense, so probably Xen patches wiki should be updated with this
>>>> information and cover letter should become not a recommendation, but a
>>>> rule.
>>> 
>>> Update to the wiki are always welcomed.
>> I still have an action to rework https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches and <https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patchesand> migrate the content to the sphinx docs
>> @Victor: can you quickly point out where we recommend to use cover letters (if you remember). I thought it was a requirement
> 
> It is not explicitly written in the wiki page. But it is implied in a few places such as in the section "Providing a git branch", "Using git send-email alone".

You are right. That should be made explicit. I think you are the only person in years that sent a series without cover letter

I will have a go over that page in the next few days reducing the options and making it more strict and clear

It would be good if you could go over it, once I am done, and let me know whether it is clearer

Lars



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

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

[Viktor Mitin]

> > It is not explicitly written in the wiki page. But it is implied in a few places such as in the section "Providing a git branch", "Using git send-email alone".
>
> You are right. That should be made explicit. I think you are the only person in years that sent a series without cover letter
>
> I will have a go over that page in the next few days reducing the options and making it more strict and clear
>
> It would be good if you could go over it, once I am done, and let me know whether it is clearer

Lars, sure, will go over it once it ready. BTW what is the policy for
editing Xen wiki? Is there some actual pointer to read about it, for
example, who is allowed to edit it, what are the rules, etc?

Thanks

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