Re: [Xen-devel] [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node
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 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 2/2] xen/arm: merge make_timer_node and make_timer_domU_node
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
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
Signed-off-by: Viktor Mitin
---
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", );
-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
Re: [Xen-devel] [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node
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
Signed-off-by: Viktor Mitin
---
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
Viktor Mitin writes:
> On Wed, Jul 31, 2019 at 3:33 PM Volodymyr Babchuk
> 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
>> > Signed-off-by: Viktor Mitin
>> > ---
>> > 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", );
>> > -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
Re: [Xen-devel] [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node
On Wed, Jul 31, 2019 at 3:33 PM Volodymyr Babchuk
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
> > Signed-off-by: Viktor Mitin
> > ---
> > 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", );
> > -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);
> > +
Re: [Xen-devel] [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node
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
> Signed-off-by: Viktor Mitin
> ---
> 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", );
> -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);
> -
[Xen-devel] [PATCH v4 2/2] xen/arm: merge make_timer_node and make_timer_domU_node
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
Signed-off-by: Viktor Mitin
---
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", );
-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 =
