| Message ID | 20190726090124.26444-1-viktor.mitin.19@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: extend fdt_property_interrupts | expand |
Hello Vikor, Viktor Mitin writes: > xen/arm: extend fdt_property_interrupts to deal with other > domain than the hwdom It is unclear what problem you are facing. Could you please elaborate commit message? Also, looks like your commit message is formatted incorrectly. > Tested with aarch64 qemu dom0 and dom0less boot. > > Suggested-by: Julien Grall <julien.grall@arm.com> > Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> > > --- > See discussion in mail: > [PATCH] xen/arm: merge make_timer_node and make_timer_domU_node > > > 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..5f68603392 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) Wrong indentation there. > { > int res; > + uint32_t phandle = is_hardware_domain(kinfo->d) ? There is extra space at EOL. > + dt_interrupt_controller->phandle : GUEST_PHANDLE_GIC; > > - res = fdt_property(fdt, "interrupts", intr, sizeof (intr[0]) * num_irq); > + res = fdt_property(kinfo->fdt, "interrupts", There is extra space at EOL. > + 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 )
Hi Volodymyr, On Fri, Jul 26, 2019 at 2:57 PM Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> wrote: > > > Hello Vikor, > > Viktor Mitin writes: > > > xen/arm: extend fdt_property_interrupts to deal with other > > domain than the hwdom > > It is unclear what problem you are facing. Could you please elaborate > commit message? Original goal is to merge make_timer_node and make_timer_domU_node functions. For background discussion please see discussion in mail (also mentioned in the patch): [PATCH] xen/arm: merge make_timer_node and make_timer_domU_node https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00914.html > Also, looks like your commit message is formatted incorrectly. > Please see below. > > Tested with aarch64 qemu dom0 and dom0less boot. > > > > Suggested-by: Julien Grall <julien.grall@arm.com> > > Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> > > > > --- > > See discussion in mail: > > [PATCH] xen/arm: merge make_timer_node and make_timer_domU_node > > > > > > 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..5f68603392 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) > Wrong indentation there. Such cases are not defined in the Coding Style document explicitly. See my clang-format coding style mail. > > { > > int res; > > + uint32_t phandle = is_hardware_domain(kinfo->d) ? > There is extra space at EOL. > > > + dt_interrupt_controller->phandle : GUEST_PHANDLE_GIC; > > > > - res = fdt_property(fdt, "interrupts", intr, sizeof (intr[0]) * num_irq); > > + res = fdt_property(kinfo->fdt, "interrupts", > There is extra space at EOL. Will fix that. Thanks
Viktor Mitin writes: > Hi Volodymyr, > > On Fri, Jul 26, 2019 at 2:57 PM Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com> wrote: >> >> >> Hello Vikor, >> >> Viktor Mitin writes: >> >> > xen/arm: extend fdt_property_interrupts to deal with other >> > domain than the hwdom >> >> It is unclear what problem you are facing. Could you please elaborate >> commit message? > > Original goal is to merge make_timer_node and make_timer_domU_node functions. > For background discussion please see discussion in mail (also > mentioned in the patch): > [PATCH] xen/arm: merge make_timer_node and make_timer_domU_node > https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00914.html This is mentioned below the tearline and newer will be included in the git history. Here is the rules: Write a good description for each patch: - The first line the top of the patch should contain a short description of what the patch does, and hints as to what code it touches - The description should be useful for both the reviewers and people in the future trying to understand what the patch does. It can be as short as Code cleanup -- no functional changes, or as long as it takes to accurately describe the bug you are trying to solve or the new functionality you are introducing. It is hard to say what you are trying to solve with this change. I can't review functional changes in your patch, because I don't understand what you are trying to achieve. >> Also, looks like your commit message is formatted incorrectly. >> > Please see below. I was talking about commit message. I'm seeing "xen/arm:" tag both in the subject and in the e-mail body. >> > Tested with aarch64 qemu dom0 and dom0less boot. >> > >> > Suggested-by: Julien Grall <julien.grall@arm.com> >> > Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> >> > >> > --- >> > See discussion in mail: >> > [PATCH] xen/arm: merge make_timer_node and make_timer_domU_node >> > >> > >> > 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..5f68603392 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) >> Wrong indentation there. > > Such cases are not defined in the Coding Style document explicitly. > See my clang-format coding style mail. Yes, this is one of that rules that actually is not documented, but is widely enforced. -- Volodymyr Babchuk at EPAM
Hi Viktor, Thank you for the patch. In addition to Volodymyr's review, few comments below on the process and the patch. On 7/26/19 10:01 AM, Viktor Mitin wrote: > xen/arm: extend fdt_property_interrupts to deal with other > domain than the hwdom As mentioned by Volodymyr, the commit message should provide enough information for anyone to understand why this is done. Additionally this patch standalone is not entirely useful. This is a preparatory for the patch "xen/arm: merge make_timer_node and make_timer_domU_node". So it would make sense to create a series with the 2 patches. This has the advantage to give some context to the reviewer how this is going to be used. If you haven't done it yet, I would recommend to have a look through the wiki page [1]. This will give you some guidelines how to prepare a patch series. Feel free to ask any question if you have any doubt :). > > Tested with aarch64 qemu dom0 and dom0less boot. While testing is important this is not entirely useful to retain in the commit message. If you want to specify your setup, then I would recommend to add after the --- so it get strips when the patch is applied. > > Suggested-by: Julien Grall <julien.grall@arm.com> > Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> > > --- > See discussion in mail: > [PATCH] xen/arm: merge make_timer_node and make_timer_domU_node Finding a patch just with the subject can sometimes be tricky. So it is common to specify the Message-ID of the e-mail you refer too. For this e-mail, the Message-ID is: <20190620103805.927-1-viktor.mitin.19@gmail.com> This is an uniq identify so it is easily to lookup in your inbox or online archives if you don't have the e-mail. Additionally, you can provide a link to the archived e-mail. > > > 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..5f68603392 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) You wan to explain in your commit message why the prototype has been modified. > { > + 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 ) > Cheers,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 4c8404155a..5f68603392 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 )
xen/arm: extend fdt_property_interrupts to deal with other domain than the hwdom Tested with aarch64 qemu dom0 and dom0less boot. Suggested-by: Julien Grall <julien.grall@arm.com> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> --- See discussion in mail: [PATCH] xen/arm: merge make_timer_node and make_timer_domU_node xen/arch/arm/domain_build.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)