| Message ID | 20190801120452.6814-3-viktor.mitin.19@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Consolidate make_timer_node and make_timer_domU_node | expand |
Viktor Mitin writes: > Functions make_timer_node and make_timer_domU_node are quite similar. > So it is better to consolidate them to avoid discrepancy. > The main difference between the functions is the timer interrupts used. > > Keep the domU version for the compatible as it is simpler. > Mean 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. It is hard to parse the last sentence. What about "Keep the domU version for the compatible as it is simpler: do not copy platform's 'compatible' property into hwdom device tree, instead set either arm,armv7-timer or arm,armv8-timer, depending on the platform type." ? > Keep the hw version for the clock as it is relevant for the both cases. > > The new function has changed prototype due to fdt_property_interrupts > make_timer_node(const struct kernel_info *kinfo) > > Suggested-by: Julien Grall <julien.grall@arm.com> > Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> > --- > v4 updates: > updated "Keep the domU version for the compatible as it is simpler" > > v5 updates: > - changed 'kept' to 'keep', etc. > - removed empty line > - updated indentation of parameters in functions calls > - fixed NITs > - updated commit message > --- > xen/arch/arm/domain_build.c | 106 +++++++++++++----------------------- > 1 file changed, 39 insertions(+), 67 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index bc7d17dd2c..58542130ca 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -973,10 +973,8 @@ static int __init make_timer_node(const struct kernel_info *kinfo) > { /* sentinel */ }, > }; > struct dt_device_node *dev; > - u32 len; > - const void *compatible; > int res; > - unsigned int irq; > + unsigned int irq[MAX_TIMER_PPI]; As I said in the previous version, you are wasting stack space there. Also, this is misleading. When I see array of 4 items, I expect that all 4 items will be used. But you are using only 3, so it leads to two conclusions: either you made a mistake, or I don't understand what it happening. Either option is bad. > gic_interrupt_t intrs[3]; > u32 clock_frequency; > bool clock_valid; > @@ -990,35 +988,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; > - > - /* The timer IRQ is emulated by Xen. It always exposes an active-low > - * level-sensitive interrupt */ > - > - irq = timer_get_irq(TIMER_PHYS_SECURE_PPI); > - dt_dprintk(" Secure interrupt %u\n", irq); > - set_interrupt(intrs[0], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW); > + 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; > + } Both cases bear the same piece of code: if ( res ) return res; You can move it out of outer "if". This will make code shorter and simpler. > > - irq = timer_get_irq(TIMER_PHYS_NONSECURE_PPI); > - dt_dprintk(" Non secure interrupt %u\n", irq); > - set_interrupt(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW); > + /* > + * The timer IRQ is emulated by Xen. > + * It always exposes an active-low level-sensitive interrupt > + */ Missing full stop at the end of the last sentence. > > - irq = timer_get_irq(TIMER_VIRT_PPI); > - dt_dprintk(" Virt interrupt %u\n", irq); > - set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW); > + if ( is_hardware_domain(kinfo->d) ) > + { > + irq[TIMER_PHYS_SECURE_PPI] = timer_get_irq(TIMER_PHYS_SECURE_PPI); > + irq[TIMER_PHYS_NONSECURE_PPI] = > + timer_get_irq(TIMER_PHYS_NONSECURE_PPI); > + irq[TIMER_VIRT_PPI] = timer_get_irq(TIMER_VIRT_PPI); > + } > + else > + { > + irq[TIMER_PHYS_SECURE_PPI] = GUEST_TIMER_PHYS_S_PPI; > + irq[TIMER_PHYS_NONSECURE_PPI] = GUEST_TIMER_PHYS_NS_PPI; > + irq[TIMER_VIRT_PPI] = GUEST_TIMER_VIRT_PPI; > + } > + dt_dprintk(" Secure interrupt %u\n", irq[TIMER_PHYS_SECURE_PPI]); > + set_interrupt(intrs[0], irq[TIMER_PHYS_SECURE_PPI], > + 0xf, DT_IRQ_TYPE_LEVEL_LOW); > + dt_dprintk(" Non secure interrupt %u\n", irq[TIMER_PHYS_NONSECURE_PPI]); > + set_interrupt(intrs[1], irq[TIMER_PHYS_NONSECURE_PPI], > + 0xf, DT_IRQ_TYPE_LEVEL_LOW); > + dt_dprintk(" Virt interrupt %u\n", irq[TIMER_VIRT_PPI]); > + set_interrupt(intrs[2], irq[TIMER_VIRT_PPI], 0xf, DT_IRQ_TYPE_LEVEL_LOW); Why are you using plain indexes for intrs[] and enums for irq[]? > > res = fdt_property_interrupts(kinfo, intrs, 3); > if ( res ) > @@ -1603,46 +1615,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 +1720,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
Hi, On 01/08/2019 14:49, Volodymyr Babchuk wrote: > > Viktor Mitin writes: > >> Functions make_timer_node and make_timer_domU_node are quite similar. >> So it is better to consolidate them to avoid discrepancy. >> The main difference between the functions is the timer interrupts used. >> >> Keep the domU version for the compatible as it is simpler. >> Mean 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. > It is hard to parse the last sentence. What about "Keep the domU version > for the compatible as it is simpler: do not copy platform's > 'compatible' property into hwdom device tree, instead set either > arm,armv7-timer or arm,armv8-timer, depending on the platform type." ? > > >> Keep the hw version for the clock as it is relevant for the both cases. >> >> The new function has changed prototype due to fdt_property_interrupts >> make_timer_node(const struct kernel_info *kinfo) >> >> Suggested-by: Julien Grall <julien.grall@arm.com> >> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> >> --- >> v4 updates: >> updated "Keep the domU version for the compatible as it is simpler" >> >> v5 updates: >> - changed 'kept' to 'keep', etc. >> - removed empty line >> - updated indentation of parameters in functions calls >> - fixed NITs >> - updated commit message >> --- >> xen/arch/arm/domain_build.c | 106 +++++++++++++----------------------- >> 1 file changed, 39 insertions(+), 67 deletions(-) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index bc7d17dd2c..58542130ca 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -973,10 +973,8 @@ static int __init make_timer_node(const struct kernel_info *kinfo) >> { /* sentinel */ }, >> }; >> struct dt_device_node *dev; >> - u32 len; >> - const void *compatible; >> int res; >> - unsigned int irq; >> + unsigned int irq[MAX_TIMER_PPI]; > As I said in the previous version, you are wasting stack space > there. Also, this is misleading. When I see array of 4 items, I expect > that all 4 items will be used. But you are using only 3, so it leads to > two conclusions: either you made a mistake, or I don't understand what > it happening. Either option is bad. 4 byte on a stack of 16KB... that's not really a waste worth an argument. The more the stack is pretty empty as this is boot. So yes, you will not use the last index because you don't expose hypervisor timer to guest yet! (Imagine nested virt). But at least it avoids hardcoding a number of index and match the enum. Cheers,
On 01/08/2019 14:57, Julien Grall wrote: > Hi, > > On 01/08/2019 14:49, Volodymyr Babchuk wrote: >> >> Viktor Mitin writes: >> >>> Functions make_timer_node and make_timer_domU_node are quite similar. >>> So it is better to consolidate them to avoid discrepancy. >>> The main difference between the functions is the timer interrupts used. >>> >>> Keep the domU version for the compatible as it is simpler. >>> Mean 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. >> It is hard to parse the last sentence. What about "Keep the domU version >> for the compatible as it is simpler: do not copy platform's >> 'compatible' property into hwdom device tree, instead set either >> arm,armv7-timer or arm,armv8-timer, depending on the platform type." ? >> >> >>> Keep the hw version for the clock as it is relevant for the both cases. >>> >>> The new function has changed prototype due to fdt_property_interrupts >>> make_timer_node(const struct kernel_info *kinfo) >>> >>> Suggested-by: Julien Grall <julien.grall@arm.com> >>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> >>> --- >>> v4 updates: >>> updated "Keep the domU version for the compatible as it is simpler" >>> >>> v5 updates: >>> - changed 'kept' to 'keep', etc. >>> - removed empty line >>> - updated indentation of parameters in functions calls >>> - fixed NITs >>> - updated commit message >>> --- >>> xen/arch/arm/domain_build.c | 106 +++++++++++++----------------------- >>> 1 file changed, 39 insertions(+), 67 deletions(-) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index bc7d17dd2c..58542130ca 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -973,10 +973,8 @@ static int __init make_timer_node(const struct >>> kernel_info *kinfo) >>> { /* sentinel */ }, >>> }; >>> struct dt_device_node *dev; >>> - u32 len; >>> - const void *compatible; >>> int res; >>> - unsigned int irq; >>> + unsigned int irq[MAX_TIMER_PPI]; >> As I said in the previous version, you are wasting stack space >> there. Also, this is misleading. When I see array of 4 items, I expect >> that all 4 items will be used. But you are using only 3, so it leads to >> two conclusions: either you made a mistake, or I don't understand what >> it happening. Either option is bad. > > 4 byte on a stack of 16KB... that's not really a waste worth an argument. The > more the stack is pretty empty as this is boot. So yes, you will not use the > last index because you don't expose hypervisor timer to guest yet! (Imagine > nested virt). But at least it avoids hardcoding a number of index and match the > enum. I forgot to mention. @Viktor, it is good to try to reply to each comment at least those you don't plan to address. So the reviewer doesn't have the feeling comments are ignored... Cheers,
Hi Julien, Julien Grall writes: > Hi, > > On 01/08/2019 14:49, Volodymyr Babchuk wrote: >> >> Viktor Mitin writes: >> >>> Functions make_timer_node and make_timer_domU_node are quite similar. >>> So it is better to consolidate them to avoid discrepancy. >>> The main difference between the functions is the timer interrupts used. >>> >>> Keep the domU version for the compatible as it is simpler. >>> Mean 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. >> It is hard to parse the last sentence. What about "Keep the domU version >> for the compatible as it is simpler: do not copy platform's >> 'compatible' property into hwdom device tree, instead set either >> arm,armv7-timer or arm,armv8-timer, depending on the platform type." ? >> >> >>> Keep the hw version for the clock as it is relevant for the both cases. >>> >>> The new function has changed prototype due to fdt_property_interrupts >>> make_timer_node(const struct kernel_info *kinfo) >>> >>> Suggested-by: Julien Grall <julien.grall@arm.com> >>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> >>> --- >>> v4 updates: >>> updated "Keep the domU version for the compatible as it is simpler" >>> >>> v5 updates: >>> - changed 'kept' to 'keep', etc. >>> - removed empty line >>> - updated indentation of parameters in functions calls >>> - fixed NITs >>> - updated commit message >>> --- >>> xen/arch/arm/domain_build.c | 106 +++++++++++++----------------------- >>> 1 file changed, 39 insertions(+), 67 deletions(-) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index bc7d17dd2c..58542130ca 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -973,10 +973,8 @@ static int __init make_timer_node(const struct kernel_info *kinfo) >>> { /* sentinel */ }, >>> }; >>> struct dt_device_node *dev; >>> - u32 len; >>> - const void *compatible; >>> int res; >>> - unsigned int irq; >>> + unsigned int irq[MAX_TIMER_PPI]; >> As I said in the previous version, you are wasting stack space >> there. Also, this is misleading. When I see array of 4 items, I expect >> that all 4 items will be used. But you are using only 3, so it leads to >> two conclusions: either you made a mistake, or I don't understand what >> it happening. Either option is bad. > > 4 byte on a stack of 16KB... that's not really a waste worth an > argument. The more the stack is pretty empty as this is boot. So yes, > you will not use the last index because you don't expose hypervisor > timer to guest yet! (Imagine nested virt). But at least it avoids > hardcoding a number of index and match the enum. Yes, but then it should be documented at least. Comment above will be fine. In this case we also can declare and use intrs[] in the same way.
Hi Volodymyr, On 01/08/2019 15:07, Volodymyr Babchuk wrote: > > Hi Julien, > > Julien Grall writes: > >> Hi, >> >> On 01/08/2019 14:49, Volodymyr Babchuk wrote: >>> >>> Viktor Mitin writes: >>> >>>> Functions make_timer_node and make_timer_domU_node are quite similar. >>>> So it is better to consolidate them to avoid discrepancy. >>>> The main difference between the functions is the timer interrupts used. >>>> >>>> Keep the domU version for the compatible as it is simpler. >>>> Mean 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. >>> It is hard to parse the last sentence. What about "Keep the domU version >>> for the compatible as it is simpler: do not copy platform's >>> 'compatible' property into hwdom device tree, instead set either >>> arm,armv7-timer or arm,armv8-timer, depending on the platform type." ? >>> >>> >>>> Keep the hw version for the clock as it is relevant for the both cases. >>>> >>>> The new function has changed prototype due to fdt_property_interrupts >>>> make_timer_node(const struct kernel_info *kinfo) >>>> >>>> Suggested-by: Julien Grall <julien.grall@arm.com> >>>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> >>>> --- >>>> v4 updates: >>>> updated "Keep the domU version for the compatible as it is simpler" >>>> >>>> v5 updates: >>>> - changed 'kept' to 'keep', etc. >>>> - removed empty line >>>> - updated indentation of parameters in functions calls >>>> - fixed NITs >>>> - updated commit message >>>> --- >>>> xen/arch/arm/domain_build.c | 106 +++++++++++++----------------------- >>>> 1 file changed, 39 insertions(+), 67 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>> index bc7d17dd2c..58542130ca 100644 >>>> --- a/xen/arch/arm/domain_build.c >>>> +++ b/xen/arch/arm/domain_build.c >>>> @@ -973,10 +973,8 @@ static int __init make_timer_node(const struct kernel_info *kinfo) >>>> { /* sentinel */ }, >>>> }; >>>> struct dt_device_node *dev; >>>> - u32 len; >>>> - const void *compatible; >>>> int res; >>>> - unsigned int irq; >>>> + unsigned int irq[MAX_TIMER_PPI]; >>> As I said in the previous version, you are wasting stack space >>> there. Also, this is misleading. When I see array of 4 items, I expect >>> that all 4 items will be used. But you are using only 3, so it leads to >>> two conclusions: either you made a mistake, or I don't understand what >>> it happening. Either option is bad. >> >> 4 byte on a stack of 16KB... that's not really a waste worth an >> argument. The more the stack is pretty empty as this is boot. So yes, >> you will not use the last index because you don't expose hypervisor >> timer to guest yet! (Imagine nested virt). But at least it avoids >> hardcoding a number of index and match the enum. > Yes, but then it should be documented at least. Comment above will be > fine. I don't really see the problem with the current code... This is similar to when we use a structure but not all the field in certain circumstance (see dt_device_match for instance). So I would not force the contributor to do it. > In this case we also can declare and use intrs[] in the same way. There is no guarantee the index in irq will match intrs[...]. So you need to keep them hardcoded in the latter case. Cheers,
Julien Grall writes: > Hi Volodymyr, > > On 01/08/2019 15:07, Volodymyr Babchuk wrote: >> >> Hi Julien, >> >> Julien Grall writes: >> >>> Hi, >>> >>> On 01/08/2019 14:49, Volodymyr Babchuk wrote: >>>> >>>> Viktor Mitin writes: >>>> >>>>> Functions make_timer_node and make_timer_domU_node are quite similar. >>>>> So it is better to consolidate them to avoid discrepancy. >>>>> The main difference between the functions is the timer interrupts used. >>>>> >>>>> Keep the domU version for the compatible as it is simpler. >>>>> Mean 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. >>>> It is hard to parse the last sentence. What about "Keep the domU version >>>> for the compatible as it is simpler: do not copy platform's >>>> 'compatible' property into hwdom device tree, instead set either >>>> arm,armv7-timer or arm,armv8-timer, depending on the platform type." ? >>>> >>>> >>>>> Keep the hw version for the clock as it is relevant for the both cases. >>>>> >>>>> The new function has changed prototype due to fdt_property_interrupts >>>>> make_timer_node(const struct kernel_info *kinfo) >>>>> >>>>> Suggested-by: Julien Grall <julien.grall@arm.com> >>>>> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> >>>>> --- >>>>> v4 updates: >>>>> updated "Keep the domU version for the compatible as it is simpler" >>>>> >>>>> v5 updates: >>>>> - changed 'kept' to 'keep', etc. >>>>> - removed empty line >>>>> - updated indentation of parameters in functions calls >>>>> - fixed NITs >>>>> - updated commit message >>>>> --- >>>>> xen/arch/arm/domain_build.c | 106 +++++++++++++----------------------- >>>>> 1 file changed, 39 insertions(+), 67 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>>> index bc7d17dd2c..58542130ca 100644 >>>>> --- a/xen/arch/arm/domain_build.c >>>>> +++ b/xen/arch/arm/domain_build.c >>>>> @@ -973,10 +973,8 @@ static int __init make_timer_node(const struct kernel_info *kinfo) >>>>> { /* sentinel */ }, >>>>> }; >>>>> struct dt_device_node *dev; >>>>> - u32 len; >>>>> - const void *compatible; >>>>> int res; >>>>> - unsigned int irq; >>>>> + unsigned int irq[MAX_TIMER_PPI]; >>>> As I said in the previous version, you are wasting stack space >>>> there. Also, this is misleading. When I see array of 4 items, I expect >>>> that all 4 items will be used. But you are using only 3, so it leads to >>>> two conclusions: either you made a mistake, or I don't understand what >>>> it happening. Either option is bad. >>> >>> 4 byte on a stack of 16KB... that's not really a waste worth an >>> argument. The more the stack is pretty empty as this is boot. So yes, >>> you will not use the last index because you don't expose hypervisor >>> timer to guest yet! (Imagine nested virt). But at least it avoids >>> hardcoding a number of index and match the enum. >> Yes, but then it should be documented at least. Comment above will be >> fine. > > I don't really see the problem with the current code... This is > similar to when we use a structure but not all the field in certain > circumstance (see dt_device_match for instance). So I would not force > the contributor to do it. Okay, then. >> In this case we also can declare and use intrs[] in the same way. > > There is no guarantee the index in irq will match intrs[...]. So you > need to keep them hardcoded in the latter case. Oh, right.
On Thu, Aug 1, 2019 at 4:58 PM Julien Grall <julien.grall@arm.com> wrote: > > > > On 01/08/2019 14:57, Julien Grall wrote: > >>> + unsigned int irq[MAX_TIMER_PPI]; > >> As I said in the previous version, you are wasting stack space > >> there. Also, this is misleading. When I see array of 4 items, I expect > >> that all 4 items will be used. But you are using only 3, so it leads to > >> two conclusions: either you made a mistake, or I don't understand what > >> it happening. Either option is bad. > > > > 4 byte on a stack of 16KB... that's not really a waste worth an argument. The > > more the stack is pretty empty as this is boot. So yes, you will not use the > > last index because you don't expose hypervisor timer to guest yet! (Imagine > > nested virt). But at least it avoids hardcoding a number of index and match the > > enum. > > I forgot to mention. @Viktor, it is good to try to reply to each comment at > least those you don't plan to address. So the reviewer doesn't have the feeling > comments are ignored... Well, I address each of the comments or write about it explicitly in other cases. In this particular case, I'd added '-1', but later did not merge it due to mistake. So it supposed to be the next: + unsigned int irq[MAX_TIMER_PPI-1] There are no comments ignored from my side, at least not intentionally. In this case, there were myriads of the small things, like add space here or remove empty line there. I did not agree with all of them, however, in the next version (in v5), all of them have been addressed. Thanks > Cheers, > > -- > Julien Grall
On Thu, Aug 1, 2019 at 5:50 PM Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> wrote: > >> In this case we also can declare and use intrs[] in the same way. > > > > There is no guarantee the index in irq will match intrs[...]. So you > > need to keep them hardcoded in the latter case. > Oh, right. I don't like the idea of using hardcoded numbers in the code. BTW, Misra rule says it should not be used as well. However, in this case, I did not change it, because it should be done in another patch. Anyway, hardcoded numbers should be avoided in the code, it seems this is a good candidate for a new Xen coding style rule if we want to achieve Misra compliance someday. Thanks
Hi, On 01/08/2019 17:56, Viktor Mitin wrote: > On Thu, Aug 1, 2019 at 5:50 PM Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com> wrote: > >>>> In this case we also can declare and use intrs[] in the same way. >>> >>> There is no guarantee the index in irq will match intrs[...]. So you >>> need to keep them hardcoded in the latter case. >> Oh, right. > > I don't like the idea of using hardcoded numbers in the code. BTW, > Misra rule says it should not be used as well. When mentioning a spec, it is common to also specify the exact section so others don't have to spend time look for it. I skimmed quickly through the MISRA and can't find the rule you suggest here. Furthermore, they have a lot of examples in the spec with harcoded size. So I am perplexed they actively discourage it... Cheers,
Hi Viktor, On 01/08/2019 17:46, Viktor Mitin wrote: > On Thu, Aug 1, 2019 at 4:58 PM Julien Grall <julien.grall@arm.com> wrote: >> >> >> >> On 01/08/2019 14:57, Julien Grall wrote: > >>>>> + unsigned int irq[MAX_TIMER_PPI]; >>>> As I said in the previous version, you are wasting stack space >>>> there. Also, this is misleading. When I see array of 4 items, I expect >>>> that all 4 items will be used. But you are using only 3, so it leads to >>>> two conclusions: either you made a mistake, or I don't understand what >>>> it happening. Either option is bad. >>> >>> 4 byte on a stack of 16KB... that's not really a waste worth an argument. The >>> more the stack is pretty empty as this is boot. So yes, you will not use the >>> last index because you don't expose hypervisor timer to guest yet! (Imagine >>> nested virt). But at least it avoids hardcoding a number of index and match the >>> enum. >> >> I forgot to mention. @Viktor, it is good to try to reply to each comment at >> least those you don't plan to address. So the reviewer doesn't have the feeling >> comments are ignored... > > Well, I address each of the comments or write about it explicitly in > other cases. > In this particular case, I'd added '-1', but later did not merge it > due to mistake. > So it supposed to be the next: > + unsigned int irq[MAX_TIMER_PPI-1] Please no '-1', it is worst than hardcoding value. In the code you are using an element of an enum to access the array. There are no guarantee the last element is actually the one you want to drop and therefore you risk to overflow it if mistakenly used. The risk is not worth compare to saving just 4-byte on the stack. Cheers,
On Fri, Aug 2, 2019 at 12:41 PM Julien Grall <julien.grall@arm.com> wrote: > > > > Well, I address each of the comments or write about it explicitly in > > other cases. > > In this particular case, I'd added '-1', but later did not merge it > > due to mistake. > > So it supposed to be the next: > > + unsigned int irq[MAX_TIMER_PPI-1] > > Please no '-1', it is worst than hardcoding value. In the code you are using an > element of an enum to access the array. There are no guarantee the last element > is actually the one you want to drop and therefore you risk to overflow it if > mistakenly used. > I agree that using -1 is not the best idea. It would be better to introduce a new enum for that. However, since we already have the enum with 4 items for that, it is better to use it as is. > The risk is not worth compare to saving just 4-byte on the stack. Completely agree about it, so I will use MAX_TIMER_PPI(as it is done now) in the next patch series version, Thanks
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index bc7d17dd2c..58542130ca 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -973,10 +973,8 @@ static int __init make_timer_node(const struct kernel_info *kinfo) { /* sentinel */ }, }; struct dt_device_node *dev; - u32 len; - const void *compatible; int res; - unsigned int irq; + unsigned int irq[MAX_TIMER_PPI]; gic_interrupt_t intrs[3]; u32 clock_frequency; bool clock_valid; @@ -990,35 +988,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; - - /* The timer IRQ is emulated by Xen. It always exposes an active-low - * level-sensitive interrupt */ - - irq = timer_get_irq(TIMER_PHYS_SECURE_PPI); - dt_dprintk(" Secure interrupt %u\n", irq); - set_interrupt(intrs[0], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW); + 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; + } - irq = timer_get_irq(TIMER_PHYS_NONSECURE_PPI); - dt_dprintk(" Non secure interrupt %u\n", irq); - set_interrupt(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW); + /* + * The timer IRQ is emulated by Xen. + * It always exposes an active-low level-sensitive interrupt + */ - irq = timer_get_irq(TIMER_VIRT_PPI); - dt_dprintk(" Virt interrupt %u\n", irq); - set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW); + if ( is_hardware_domain(kinfo->d) ) + { + irq[TIMER_PHYS_SECURE_PPI] = timer_get_irq(TIMER_PHYS_SECURE_PPI); + irq[TIMER_PHYS_NONSECURE_PPI] = + timer_get_irq(TIMER_PHYS_NONSECURE_PPI); + irq[TIMER_VIRT_PPI] = timer_get_irq(TIMER_VIRT_PPI); + } + else + { + irq[TIMER_PHYS_SECURE_PPI] = GUEST_TIMER_PHYS_S_PPI; + irq[TIMER_PHYS_NONSECURE_PPI] = GUEST_TIMER_PHYS_NS_PPI; + irq[TIMER_VIRT_PPI] = GUEST_TIMER_VIRT_PPI; + } + dt_dprintk(" Secure interrupt %u\n", irq[TIMER_PHYS_SECURE_PPI]); + set_interrupt(intrs[0], irq[TIMER_PHYS_SECURE_PPI], + 0xf, DT_IRQ_TYPE_LEVEL_LOW); + dt_dprintk(" Non secure interrupt %u\n", irq[TIMER_PHYS_NONSECURE_PPI]); + set_interrupt(intrs[1], irq[TIMER_PHYS_NONSECURE_PPI], + 0xf, DT_IRQ_TYPE_LEVEL_LOW); + dt_dprintk(" Virt interrupt %u\n", irq[TIMER_VIRT_PPI]); + set_interrupt(intrs[2], irq[TIMER_VIRT_PPI], 0xf, DT_IRQ_TYPE_LEVEL_LOW); res = fdt_property_interrupts(kinfo, intrs, 3); if ( res ) @@ -1603,46 +1615,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 +1720,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;
Functions make_timer_node and make_timer_domU_node are quite similar. So it is better to consolidate them to avoid discrepancy. The main difference between the functions is the timer interrupts used. Keep the domU version for the compatible as it is simpler. Mean do not copy platform's 'compatible' property into hwdom device tree, instead set either arm,armv7-timer or arm,armv8-timer, depending on the platform type. Keep the hw version for the clock as it is relevant for the both cases. The new function has changed prototype due to fdt_property_interrupts make_timer_node(const struct kernel_info *kinfo) Suggested-by: Julien Grall <julien.grall@arm.com> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com> --- v4 updates: updated "Keep the domU version for the compatible as it is simpler" v5 updates: - changed 'kept' to 'keep', etc. - removed empty line - updated indentation of parameters in functions calls - fixed NITs - updated commit message --- xen/arch/arm/domain_build.c | 106 +++++++++++++----------------------- 1 file changed, 39 insertions(+), 67 deletions(-)