[PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Julien Grall]

When the property "clock-frequency" is present in the DT timer node, it
means that the bootloader/firmware didn't correctly configured the
CNTFRQ/CNTFRQ_EL0 on each processor.

The best solution would be to fix the offending firmware/bootloader,
although it may not always be possible to modify and re-flash it.

As it's not possible to trap the register CNTFRQ/CNTFRQ_EL0, we have
to extend xen_arch_domainconfig to provide the timer frequency to the
toolstack when the property "clock-frequency" is present to the host DT
timer node. Then, a property "clock-frequency" will be created in the guest
DT timer node if the value is not 0.

We could have set the property in the guest DT no matter if the property
is present in the host DT. Although, we still want to let the guest
using CNTFRQ in normal case. After all, the property "clock-frequency"
is just a workaround for buggy firmware.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Cc: Chris Brand <chris.brand@broadcom.com>
---
 tools/libxl/libxl_arm.c       |  9 +++++++--
 xen/arch/arm/domain.c         |  2 +-
 xen/arch/arm/time.c           |  5 +++++
 xen/arch/arm/vtimer.c         |  4 +++-
 xen/arch/arm/vtimer.h         |  3 ++-
 xen/include/asm-arm/time.h    |  6 ++++++
 xen/include/public/arch-arm.h | 14 ++++++++++++++
 7 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 4fb5e26..f09c860 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -444,7 +444,9 @@ static int make_gicv3_node(libxl__gc *gc, void *fdt)
     return 0;
 }
 
-static int make_timer_node(libxl__gc *gc, void *fdt, const struct arch_info *ainfo)
+static int make_timer_node(libxl__gc *gc, void *fdt,
+                           const struct arch_info *ainfo,
+                           uint32_t frequency)
 {
     int res;
     gic_interrupt ints[3];
@@ -462,6 +464,9 @@ static int make_timer_node(libxl__gc *gc, void *fdt, const struct arch_info *ain
     res = fdt_property_interrupts(gc, fdt, ints, 3);
     if (res) return res;
 
+    if ( frequency )
+        fdt_property_u32(fdt, "clock-frequency", frequency);
+
     res = fdt_end_node(fdt);
     if (res) return res;
 
@@ -805,7 +810,7 @@ next_resize:
             goto out;
         }
 
-        FDT( make_timer_node(gc, fdt, ainfo) );
+        FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) );
         FDT( make_hypervisor_node(gc, fdt, vers) );
 
         if (pfdt)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 24b8938..8b1bf5a 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -593,7 +593,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     if ( (rc = domain_vgic_init(d, config->nr_spis)) != 0 )
         goto fail;
 
-    if ( (rc = domain_vtimer_init(d)) != 0 )
+    if ( (rc = domain_vtimer_init(d, config)) != 0 )
         goto fail;
 
     /*
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index ce6d3fd..5ded30c 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -42,6 +42,8 @@ uint64_t __read_mostly boot_count;
  * register-mapped time source in the SoC. */
 unsigned long __read_mostly cpu_khz;  /* CPU clock frequency in kHz. */
 
+uint32_t __read_mostly timer_dt_clock_frequency;
+
 static unsigned int timer_irq[MAX_TIMER_PPI];
 
 unsigned int timer_get_irq(enum timer_ppi ppi)
@@ -86,7 +88,10 @@ void __init preinit_xen_time(void)
 
     res = dt_property_read_u32(timer, "clock-frequency", &rate);
     if ( res )
+    {
         cpu_khz = rate / 1000;
+        timer_dt_clock_frequency = rate;
+    }
     else
         cpu_khz = READ_SYSREG32(CNTFRQ_EL0) / 1000;
 
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 685bfea..1418092 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -60,11 +60,13 @@ static void virt_timer_expired(void *data)
     perfc_incr(vtimer_virt_inject);
 }
 
-int domain_vtimer_init(struct domain *d)
+int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
 {
     d->arch.phys_timer_base.offset = NOW();
     d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
 
+    config->clock_frequency = timer_dt_clock_frequency;
+
     /* At this stage vgic_reserve_virq can't fail */
     if ( is_hardware_domain(d) )
     {
diff --git a/xen/arch/arm/vtimer.h b/xen/arch/arm/vtimer.h
index 6d2e46e..99e8145 100644
--- a/xen/arch/arm/vtimer.h
+++ b/xen/arch/arm/vtimer.h
@@ -20,7 +20,8 @@
 #ifndef __ARCH_ARM_VTIMER_H__
 #define __ARCH_ARM_VTIMER_H__
 
-extern int domain_vtimer_init(struct domain *d);
+extern int domain_vtimer_init(struct domain *d,
+                              struct xen_arch_domainconfig *config);
 extern int vcpu_vtimer_init(struct vcpu *v);
 extern int vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr);
 extern int virt_timer_save(struct vcpu *v);
diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
index 039039a..d755f36 100644
--- a/xen/include/asm-arm/time.h
+++ b/xen/include/asm-arm/time.h
@@ -22,6 +22,12 @@ enum timer_ppi
     MAX_TIMER_PPI = 4,
 };
 
+/*
+ * Value of "clock-frequency" in the DT timer node if present.
+ * 0 means the property doesn't exist.
+ */
+extern uint32_t timer_dt_clock_frequency;
+
 /* Get one of the timer IRQ number */
 unsigned int timer_get_irq(enum timer_ppi ppi);
 
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index a320851..a819196 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -311,6 +311,20 @@ struct xen_arch_domainconfig {
     uint8_t gic_version;
     /* IN */
     uint32_t nr_spis;
+    /*
+     * OUT
+     * Based on the property clock-frequency in the DT timer node.
+     * The property may be present when the bootloader/firmware doesn't
+     * set correctly CNTFRQ which hold the timer frequency.
+     *
+     * As it's not possible to trap this register, we have to replicate
+     * the value in the guest DT.
+     *
+     * = 0 => property not present
+     * > 0 => Value of the property
+     *
+     */
+    uint32_t clock_frequency;
 };
 #endif /* __XEN__ || __XEN_TOOLS__ */
 
-- 
2.1.4

Re: [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Chris (Christopher) Brand]

Hi Julien,

>When the property "clock-frequency" is present in the DT timer node, it means that the bootloader/firmware didn't correctly configured the
>CNTFRQ/CNTFRQ_EL0 on each processor.

I will test this patch, but it doesn't apply cleanly to the version of Xen I'm currently using, so I need to update that first.

I also looked at whether it would be possible to set the CNTFRQ register in the other cores when they come up. Eventually, I think we should do this in the (platform-specific) PSCI code. There doesn't seem to be a suitable hook in the platform-specific Xen code - it looks like all the code there related to bringing up secondary cores runs on the primary.

Chris

Re: [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Julien Grall]

On 03/06/2015 23:45, Chris (Christopher) Brand wrote:
> Hi Julien,

Hi Chris,

>> When the property "clock-frequency" is present in the DT timer node, it means that the bootloader/firmware didn't correctly configured the
>> CNTFRQ/CNTFRQ_EL0 on each processor.
> I will test this patch, but it doesn't apply cleanly to the version of Xen I'm currently using, so I need to update that first.

It's based on the latest staging.

> I also looked at whether it would be possible to set the CNTFRQ register in the other cores when they come up. Eventually, I think we should do this in the (platform-specific) PSCI code. There doesn't seem to be a suitable hook in the platform-specific Xen code - it looks like all the code there related to bringing up secondary cores runs on the primary.

CNTFRQ is read-only in the hypervisor when the platform supports EL3 
(i.e secure monitor).

So it has to be done either by the bootloader or by the firmware.

Regards,


-- 
Julien Grall

Re: [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Chris (Christopher) Brand]

Hi Julien,

>When the property "clock-frequency" is present in the DT timer node, it means that the bootloader/firmware didn't correctly configured the
CNTFRQ/CNTFRQ_EL0 on each processor.

I did try this out, and it didn't affect my results. I don't understand why, though :-)

What I see is that in preinit_xen_time(), the call to dt_property_read_u32() returns zero. When I built Xen, I set CONFIG_DTB_FILE, and looking at the corresponding dts file it has a timer node with a clock-frequency property. I know that our bootloader also creates a DTB, though, and it looks like that one does *not* have a clock-frequency property in the timer node, so I guess Xen ends up using that one somehow. CNTFRQ on core 0 (only) is also set to the correct frequency, so I end up with the correct frequency in my Dom0 kernel anyway.

I'm guessing this is me not understanding what's happening in my system rather than anything wrong with the patch.

Chris

Re: [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Julien Grall]

On 05/06/2015 21:14, Chris (Christopher) Brand wrote:
> Hi Julien,

Hi Chris,

>> When the property "clock-frequency" is present in the DT timer node, it means that the bootloader/firmware didn't correctly configured the
> CNTFRQ/CNTFRQ_EL0 on each processor.
>
> I did try this out, and it didn't affect my results. I don't understand why, though :-)

My test was limited as I don't have a platform where CNTFRQ/CNTFRQ_EL0 
is not valid. I may have done a mistake in the code.

> What I see is that in preinit_xen_time(), the call to dt_property_read_u32() returns zero. When I built Xen, I set CONFIG_DTB_FILE, and looking at the corresponding dts file it has a timer node with a clock-frequency property. I know that our bootloader also creates a DTB, though, and it looks like that one does *not* have a clock-frequency property in the timer node, so I guess Xen ends up using that one somehow. CNTFRQ on core 0 (only) is also set to the correct frequency, so I end up with the correct frequency in my Dom0 kernel anyway.

dt_property_read_u32 returns 0 when it cannot find a property or because 
the size of the value is not valid.

The device tree provided via CONFIG_DTB_FILE will always take precedence 
to the one pass by the bootloader.

How do you set CONFIG_DTB_FILE?

I would also look to see if by any chance the wrong device tree is set 
via CONFIG_DTB_FILE.

You can check what is the device tree used by dumping it from DOM0. 
Thought, it may be slightly different (some nodes are rewritten). You 
can dump it using dtc -I fs /proc/device-tree -O dts

Regards,

[1] 
http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot

-- 
Julien Grall

Re: [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Chris (Christopher) Brand]

Hi Julien,

>My test was limited as I don't have a platform where CNTFRQ/CNTFRQ_EL0 is not valid. I may have done a mistake in the code.

Understood. That's why I thought it would be worthwhile posting my results :-)

>> What I see is that in preinit_xen_time(), the call to dt_property_read_u32() returns zero. When I built Xen, I set CONFIG_DTB_FILE, and looking at the corresponding dts file it has a timer node with a clock-frequency property. I know that our bootloader also creates a DTB, though, and it looks like that one does *not* have a clock-frequency property in the timer node, so I guess Xen ends up using that one somehow. CNTFRQ on core 0 (only) is also set to the correct frequency, so I end up with the correct frequency in my Dom0 kernel anyway.
>
>dt_property_read_u32 returns 0 when it cannot find a property or because the size of the value is not valid.
>
>The device tree provided via CONFIG_DTB_FILE will always take precedence to the one pass by the bootloader.
>
>How do you set CONFIG_DTB_FILE?

A simple "export CONFIG_DTB_FILE=...".

>I would also look to see if by any chance the wrong device tree is set via CONFIG_DTB_FILE.

I did double-check before I posted, and everything looks right to me. I certainly could have missed something. It does look like it's *somehow* getting the DT generated by the bootloader.

>You can check what is the device tree used by dumping it from DOM0. 
>Thought, it may be slightly different (some nodes are rewritten). You can dump it using dtc -I fs /proc/device-tree -O dts

When I do that (at a shell prompt in Dom0), the timer node does not have a clock-frequency property. So your patch isn't designed to help in this case (CNTFRQ set on core 0 only, no clock-frequency property in timer node of DT).

Chris

Re: [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Julien Grall]

On 08/06/2015 16:44, Chris (Christopher) Brand wrote:
> Hi Julien,

Hi Chris,

>> My test was limited as I don't have a platform where CNTFRQ/CNTFRQ_EL0 is not valid. I may have done a mistake in the code.
>
> Understood. That's why I thought it would be worthwhile posting my results :-)
>
>>> What I see is that in preinit_xen_time(), the call to dt_property_read_u32() returns zero. When I built Xen, I set CONFIG_DTB_FILE, and looking at the corresponding dts file it has a timer node with a clock-frequency property. I know that our bootloader also creates a DTB, though, and it looks like that one does *not* have a clock-frequency property in the timer node, so I guess Xen ends up using that one somehow. CNTFRQ on core 0 (only) is also set to the correct frequency, so I end up with the correct frequency in my Dom0 kernel anyway.
>>
>> dt_property_read_u32 returns 0 when it cannot find a property or because the size of the value is not valid.
>>
>> The device tree provided via CONFIG_DTB_FILE will always take precedence to the one pass by the bootloader.
>>
>> How do you set CONFIG_DTB_FILE?
>
> A simple "export CONFIG_DTB_FILE=...".

Are you using an absolute path?

>> I would also look to see if by any chance the wrong device tree is set via CONFIG_DTB_FILE.
>
> I did double-check before I posted, and everything looks right to me. I certainly could have missed something. It does look like it's *somehow* getting the DT generated by the bootloader.
>
>> You can check what is the device tree used by dumping it from DOM0.
>> Thought, it may be slightly different (some nodes are rewritten). You can dump it using dtc -I fs /proc/device-tree -O dts
>
> When I do that (at a shell prompt in Dom0), the timer node does not have a clock-frequency property. So your patch isn't designed to help in this case (CNTFRQ set on core 0 only, no clock-frequency property in timer node of DT).

Well, my patch is designed to propagate the "clock-frequency" property 
in DOMU via the toolstack. The propagation for Dom0 is already present 
in Xen since last year [1].

I don't usually use CONFIG_DTB_FILE because my bootloader (u-boot via a 
script) is creating the different node for Xen (multiboot, chosen,...). 
Although, the CONFIG_DTB_FILE implementation in Xen is very obvious 
(only 2 lines which override the register which store the pointer in 
head.S).

Does your bootloader create Xen nodes (multiboot, chosen, ...) or the 
device tree provided by the bootloader contain such nodes? If not, then 
you are using an appended DT (via CONFIG_DTB_FILE) to Xen.

If you didn't yet do it, I would try to clean Xen repository and try to 
rebuild Xen to see if the build system was using a stall DTB by mistake.

Regards,

[1] 94191dcee8ff9f8b925824e54741f28b954bf95e "xen/arm: Pass the timer 
"clock-frequency" to DOM0 in make_timer_node()"

-- 
Julien Grall

Re: [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Julien Grall]

Hi,

On 09/06/2015 19:48, Julien Grall wrote:
> I don't usually use CONFIG_DTB_FILE because my bootloader (u-boot via a
> script) is creating the different node for Xen (multiboot, chosen,...).
> Although, the CONFIG_DTB_FILE implementation in Xen is very obvious
> (only 2 lines which override the register which store the pointer in
> head.S).

FWIW, I did try to use it today via the export solution and it's working 
perfectly.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Chris (Christopher) Brand]

Hi Julien,

>> A simple "export CONFIG_DTB_FILE=...".
>
>Are you using an absolute path?

Absolute path, yes.

>> So your patch isn't designed to help in this case (CNTFRQ set on core 0 only, no clock-frequency property in timer node of DT).
>
>Well, my patch is designed to propagate the "clock-frequency" property in DOMU via the toolstack. The propagation for Dom0 is already present in Xen since last year [1].

Understood. In my case, the DomU kernel dies during boot if it happens to be scheduled on a core other than core 0 (it comes up fine if it happens to run on core 0). Core 0 has CNTFRQ set, but the other cores don't. We will fix this for real by writing to CNTFRQ on the other cores in the PSCI code in the SMC.

>Does your bootloader create Xen nodes (multiboot, chosen, ...) or the device tree provided by the bootloader contain such nodes? If not, then you are using an appended DT (via CONFIG_DTB_FILE) to Xen.

It doesn't do that, no. In that case the mystery is why Xen apparently doesn't see the clock-frequency property. I'll keep investigating.

>If you didn't yet do it, I would try to clean Xen repository and try to rebuild Xen to see if the build system was using a stall DTB by mistake.

I have, but I'll do it again.

Chris

Re: [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Chris (Christopher) Brand]

Hi Julien,

The patch does work exactly as advertised.

When I used dtc to convert CONFIG_DTB_FILE from dtb to dts, I could see that it didn't in fact have a timer clock-frequency node. After re-creating the dtb and rebuilding Xen, "ls /proc/device-tree/timer/" shows a clock-frequency file. When I then fire up DomU and do the same command, it too has a clock-frequency node.

Thanks,

Chris

Re: [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Julien Grall]

On 11/06/2015 17:43, Chris (Christopher) Brand wrote:
> Hi Julien,

Hi Chris,

> The patch does work exactly as advertised.
>
> When I used dtc to convert CONFIG_DTB_FILE from dtb to dts, I could see that it didn't in fact have a timer clock-frequency node. After re-creating the dtb and rebuilding Xen, "ls /proc/device-tree/timer/" shows a clock-frequency file. When I then fire up DomU and do the same command, it too has a clock-frequency node.

Can I get your Tested-by on this patch?

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Chris (Christopher) Brand]

>Can I get your Tested-by on this patch?

Tested-by: Chris Brand <chris.brand@broadcom.com>

Chris

Re: [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Ian Campbell]

On Wed, 2015-06-03 at 15:48 +0100, Julien Grall wrote:
> When the property "clock-frequency" is present in the DT timer node, it
> means that the bootloader/firmware didn't correctly configured the

"configure"

> CNTFRQ/CNTFRQ_EL0 on each processor.
> 
> The best solution would be to fix the offending firmware/bootloader,
> although it may not always be possible to modify and re-flash it.
> 
> As it's not possible to trap the register CNTFRQ/CNTFRQ_EL0, we have
> to extend xen_arch_domainconfig to provide the timer frequency to the
> toolstack when the property "clock-frequency" is present to the host DT
> timer node. Then, a property "clock-frequency" will be created in the guest
> DT timer node if the value is not 0.
> 
> We could have set the property in the guest DT no matter if the property
> is present in the host DT. Although, we still want to let the guest
> using CNTFRQ in normal case. After all, the property "clock-frequency"
> is just a workaround for buggy firmware.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Cc: Chris Brand <chris.brand@broadcom.com>

I was about to apply but:

libxl_arm.c: In function ‘make_timer_node’:
libxl_arm.c:468:9: error: implicit declaration of function ‘fdt_property_u32’ [-Werror=implicit-function-declaration]
cc1: all warnings being treated as errors

That happen on both my 32 bit build (Debian Wheezy) and my 64 bit on
(Ubuntu Saucy).

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Julien Grall]

Hi Ian,

On 17/06/15 11:57, Ian Campbell wrote:
> On Wed, 2015-06-03 at 15:48 +0100, Julien Grall wrote:
>> When the property "clock-frequency" is present in the DT timer node, it
>> means that the bootloader/firmware didn't correctly configured the
> 
> "configure"
> 
>> CNTFRQ/CNTFRQ_EL0 on each processor.
>>
>> The best solution would be to fix the offending firmware/bootloader,
>> although it may not always be possible to modify and re-flash it.
>>
>> As it's not possible to trap the register CNTFRQ/CNTFRQ_EL0, we have
>> to extend xen_arch_domainconfig to provide the timer frequency to the
>> toolstack when the property "clock-frequency" is present to the host DT
>> timer node. Then, a property "clock-frequency" will be created in the guest
>> DT timer node if the value is not 0.
>>
>> We could have set the property in the guest DT no matter if the property
>> is present in the host DT. Although, we still want to let the guest
>> using CNTFRQ in normal case. After all, the property "clock-frequency"
>> is just a workaround for buggy firmware.
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>> Cc: Chris Brand <chris.brand@broadcom.com>
> 
> I was about to apply but:
> 
> libxl_arm.c: In function ‘make_timer_node’:
> libxl_arm.c:468:9: error: implicit declaration of function ‘fdt_property_u32’ [-Werror=implicit-function-declaration]
> cc1: all warnings being treated as errors
> 
> That happen on both my 32 bit build (Debian Wheezy) and my 64 bit on
> (Ubuntu Saucy).

Hmmm... I was expecting to this helper present on libfdt for a long
time. But it seems to be present only from 1.4.

Given the size of the helper (2 line), I'm thinking to open-code and
call directly fdt_property:

static inline int fdt_property_u32(void *fdt, const char *name, uint32_t
val)
{
        fdt32_t tmp = cpu_to_fdt32(val);
        return fdt_property(fdt, name, &tmp, sizeof(tmp));
}

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel