| Message ID | 20190731102856.23215-2-viktor.mitin.19@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/2] xen/arm: extend fdt_property_interrupts | expand |
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
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
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
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,
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 >
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,
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;
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(-)