[XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware

[XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware

[Suriyan Ramasami]

The existence of secure firmware is dictated by the presence of
"samsung,secure-firmware" in the DT.

The Arndale board does not have that entry, and uses the address as defined
in "samsung,exynos4210-sysram", offset 0 as the smp init address. This is
possibly true for all SoCs without secure firmware.

For other boards which do have a "secure-firmware" node, use sysram-ns
at offset +0x1c as the smp init address.

Have tested this on the Odroid XU. I have also tested the other code path
on the Odroid XU by removing "secure-firmware" from its DT. I could see
that the other code path was exercised with correct smp init address
values.

Signed-off-by: Suriyan Ramasami <suriyan.r@gmail.com>
---

This patch is based off of the staging area.

Changes between versions as follows:
v2:
Consolidate exynos_smp_init_getbaseandoffset().
Use the size from the DT as mapping length conistently.
Do not check if the poking addresses are within size. We just expect it
to be.
Add smc call in cpu bring up code. Linux does this call irrespective of
the presence of "secure-firmware" in the DT.
Blacklist "samsung,secure-firmware".
Change subject line appropriately.

v1:
Unbreak Arndale XEN boot.

Remove BUG_ON for checking the address offsets that we poke with.
Instead use the size field from the DT to make valid comaprisons.
---
 xen/arch/arm/platforms/exynos5.c | 98 ++++++++++++++++++++++++++++------------
 1 file changed, 70 insertions(+), 28 deletions(-)

diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index 22a38f0..8fb301b 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -28,11 +28,16 @@
 #include <asm/platform.h>
 #include <asm/io.h>
 
+/* This corresponds to CONFIG_NR_CPUS in linux config */
+#define EXYNOS_CONFIG_NR_CPUS       0x08
+
 #define EXYNOS_ARM_CORE0_CONFIG     0x2000
-#define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr))
+#define EXYNOS_ARM_CORE_CONFIG(_nr) (EXYNOS_ARM_CORE0_CONFIG + (0x80 * (_nr)))
 #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) + 0x4)
 #define S5P_CORE_LOCAL_PWR_EN       0x3
 
+#define SMC_CMD_CPU1BOOT            (-4)
+
 static int exynos5_init_time(void)
 {
     uint32_t reg;
@@ -42,8 +47,6 @@ static int exynos5_init_time(void)
     u64 mct_base_addr;
     u64 size;
 
-    BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE);
-
     node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-mct");
     if ( !node )
     {
@@ -61,7 +64,7 @@ static int exynos5_init_time(void)
     dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
             mct_base_addr, size);
 
-    mct = ioremap_attr(mct_base_addr, PAGE_SIZE, PAGE_HYPERVISOR_NOCACHE);
+    mct = ioremap_attr(mct_base_addr, size, PAGE_HYPERVISOR_NOCACHE);
     if ( !mct )
     {
         dprintk(XENLOG_ERR, "Unable to map MCT\n");
@@ -91,32 +94,62 @@ static int exynos5250_specific_mapping(struct domain *d)
     return 0;
 }
 
-static int __init exynos5_smp_init(void)
+static int exynos_smp_init_getbasesizeoffset(u64 *sysram_addr,
+                                             u64 *size,
+                                             u64 *sysram_offset)
 {
-    void __iomem *sysram;
     struct dt_device_node *node;
-    u64 sysram_ns_base_addr;
-    u64 size;
     int rc;
+    char *compatible;
+
+    node = dt_find_compatible_node(NULL, NULL, "samsung,secure-firmware");
+    if ( node )
+    {
+        /* Have to use sysram_ns_base_addr + 0x1c for boot address */
+        compatible = "samsung,exynos4210-sysram-ns";
+        *sysram_offset = 0x1c;
+        printk("Running under secure firmware.\n");
+    }
+    else
+    {
+        /* Have to use sysram_base_addr + offset 0 for boot address */
+        compatible = "samsung,exynos4210-sysram";
+        *sysram_offset = 0;
+    }
 
-    node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-sysram");
+    node = dt_find_compatible_node(NULL, NULL, compatible);
     if ( !node )
     {
-        dprintk(XENLOG_ERR, "samsung,exynos4210-sysram-ns missing in DT\n");
+        dprintk(XENLOG_ERR, "%s missing in DT\n", compatible);
         return -ENXIO;
     }
 
-    rc = dt_device_get_address(node, 0, &sysram_ns_base_addr, &size);
+    rc = dt_device_get_address(node, 0, sysram_addr, size);
     if ( rc )
     {
-        dprintk(XENLOG_ERR, "Error in \"samsung,exynos4210-sysram-ns\"\n");
+        dprintk(XENLOG_ERR, "Error in %s\n", compatible);
         return -ENXIO;
     }
 
-    dprintk(XENLOG_INFO, "sysram_ns_base_addr: %016llx size: %016llx\n",
-            sysram_ns_base_addr, size);
+    return 0;
+}
 
-    sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE);
+static int __init exynos5_smp_init(void)
+{
+    void __iomem *sysram;
+    u64 sysram_addr;
+    u64 size;
+    u64 sysram_offset;
+    int rc;
+
+    rc = exynos_smp_init_getbasesizeoffset(&sysram_addr, &size, &sysram_offset);
+    if ( rc )
+        return rc;
+
+    dprintk(XENLOG_INFO, "sysram_addr: %016llx size: %016llx offset: %016llx\n",
+            sysram_addr, size, sysram_offset);
+
+    sysram = ioremap_nocache(sysram_addr, size);
     if ( !sysram )
     {
         dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
@@ -125,7 +158,7 @@ static int __init exynos5_smp_init(void)
 
     printk("Set SYSRAM to %"PRIpaddr" (%p)\n",
            __pa(init_secondary), init_secondary);
-    writel(__pa(init_secondary), sysram);
+    writel(__pa(init_secondary), sysram + sysram_offset);
 
     iounmap(sysram);
 
@@ -135,7 +168,7 @@ static int __init exynos5_smp_init(void)
 static int exynos_cpu_power_state(void __iomem *power, int cpu)
 {
     return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
-                       S5P_CORE_LOCAL_PWR_EN;
+           S5P_CORE_LOCAL_PWR_EN;
 }
 
 static void exynos_cpu_power_up(void __iomem *power, int cpu)
@@ -171,8 +204,7 @@ static int exynos5_cpu_power_up(void __iomem *power, int cpu)
     return 0;
 }
 
-static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
-    u64 size;
+static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64 *size) {
     struct dt_device_node *node;
     int rc;
     static const struct dt_device_match exynos_dt_pmu_matches[] __initconst =
@@ -190,7 +222,7 @@ static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
         return -ENXIO;
     }
 
-    rc = dt_device_get_address(node, 0, power_base_addr, &size);
+    rc = dt_device_get_address(node, 0, power_base_addr, size);
     if ( rc )
     {
         dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXX-pmu\"\n");
@@ -198,23 +230,31 @@ static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
     }
 
     dprintk(XENLOG_DEBUG, "power_base_addr: %016llx size: %016llx\n",
-            *power_base_addr, size);
+            *power_base_addr, *size);
 
     return 0;
 }
 
+static void exynos_smc(u32 cmd, u32 arg1, u32 arg2, u32 arg3)
+{
+    asm(
+        "dsb;"
+        "smc    #0;"
+    );
+}
+
 static int exynos5_cpu_up(int cpu)
 {
     u64 power_base_addr;
+    u64 size;
     void __iomem *power;
     int rc;
 
-    rc = exynos5_get_pmu_base_addr(&power_base_addr);
+    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
     if ( rc )
         return rc;
 
-    power = ioremap_nocache(power_base_addr +
-                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
+    power = ioremap_nocache(power_base_addr, size);
     if ( !power )
     {
         dprintk(XENLOG_ERR, "Unable to map power MMIO\n");
@@ -230,22 +270,23 @@ static int exynos5_cpu_up(int cpu)
 
     iounmap(power);
 
+    exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0);
+
     return cpu_up_send_sgi(cpu);
 }
 
 static void exynos5_reset(void)
 {
     u64 power_base_addr;
+    u64 size;
     void __iomem *pmu;
     int rc;
 
-    BUILD_BUG_ON(EXYNOS5_SWRESET >= PAGE_SIZE);
-
-    rc = exynos5_get_pmu_base_addr(&power_base_addr);
+    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
     if ( rc )
         return;
 
-    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
+    pmu = ioremap_nocache(power_base_addr, size);
     if ( !pmu )
     {
         dprintk(XENLOG_ERR, "Unable to map PMU\n");
@@ -264,6 +305,7 @@ static const struct dt_device_match exynos5_blacklist_dev[] __initconst =
      * This is result to random freeze.
      */
     DT_MATCH_COMPATIBLE("samsung,exynos4210-mct"),
+    DT_MATCH_COMPATIBLE("samsung,secure-firmware"),
     { /* sentinel */ },
 };
 
-- 
1.9.1

Re: [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware

[Julien Grall]

Hi Suriyan,

On 12/09/14 16:01, Suriyan Ramasami wrote:
> +static int __init exynos5_smp_init(void)
> +{
> +    void __iomem *sysram;
> +    u64 sysram_addr;
> +    u64 size;
> +    u64 sysram_offset;
> +    int rc;
> +
> +    rc = exynos_smp_init_getbasesizeoffset(&sysram_addr, &size, &sysram_offset);

The function name is odd. As you call the function only here, couldn't 
you inline it?

> +    if ( rc )
> +        return rc;
> +
> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx size: %016llx offset: %016llx\n",
> +            sysram_addr, size, sysram_offset);
> +
> +    sysram = ioremap_nocache(sysram_addr, size);
>       if ( !sysram )
>       {
>           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
> @@ -125,7 +158,7 @@ static int __init exynos5_smp_init(void)
>
>       printk("Set SYSRAM to %"PRIpaddr" (%p)\n",
>              __pa(init_secondary), init_secondary);
> -    writel(__pa(init_secondary), sysram);
> +    writel(__pa(init_secondary), sysram + sysram_offset);
>
>       iounmap(sysram);
>
> @@ -135,7 +168,7 @@ static int __init exynos5_smp_init(void)
>   static int exynos_cpu_power_state(void __iomem *power, int cpu)
>   {
>       return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
> -                       S5P_CORE_LOCAL_PWR_EN;
> +           S5P_CORE_LOCAL_PWR_EN;

Why this change?

>   }
>
>   static void exynos_cpu_power_up(void __iomem *power, int cpu)
> @@ -171,8 +204,7 @@ static int exynos5_cpu_power_up(void __iomem *power, int cpu)
>       return 0;
>   }
>
> -static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
> -    u64 size;
> +static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64 *size) {

The Xen coding style is

static int foo(...)
{

>       struct dt_device_node *node;
>       int rc;
>       static const struct dt_device_match exynos_dt_pmu_matches[] __initconst =
> @@ -190,7 +222,7 @@ static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
>           return -ENXIO;
>       }
>
> -    rc = dt_device_get_address(node, 0, power_base_addr, &size);
> +    rc = dt_device_get_address(node, 0, power_base_addr, size);
>       if ( rc )
>       {
>           dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXX-pmu\"\n");
> @@ -198,23 +230,31 @@ static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
>       }
>
>       dprintk(XENLOG_DEBUG, "power_base_addr: %016llx size: %016llx\n",
> -            *power_base_addr, size);
> +            *power_base_addr, *size);
>
>       return 0;
>   }
>
> +static void exynos_smc(u32 cmd, u32 arg1, u32 arg2, u32 arg3)
> +{
> +    asm(
> +        "dsb;"
> +        "smc    #0;"
> +    );
> +}
>+

The compiler may decide to inline the function. This will end up to the 
command register not in register r0.

Give a look to __invoke_psci_fn_smc in xen/arch/arm/psci.c. It might be 
worth to introduce an SMC helper.

>   static int exynos5_cpu_up(int cpu)
>   {
>       u64 power_base_addr;
> +    u64 size;
>       void __iomem *power;
>       int rc;
>
> -    rc = exynos5_get_pmu_base_addr(&power_base_addr);
> +    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
>       if ( rc )
>           return rc;
>
> -    power = ioremap_nocache(power_base_addr +
> -                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
> +    power = ioremap_nocache(power_base_addr, size);
>       if ( !power )
>       {
>           dprintk(XENLOG_ERR, "Unable to map power MMIO\n");
> @@ -230,22 +270,23 @@ static int exynos5_cpu_up(int cpu)
>
>       iounmap(power);
>
> +    exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0);
> +

The call is not done unconditionally on Linux. It's only done when the 
secure firmware is present.

>       return cpu_up_send_sgi(cpu);
>   }
>
>   static void exynos5_reset(void)
>   {
>       u64 power_base_addr;
> +    u64 size;
>       void __iomem *pmu;
>       int rc;
>
> -    BUILD_BUG_ON(EXYNOS5_SWRESET >= PAGE_SIZE);
> -
> -    rc = exynos5_get_pmu_base_addr(&power_base_addr);
> +    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
>       if ( rc )
>           return;
>
> -    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
> +    pmu = ioremap_nocache(power_base_addr, size);
>       if ( !pmu )
>       {
>           dprintk(XENLOG_ERR, "Unable to map PMU\n");
> @@ -264,6 +305,7 @@ static const struct dt_device_match exynos5_blacklist_dev[] __initconst =
>        * This is result to random freeze.
>        */
>       DT_MATCH_COMPATIBLE("samsung,exynos4210-mct"),
> +    DT_MATCH_COMPATIBLE("samsung,secure-firmware"),

Can you add a comment explaining why we blacklist the secure firmware?

Regards,

-- 
Julien Grall

Re: [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware

[Suriyan Ramasami]

On Fri, Sep 12, 2014 at 4:52 PM, Julien Grall <julien.grall@linaro.org> wrote:
> Hi Suriyan,
>
Hello Julien,

> On 12/09/14 16:01, Suriyan Ramasami wrote:
>>
>> +static int __init exynos5_smp_init(void)
>> +{
>> +    void __iomem *sysram;
>> +    u64 sysram_addr;
>> +    u64 size;
>> +    u64 sysram_offset;
>> +    int rc;
>> +
>> +    rc = exynos_smp_init_getbasesizeoffset(&sysram_addr, &size,
>> &sysram_offset);
>
>
> The function name is odd. As you call the function only here, couldn't you
> inline it?
>
OK, I shall do that.

>> +    if ( rc )
>> +        return rc;
>> +
>> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx size: %016llx offset:
>> %016llx\n",
>> +            sysram_addr, size, sysram_offset);
>> +
>> +    sysram = ioremap_nocache(sysram_addr, size);
>>       if ( !sysram )
>>       {
>>           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
>> @@ -125,7 +158,7 @@ static int __init exynos5_smp_init(void)
>>
>>       printk("Set SYSRAM to %"PRIpaddr" (%p)\n",
>>              __pa(init_secondary), init_secondary);
>> -    writel(__pa(init_secondary), sysram);
>> +    writel(__pa(init_secondary), sysram + sysram_offset);
>>
>>       iounmap(sysram);
>>
>> @@ -135,7 +168,7 @@ static int __init exynos5_smp_init(void)
>>   static int exynos_cpu_power_state(void __iomem *power, int cpu)
>>   {
>>       return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
>> -                       S5P_CORE_LOCAL_PWR_EN;
>> +           S5P_CORE_LOCAL_PWR_EN;
>
>
> Why this change?
>
We are anding the result of the readl, and hence as its outside of the
readl (and not a parameter to it), I aligned it as such. Is that not
right? Cause, if I align it under the ( of readl, it will appear as if
it was a parameter to readl. Please let me know.

>>   }
>>
>>   static void exynos_cpu_power_up(void __iomem *power, int cpu)
>> @@ -171,8 +204,7 @@ static int exynos5_cpu_power_up(void __iomem *power,
>> int cpu)
>>       return 0;
>>   }
>>
>> -static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
>> -    u64 size;
>> +static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64 *size) {
>
>
> The Xen coding style is
>
> static int foo(...)
> {
>
Sorry, forgot the coding style in a momentary lapse of reason :-)

>>       struct dt_device_node *node;
>>       int rc;
>>       static const struct dt_device_match exynos_dt_pmu_matches[]
>> __initconst =
>> @@ -190,7 +222,7 @@ static int exynos5_get_pmu_base_addr(u64
>> *power_base_addr) {
>>           return -ENXIO;
>>       }
>>
>> -    rc = dt_device_get_address(node, 0, power_base_addr, &size);
>> +    rc = dt_device_get_address(node, 0, power_base_addr, size);
>>       if ( rc )
>>       {
>>           dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXX-pmu\"\n");
>> @@ -198,23 +230,31 @@ static int exynos5_get_pmu_base_addr(u64
>> *power_base_addr) {
>>       }
>>
>>       dprintk(XENLOG_DEBUG, "power_base_addr: %016llx size: %016llx\n",
>> -            *power_base_addr, size);
>> +            *power_base_addr, *size);
>>
>>       return 0;
>>   }
>>
>> +static void exynos_smc(u32 cmd, u32 arg1, u32 arg2, u32 arg3)
>> +{
>> +    asm(
>> +        "dsb;"
>> +        "smc    #0;"
>> +    );
>> +}
>> +
>
>
> The compiler may decide to inline the function. This will end up to the
> command register not in register r0.
>
> Give a look to __invoke_psci_fn_smc in xen/arch/arm/psci.c. It might be
> worth to introduce an SMC helper.
>
OK, will check that out.

>>   static int exynos5_cpu_up(int cpu)
>>   {
>>       u64 power_base_addr;
>> +    u64 size;
>>       void __iomem *power;
>>       int rc;
>>
>> -    rc = exynos5_get_pmu_base_addr(&power_base_addr);
>> +    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
>>       if ( rc )
>>           return rc;
>>
>> -    power = ioremap_nocache(power_base_addr +
>> -                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
>> +    power = ioremap_nocache(power_base_addr, size);
>>       if ( !power )
>>       {
>>           dprintk(XENLOG_ERR, "Unable to map power MMIO\n");
>> @@ -230,22 +270,23 @@ static int exynos5_cpu_up(int cpu)
>>
>>       iounmap(power);
>>
>> +    exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0);
>> +
>
>
> The call is not done unconditionally on Linux. It's only done when the
> secure firmware is present.
>
You are right again. I shall update the comment, and probably do the
call only if its under secure firmware.

>>       return cpu_up_send_sgi(cpu);
>>   }
>>
>>   static void exynos5_reset(void)
>>   {
>>       u64 power_base_addr;
>> +    u64 size;
>>       void __iomem *pmu;
>>       int rc;
>>
>> -    BUILD_BUG_ON(EXYNOS5_SWRESET >= PAGE_SIZE);
>> -
>> -    rc = exynos5_get_pmu_base_addr(&power_base_addr);
>> +    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
>>       if ( rc )
>>           return;
>>
>> -    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
>> +    pmu = ioremap_nocache(power_base_addr, size);
>>       if ( !pmu )
>>       {
>>           dprintk(XENLOG_ERR, "Unable to map PMU\n");
>> @@ -264,6 +305,7 @@ static const struct dt_device_match
>> exynos5_blacklist_dev[] __initconst =
>>        * This is result to random freeze.
>>        */
>>       DT_MATCH_COMPATIBLE("samsung,exynos4210-mct"),
>> +    DT_MATCH_COMPATIBLE("samsung,secure-firmware"),
>
>
> Can you add a comment explaining why we blacklist the secure firmware?
>
I shall add your comment in.
Thanks!
- Suriyan

> Regards,
>
> --
> Julien Grall

Re: [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 9310 bytes --]

Hi all,
just wanted to let you know that right now xen reboot is broken on Arndale
as of 72af6f455ac6afcd46d9a556f90349f2397507e8.

[   24.559917] reboot: Restarting system
(XEN) Domain 0 shutdown: rebooting machine.
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ----[ Xen-4.5-unstable  arm32  debug=y  Tainted:    C ]----
(XEN) CPU:    0
(XEN) PC:     00205e0c dt_match_node+0xb0/0x120
(XEN) CPSR:   200f01da MODE:Hypervisor
(XEN)      R0: 00288204 R1: 40010000 R2: 00003333 R3: 002b8048
(XEN)      R4: 40010000 R5: 00288214 R6: 00288214 R7: 40010000
(XEN)      R8: 00000001 R9: 97666000 R10:00000000 R11:4002fe04 R12:00005555
(XEN) HYP: SP: 4002fde4 LR: 00205ec8
(XEN)
(XEN)   VTCR_EL2: 80003558
(XEN)  VTTBR_EL2: 00010000bfefe000
(XEN)
(XEN)  SCTLR_EL2: 30cd187f
(XEN)    HCR_EL2: 000000000038643f
(XEN)  TTBR0_EL2: 00000000bfef0000
(XEN)
(XEN)    ESR_EL2: 94000007
(XEN)  HPFAR_EL2: 0000000000000000
(XEN)      HDFAR: 00288204
(XEN)      HIFAR: 00000000
(XEN)
(XEN) Xen stack trace from sp=4002fde4:
(XEN)    97dc6800 40010000 00288204 00000064 000003e8 4003d0d8 97666000
4002fe14
(XEN)    00205ec8 4002fe40 000003e8 4002fe3c 00260b34 ba05e9b2 00000006
00000001
(XEN)    000003e8 4003d0d8 97666000 4002fe3c 00276b80 4002fe4c 00260bec
000003e8
(XEN)    4003d0d8 4002fe54 002583b4 4002fe6c 0025946c 00000001 00000001
4003d000
(XEN)    00000003 4002fe74 0022dee0 4002fe94 0020925c 4002ff58 8000ed24
00000000
(XEN)    00000003 00000ea1 97666000 4002fedc 0022ce6c 00000000 00000004
00000001
(XEN)    002f8508 4002fecc 00250b88 4002ff58 80abfa44 00000000 00000003
4002ff58
(XEN)    8000ed24 00000000 00000003 00000ea1 97666000 4002ff54 0025b9f0
00000000
(XEN)    0024fa7c 4002ff0c 200f01da 00000004 002c1ff0 002be000 002f9594
00276b80
(XEN)    002c1ff0 00000004 40033000 00000000 0000000a 00000000 0000000a
00000029
(XEN)    00000000 00000000 80afb1ac 4002ff44 00000000 80abfa44 00000000
00000003
(XEN)    fee1dead 97666000 00000000 4002ff58 0025f350 00000002 97667e34
8000ec18
(XEN)    00000001 00000000 80abfa44 00000000 00000003 fee1dead 97666000
00000000
(XEN)    97667e44 0000001d ffffffff 76f3d51d 8000ed24 600f0093 00000000
7edefcbc
(XEN)    80af7b80 800146c0 97667e30 8000ec44 80af7b8c 80014800 80af7b98
800148a0
(XEN)    00000000 00000000 00000000 00000000 00000000 00000000 00000000
600f0013
(XEN)    600b0193 600b0093 600f0193 00000000 00000000 00000000 00000000
(XEN) Xen call trace:
(XEN)    [<00205e0c>] dt_match_node+0xb0/0x120 (PC)
(XEN)    [<00205ec8>] dt_find_matching_node+0x4c/0x84 (LR)
(XEN)    [<00205ec8>] dt_find_matching_node+0x4c/0x84
(XEN)    [<00260b34>] exynos5_get_pmu_base_addr+0x28/0xc8
(XEN)    [<00260bec>] exynos5_reset+0x18/0x7c
(XEN)    [<002583b4>] platform_reset+0x30/0x40
(XEN)    [<0025946c>] machine_restart+0xa0/0xb8
(XEN)    [<0022dee0>] hwdom_shutdown+0x64/0x88
(XEN)    [<0020925c>] domain_shutdown+0x58/0xf8
(XEN)    [<0022ce6c>] do_sched_op+0xf4/0x6c4
(XEN)    [<0025b9f0>] do_trap_hypervisor+0xe40/0x1184
(XEN)    [<0025f350>] return_from_trap+0/0x4
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN)
(XEN) ****************************************
(XEN)
(XEN) Manual reset required ('noreboot' specified)

Tamas


On Sat, Sep 13, 2014 at 4:08 AM, Suriyan Ramasami <suriyan.r@gmail.com>
wrote:

> On Fri, Sep 12, 2014 at 4:52 PM, Julien Grall <julien.grall@linaro.org>
> wrote:
> > Hi Suriyan,
> >
> Hello Julien,
>
> > On 12/09/14 16:01, Suriyan Ramasami wrote:
> >>
> >> +static int __init exynos5_smp_init(void)
> >> +{
> >> +    void __iomem *sysram;
> >> +    u64 sysram_addr;
> >> +    u64 size;
> >> +    u64 sysram_offset;
> >> +    int rc;
> >> +
> >> +    rc = exynos_smp_init_getbasesizeoffset(&sysram_addr, &size,
> >> &sysram_offset);
> >
> >
> > The function name is odd. As you call the function only here, couldn't
> you
> > inline it?
> >
> OK, I shall do that.
>
> >> +    if ( rc )
> >> +        return rc;
> >> +
> >> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx size: %016llx offset:
> >> %016llx\n",
> >> +            sysram_addr, size, sysram_offset);
> >> +
> >> +    sysram = ioremap_nocache(sysram_addr, size);
> >>       if ( !sysram )
> >>       {
> >>           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
> >> @@ -125,7 +158,7 @@ static int __init exynos5_smp_init(void)
> >>
> >>       printk("Set SYSRAM to %"PRIpaddr" (%p)\n",
> >>              __pa(init_secondary), init_secondary);
> >> -    writel(__pa(init_secondary), sysram);
> >> +    writel(__pa(init_secondary), sysram + sysram_offset);
> >>
> >>       iounmap(sysram);
> >>
> >> @@ -135,7 +168,7 @@ static int __init exynos5_smp_init(void)
> >>   static int exynos_cpu_power_state(void __iomem *power, int cpu)
> >>   {
> >>       return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
> >> -                       S5P_CORE_LOCAL_PWR_EN;
> >> +           S5P_CORE_LOCAL_PWR_EN;
> >
> >
> > Why this change?
> >
> We are anding the result of the readl, and hence as its outside of the
> readl (and not a parameter to it), I aligned it as such. Is that not
> right? Cause, if I align it under the ( of readl, it will appear as if
> it was a parameter to readl. Please let me know.
>
> >>   }
> >>
> >>   static void exynos_cpu_power_up(void __iomem *power, int cpu)
> >> @@ -171,8 +204,7 @@ static int exynos5_cpu_power_up(void __iomem *power,
> >> int cpu)
> >>       return 0;
> >>   }
> >>
> >> -static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
> >> -    u64 size;
> >> +static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64
> *size) {
> >
> >
> > The Xen coding style is
> >
> > static int foo(...)
> > {
> >
> Sorry, forgot the coding style in a momentary lapse of reason :-)
>
> >>       struct dt_device_node *node;
> >>       int rc;
> >>       static const struct dt_device_match exynos_dt_pmu_matches[]
> >> __initconst =
> >> @@ -190,7 +222,7 @@ static int exynos5_get_pmu_base_addr(u64
> >> *power_base_addr) {
> >>           return -ENXIO;
> >>       }
> >>
> >> -    rc = dt_device_get_address(node, 0, power_base_addr, &size);
> >> +    rc = dt_device_get_address(node, 0, power_base_addr, size);
> >>       if ( rc )
> >>       {
> >>           dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXX-pmu\"\n");
> >> @@ -198,23 +230,31 @@ static int exynos5_get_pmu_base_addr(u64
> >> *power_base_addr) {
> >>       }
> >>
> >>       dprintk(XENLOG_DEBUG, "power_base_addr: %016llx size: %016llx\n",
> >> -            *power_base_addr, size);
> >> +            *power_base_addr, *size);
> >>
> >>       return 0;
> >>   }
> >>
> >> +static void exynos_smc(u32 cmd, u32 arg1, u32 arg2, u32 arg3)
> >> +{
> >> +    asm(
> >> +        "dsb;"
> >> +        "smc    #0;"
> >> +    );
> >> +}
> >> +
> >
> >
> > The compiler may decide to inline the function. This will end up to the
> > command register not in register r0.
> >
> > Give a look to __invoke_psci_fn_smc in xen/arch/arm/psci.c. It might be
> > worth to introduce an SMC helper.
> >
> OK, will check that out.
>
> >>   static int exynos5_cpu_up(int cpu)
> >>   {
> >>       u64 power_base_addr;
> >> +    u64 size;
> >>       void __iomem *power;
> >>       int rc;
> >>
> >> -    rc = exynos5_get_pmu_base_addr(&power_base_addr);
> >> +    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
> >>       if ( rc )
> >>           return rc;
> >>
> >> -    power = ioremap_nocache(power_base_addr +
> >> -                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
> >> +    power = ioremap_nocache(power_base_addr, size);
> >>       if ( !power )
> >>       {
> >>           dprintk(XENLOG_ERR, "Unable to map power MMIO\n");
> >> @@ -230,22 +270,23 @@ static int exynos5_cpu_up(int cpu)
> >>
> >>       iounmap(power);
> >>
> >> +    exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0);
> >> +
> >
> >
> > The call is not done unconditionally on Linux. It's only done when the
> > secure firmware is present.
> >
> You are right again. I shall update the comment, and probably do the
> call only if its under secure firmware.
>
> >>       return cpu_up_send_sgi(cpu);
> >>   }
> >>
> >>   static void exynos5_reset(void)
> >>   {
> >>       u64 power_base_addr;
> >> +    u64 size;
> >>       void __iomem *pmu;
> >>       int rc;
> >>
> >> -    BUILD_BUG_ON(EXYNOS5_SWRESET >= PAGE_SIZE);
> >> -
> >> -    rc = exynos5_get_pmu_base_addr(&power_base_addr);
> >> +    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
> >>       if ( rc )
> >>           return;
> >>
> >> -    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
> >> +    pmu = ioremap_nocache(power_base_addr, size);
> >>       if ( !pmu )
> >>       {
> >>           dprintk(XENLOG_ERR, "Unable to map PMU\n");
> >> @@ -264,6 +305,7 @@ static const struct dt_device_match
> >> exynos5_blacklist_dev[] __initconst =
> >>        * This is result to random freeze.
> >>        */
> >>       DT_MATCH_COMPATIBLE("samsung,exynos4210-mct"),
> >> +    DT_MATCH_COMPATIBLE("samsung,secure-firmware"),
> >
> >
> > Can you add a comment explaining why we blacklist the secure firmware?
> >
> I shall add your comment in.
> Thanks!
> - Suriyan
>
> > Regards,
> >
> > --
> > Julien Grall
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

[-- Attachment #1.2: Type: text/html, Size: 12026 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware

[Suriyan Ramasami]

On Wed, Sep 17, 2014 at 1:37 AM, Tamas K Lengyel
<tamas.lengyel@zentific.com> wrote:
> Hi all,
> just wanted to let you know that right now xen reboot is broken on Arndale
> as of 72af6f455ac6afcd46d9a556f90349f2397507e8.
>
> [   24.559917] reboot: Restarting system
> (XEN) Domain 0 shutdown: rebooting machine.
> (XEN) CPU0: Unexpected Trap: Data Abort
> (XEN) ----[ Xen-4.5-unstable  arm32  debug=y  Tainted:    C ]----
> (XEN) CPU:    0
> (XEN) PC:     00205e0c dt_match_node+0xb0/0x120
> (XEN) CPSR:   200f01da MODE:Hypervisor
> (XEN)      R0: 00288204 R1: 40010000 R2: 00003333 R3: 002b8048
> (XEN)      R4: 40010000 R5: 00288214 R6: 00288214 R7: 40010000
> (XEN)      R8: 00000001 R9: 97666000 R10:00000000 R11:4002fe04 R12:00005555
> (XEN) HYP: SP: 4002fde4 LR: 00205ec8
> (XEN)
> (XEN)   VTCR_EL2: 80003558
> (XEN)  VTTBR_EL2: 00010000bfefe000
> (XEN)
> (XEN)  SCTLR_EL2: 30cd187f
> (XEN)    HCR_EL2: 000000000038643f
> (XEN)  TTBR0_EL2: 00000000bfef0000
> (XEN)
> (XEN)    ESR_EL2: 94000007
> (XEN)  HPFAR_EL2: 0000000000000000
> (XEN)      HDFAR: 00288204
> (XEN)      HIFAR: 00000000
> (XEN)
> (XEN) Xen stack trace from sp=4002fde4:
> (XEN)    97dc6800 40010000 00288204 00000064 000003e8 4003d0d8 97666000
> 4002fe14
> (XEN)    00205ec8 4002fe40 000003e8 4002fe3c 00260b34 ba05e9b2 00000006
> 00000001
> (XEN)    000003e8 4003d0d8 97666000 4002fe3c 00276b80 4002fe4c 00260bec
> 000003e8
> (XEN)    4003d0d8 4002fe54 002583b4 4002fe6c 0025946c 00000001 00000001
> 4003d000
> (XEN)    00000003 4002fe74 0022dee0 4002fe94 0020925c 4002ff58 8000ed24
> 00000000
> (XEN)    00000003 00000ea1 97666000 4002fedc 0022ce6c 00000000 00000004
> 00000001
> (XEN)    002f8508 4002fecc 00250b88 4002ff58 80abfa44 00000000 00000003
> 4002ff58
> (XEN)    8000ed24 00000000 00000003 00000ea1 97666000 4002ff54 0025b9f0
> 00000000
> (XEN)    0024fa7c 4002ff0c 200f01da 00000004 002c1ff0 002be000 002f9594
> 00276b80
> (XEN)    002c1ff0 00000004 40033000 00000000 0000000a 00000000 0000000a
> 00000029
> (XEN)    00000000 00000000 80afb1ac 4002ff44 00000000 80abfa44 00000000
> 00000003
> (XEN)    fee1dead 97666000 00000000 4002ff58 0025f350 00000002 97667e34
> 8000ec18
> (XEN)    00000001 00000000 80abfa44 00000000 00000003 fee1dead 97666000
> 00000000
> (XEN)    97667e44 0000001d ffffffff 76f3d51d 8000ed24 600f0093 00000000
> 7edefcbc
> (XEN)    80af7b80 800146c0 97667e30 8000ec44 80af7b8c 80014800 80af7b98
> 800148a0
> (XEN)    00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 600f0013
> (XEN)    600b0193 600b0093 600f0193 00000000 00000000 00000000 00000000
> (XEN) Xen call trace:
> (XEN)    [<00205e0c>] dt_match_node+0xb0/0x120 (PC)
> (XEN)    [<00205ec8>] dt_find_matching_node+0x4c/0x84 (LR)
> (XEN)    [<00205ec8>] dt_find_matching_node+0x4c/0x84
> (XEN)    [<00260b34>] exynos5_get_pmu_base_addr+0x28/0xc8
> (XEN)    [<00260bec>] exynos5_reset+0x18/0x7c
> (XEN)    [<002583b4>] platform_reset+0x30/0x40
> (XEN)    [<0025946c>] machine_restart+0xa0/0xb8
> (XEN)    [<0022dee0>] hwdom_shutdown+0x64/0x88
> (XEN)    [<0020925c>] domain_shutdown+0x58/0xf8
> (XEN)    [<0022ce6c>] do_sched_op+0xf4/0x6c4
> (XEN)    [<0025b9f0>] do_trap_hypervisor+0xe40/0x1184
> (XEN)    [<0025f350>] return_from_trap+0/0x4
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) CPU0: Unexpected Trap: Data Abort
> (XEN)
> (XEN) ****************************************
> (XEN)
> (XEN) Manual reset required ('noreboot' specified)
>
Hello Tamas,
The code path is the same for the XU and the Arndale for the reset.
Let me check into this.
Thanks
- Suriyan

> Tamas
>
>
> On Sat, Sep 13, 2014 at 4:08 AM, Suriyan Ramasami <suriyan.r@gmail.com>
> wrote:
>>
>> On Fri, Sep 12, 2014 at 4:52 PM, Julien Grall <julien.grall@linaro.org>
>> wrote:
>> > Hi Suriyan,
>> >
>> Hello Julien,
>>
>> > On 12/09/14 16:01, Suriyan Ramasami wrote:
>> >>
>> >> +static int __init exynos5_smp_init(void)
>> >> +{
>> >> +    void __iomem *sysram;
>> >> +    u64 sysram_addr;
>> >> +    u64 size;
>> >> +    u64 sysram_offset;
>> >> +    int rc;
>> >> +
>> >> +    rc = exynos_smp_init_getbasesizeoffset(&sysram_addr, &size,
>> >> &sysram_offset);
>> >
>> >
>> > The function name is odd. As you call the function only here, couldn't
>> > you
>> > inline it?
>> >
>> OK, I shall do that.
>>
>> >> +    if ( rc )
>> >> +        return rc;
>> >> +
>> >> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx size: %016llx offset:
>> >> %016llx\n",
>> >> +            sysram_addr, size, sysram_offset);
>> >> +
>> >> +    sysram = ioremap_nocache(sysram_addr, size);
>> >>       if ( !sysram )
>> >>       {
>> >>           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
>> >> @@ -125,7 +158,7 @@ static int __init exynos5_smp_init(void)
>> >>
>> >>       printk("Set SYSRAM to %"PRIpaddr" (%p)\n",
>> >>              __pa(init_secondary), init_secondary);
>> >> -    writel(__pa(init_secondary), sysram);
>> >> +    writel(__pa(init_secondary), sysram + sysram_offset);
>> >>
>> >>       iounmap(sysram);
>> >>
>> >> @@ -135,7 +168,7 @@ static int __init exynos5_smp_init(void)
>> >>   static int exynos_cpu_power_state(void __iomem *power, int cpu)
>> >>   {
>> >>       return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
>> >> -                       S5P_CORE_LOCAL_PWR_EN;
>> >> +           S5P_CORE_LOCAL_PWR_EN;
>> >
>> >
>> > Why this change?
>> >
>> We are anding the result of the readl, and hence as its outside of the
>> readl (and not a parameter to it), I aligned it as such. Is that not
>> right? Cause, if I align it under the ( of readl, it will appear as if
>> it was a parameter to readl. Please let me know.
>>
>> >>   }
>> >>
>> >>   static void exynos_cpu_power_up(void __iomem *power, int cpu)
>> >> @@ -171,8 +204,7 @@ static int exynos5_cpu_power_up(void __iomem
>> >> *power,
>> >> int cpu)
>> >>       return 0;
>> >>   }
>> >>
>> >> -static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
>> >> -    u64 size;
>> >> +static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64
>> >> *size) {
>> >
>> >
>> > The Xen coding style is
>> >
>> > static int foo(...)
>> > {
>> >
>> Sorry, forgot the coding style in a momentary lapse of reason :-)
>>
>> >>       struct dt_device_node *node;
>> >>       int rc;
>> >>       static const struct dt_device_match exynos_dt_pmu_matches[]
>> >> __initconst =
>> >> @@ -190,7 +222,7 @@ static int exynos5_get_pmu_base_addr(u64
>> >> *power_base_addr) {
>> >>           return -ENXIO;
>> >>       }
>> >>
>> >> -    rc = dt_device_get_address(node, 0, power_base_addr, &size);
>> >> +    rc = dt_device_get_address(node, 0, power_base_addr, size);
>> >>       if ( rc )
>> >>       {
>> >>           dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXX-pmu\"\n");
>> >> @@ -198,23 +230,31 @@ static int exynos5_get_pmu_base_addr(u64
>> >> *power_base_addr) {
>> >>       }
>> >>
>> >>       dprintk(XENLOG_DEBUG, "power_base_addr: %016llx size: %016llx\n",
>> >> -            *power_base_addr, size);
>> >> +            *power_base_addr, *size);
>> >>
>> >>       return 0;
>> >>   }
>> >>
>> >> +static void exynos_smc(u32 cmd, u32 arg1, u32 arg2, u32 arg3)
>> >> +{
>> >> +    asm(
>> >> +        "dsb;"
>> >> +        "smc    #0;"
>> >> +    );
>> >> +}
>> >> +
>> >
>> >
>> > The compiler may decide to inline the function. This will end up to the
>> > command register not in register r0.
>> >
>> > Give a look to __invoke_psci_fn_smc in xen/arch/arm/psci.c. It might be
>> > worth to introduce an SMC helper.
>> >
>> OK, will check that out.
>>
>> >>   static int exynos5_cpu_up(int cpu)
>> >>   {
>> >>       u64 power_base_addr;
>> >> +    u64 size;
>> >>       void __iomem *power;
>> >>       int rc;
>> >>
>> >> -    rc = exynos5_get_pmu_base_addr(&power_base_addr);
>> >> +    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
>> >>       if ( rc )
>> >>           return rc;
>> >>
>> >> -    power = ioremap_nocache(power_base_addr +
>> >> -                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
>> >> +    power = ioremap_nocache(power_base_addr, size);
>> >>       if ( !power )
>> >>       {
>> >>           dprintk(XENLOG_ERR, "Unable to map power MMIO\n");
>> >> @@ -230,22 +270,23 @@ static int exynos5_cpu_up(int cpu)
>> >>
>> >>       iounmap(power);
>> >>
>> >> +    exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0);
>> >> +
>> >
>> >
>> > The call is not done unconditionally on Linux. It's only done when the
>> > secure firmware is present.
>> >
>> You are right again. I shall update the comment, and probably do the
>> call only if its under secure firmware.
>>
>> >>       return cpu_up_send_sgi(cpu);
>> >>   }
>> >>
>> >>   static void exynos5_reset(void)
>> >>   {
>> >>       u64 power_base_addr;
>> >> +    u64 size;
>> >>       void __iomem *pmu;
>> >>       int rc;
>> >>
>> >> -    BUILD_BUG_ON(EXYNOS5_SWRESET >= PAGE_SIZE);
>> >> -
>> >> -    rc = exynos5_get_pmu_base_addr(&power_base_addr);
>> >> +    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
>> >>       if ( rc )
>> >>           return;
>> >>
>> >> -    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
>> >> +    pmu = ioremap_nocache(power_base_addr, size);
>> >>       if ( !pmu )
>> >>       {
>> >>           dprintk(XENLOG_ERR, "Unable to map PMU\n");
>> >> @@ -264,6 +305,7 @@ static const struct dt_device_match
>> >> exynos5_blacklist_dev[] __initconst =
>> >>        * This is result to random freeze.
>> >>        */
>> >>       DT_MATCH_COMPATIBLE("samsung,exynos4210-mct"),
>> >> +    DT_MATCH_COMPATIBLE("samsung,secure-firmware"),
>> >
>> >
>> > Can you add a comment explaining why we blacklist the secure firmware?
>> >
>> I shall add your comment in.
>> Thanks!
>> - Suriyan
>>
>> > Regards,
>> >
>> > --
>> > Julien Grall
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>

Re: [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware

[Suriyan Ramasami]

I have also trimmed the cc list so there is less annoyance :-)

On Wed, Sep 17, 2014 at 8:38 AM, Suriyan Ramasami <suriyan.r@gmail.com> wrote:
> On Wed, Sep 17, 2014 at 1:37 AM, Tamas K Lengyel
> <tamas.lengyel@zentific.com> wrote:
>> Hi all,
>> just wanted to let you know that right now xen reboot is broken on Arndale
>> as of 72af6f455ac6afcd46d9a556f90349f2397507e8.
>>
>> [   24.559917] reboot: Restarting system
>> (XEN) Domain 0 shutdown: rebooting machine.
>> (XEN) CPU0: Unexpected Trap: Data Abort
>> (XEN) ----[ Xen-4.5-unstable  arm32  debug=y  Tainted:    C ]----
>> (XEN) CPU:    0
>> (XEN) PC:     00205e0c dt_match_node+0xb0/0x120
>> (XEN) CPSR:   200f01da MODE:Hypervisor
>> (XEN)      R0: 00288204 R1: 40010000 R2: 00003333 R3: 002b8048
>> (XEN)      R4: 40010000 R5: 00288214 R6: 00288214 R7: 40010000
>> (XEN)      R8: 00000001 R9: 97666000 R10:00000000 R11:4002fe04 R12:00005555
>> (XEN) HYP: SP: 4002fde4 LR: 00205ec8
>> (XEN)
>> (XEN)   VTCR_EL2: 80003558
>> (XEN)  VTTBR_EL2: 00010000bfefe000
>> (XEN)
>> (XEN)  SCTLR_EL2: 30cd187f
>> (XEN)    HCR_EL2: 000000000038643f
>> (XEN)  TTBR0_EL2: 00000000bfef0000
>> (XEN)
>> (XEN)    ESR_EL2: 94000007
>> (XEN)  HPFAR_EL2: 0000000000000000
>> (XEN)      HDFAR: 00288204
>> (XEN)      HIFAR: 00000000
>> (XEN)
>> (XEN) Xen stack trace from sp=4002fde4:
>> (XEN)    97dc6800 40010000 00288204 00000064 000003e8 4003d0d8 97666000
>> 4002fe14
>> (XEN)    00205ec8 4002fe40 000003e8 4002fe3c 00260b34 ba05e9b2 00000006
>> 00000001
>> (XEN)    000003e8 4003d0d8 97666000 4002fe3c 00276b80 4002fe4c 00260bec
>> 000003e8
>> (XEN)    4003d0d8 4002fe54 002583b4 4002fe6c 0025946c 00000001 00000001
>> 4003d000
>> (XEN)    00000003 4002fe74 0022dee0 4002fe94 0020925c 4002ff58 8000ed24
>> 00000000
>> (XEN)    00000003 00000ea1 97666000 4002fedc 0022ce6c 00000000 00000004
>> 00000001
>> (XEN)    002f8508 4002fecc 00250b88 4002ff58 80abfa44 00000000 00000003
>> 4002ff58
>> (XEN)    8000ed24 00000000 00000003 00000ea1 97666000 4002ff54 0025b9f0
>> 00000000
>> (XEN)    0024fa7c 4002ff0c 200f01da 00000004 002c1ff0 002be000 002f9594
>> 00276b80
>> (XEN)    002c1ff0 00000004 40033000 00000000 0000000a 00000000 0000000a
>> 00000029
>> (XEN)    00000000 00000000 80afb1ac 4002ff44 00000000 80abfa44 00000000
>> 00000003
>> (XEN)    fee1dead 97666000 00000000 4002ff58 0025f350 00000002 97667e34
>> 8000ec18
>> (XEN)    00000001 00000000 80abfa44 00000000 00000003 fee1dead 97666000
>> 00000000
>> (XEN)    97667e44 0000001d ffffffff 76f3d51d 8000ed24 600f0093 00000000
>> 7edefcbc
>> (XEN)    80af7b80 800146c0 97667e30 8000ec44 80af7b8c 80014800 80af7b98
>> 800148a0
>> (XEN)    00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> 600f0013
>> (XEN)    600b0193 600b0093 600f0193 00000000 00000000 00000000 00000000
>> (XEN) Xen call trace:
>> (XEN)    [<00205e0c>] dt_match_node+0xb0/0x120 (PC)
>> (XEN)    [<00205ec8>] dt_find_matching_node+0x4c/0x84 (LR)
>> (XEN)    [<00205ec8>] dt_find_matching_node+0x4c/0x84
>> (XEN)    [<00260b34>] exynos5_get_pmu_base_addr+0x28/0xc8
>> (XEN)    [<00260bec>] exynos5_reset+0x18/0x7c
>> (XEN)    [<002583b4>] platform_reset+0x30/0x40
>> (XEN)    [<0025946c>] machine_restart+0xa0/0xb8
>> (XEN)    [<0022dee0>] hwdom_shutdown+0x64/0x88
>> (XEN)    [<0020925c>] domain_shutdown+0x58/0xf8
>> (XEN)    [<0022ce6c>] do_sched_op+0xf4/0x6c4
>> (XEN)    [<0025b9f0>] do_trap_hypervisor+0xe40/0x1184
>> (XEN)    [<0025f350>] return_from_trap+0/0x4
>> (XEN)
>> (XEN)
>> (XEN) ****************************************
>> (XEN) Panic on CPU 0:
>> (XEN) CPU0: Unexpected Trap: Data Abort
>> (XEN)
>> (XEN) ****************************************
>> (XEN)
>> (XEN) Manual reset required ('noreboot' specified)
>>
> Hello Tamas,
> The code path is the same for the XU and the Arndale for the reset.
> Let me check into this.


This also has been confirmed to happen on the Odroid XU.
I found that when __initconst is used for exynos_dt_pmu_matches[],
when the reset code is executed, the memory address for
exynos_dt_pmu_matches  when accessed generates a DATA abort. I wonder
if the area where ever the init variables are, are invalidated when
the dom0 is exiting? I am not sure.

If I remove __initconst from exynos_dt_pmu_mathces[] then there is no
such issue.

I am wondering if someone has some expert comments on the use of
__initconst and how that would effect the reset code?

Just for documentation, I get an address of 00280204 with __initcost
and an address of 0025d358 when its not used.

> Thanks
> - Suriyan
>
>> Tamas
>>
>>
>> On Sat, Sep 13, 2014 at 4:08 AM, Suriyan Ramasami <suriyan.r@gmail.com>
>> wrote:
>>>
>>> On Fri, Sep 12, 2014 at 4:52 PM, Julien Grall <julien.grall@linaro.org>
>>> wrote:
>>> > Hi Suriyan,
>>> >
>>> Hello Julien,
>>>
>>> > On 12/09/14 16:01, Suriyan Ramasami wrote:
>>> >>
>>> >> +static int __init exynos5_smp_init(void)
>>> >> +{
>>> >> +    void __iomem *sysram;
>>> >> +    u64 sysram_addr;
>>> >> +    u64 size;
>>> >> +    u64 sysram_offset;
>>> >> +    int rc;
>>> >> +
>>> >> +    rc = exynos_smp_init_getbasesizeoffset(&sysram_addr, &size,
>>> >> &sysram_offset);
>>> >
>>> >
>>> > The function name is odd. As you call the function only here, couldn't
>>> > you
>>> > inline it?
>>> >
>>> OK, I shall do that.
>>>
>>> >> +    if ( rc )
>>> >> +        return rc;
>>> >> +
>>> >> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx size: %016llx offset:
>>> >> %016llx\n",
>>> >> +            sysram_addr, size, sysram_offset);
>>> >> +
>>> >> +    sysram = ioremap_nocache(sysram_addr, size);
>>> >>       if ( !sysram )
>>> >>       {
>>> >>           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
>>> >> @@ -125,7 +158,7 @@ static int __init exynos5_smp_init(void)
>>> >>
>>> >>       printk("Set SYSRAM to %"PRIpaddr" (%p)\n",
>>> >>              __pa(init_secondary), init_secondary);
>>> >> -    writel(__pa(init_secondary), sysram);
>>> >> +    writel(__pa(init_secondary), sysram + sysram_offset);
>>> >>
>>> >>       iounmap(sysram);
>>> >>
>>> >> @@ -135,7 +168,7 @@ static int __init exynos5_smp_init(void)
>>> >>   static int exynos_cpu_power_state(void __iomem *power, int cpu)
>>> >>   {
>>> >>       return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
>>> >> -                       S5P_CORE_LOCAL_PWR_EN;
>>> >> +           S5P_CORE_LOCAL_PWR_EN;
>>> >
>>> >
>>> > Why this change?
>>> >
>>> We are anding the result of the readl, and hence as its outside of the
>>> readl (and not a parameter to it), I aligned it as such. Is that not
>>> right? Cause, if I align it under the ( of readl, it will appear as if
>>> it was a parameter to readl. Please let me know.
>>>
>>> >>   }
>>> >>
>>> >>   static void exynos_cpu_power_up(void __iomem *power, int cpu)
>>> >> @@ -171,8 +204,7 @@ static int exynos5_cpu_power_up(void __iomem
>>> >> *power,
>>> >> int cpu)
>>> >>       return 0;
>>> >>   }
>>> >>
>>> >> -static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
>>> >> -    u64 size;
>>> >> +static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64
>>> >> *size) {
>>> >
>>> >
>>> > The Xen coding style is
>>> >
>>> > static int foo(...)
>>> > {
>>> >
>>> Sorry, forgot the coding style in a momentary lapse of reason :-)
>>>
>>> >>       struct dt_device_node *node;
>>> >>       int rc;
>>> >>       static const struct dt_device_match exynos_dt_pmu_matches[]
>>> >> __initconst =
>>> >> @@ -190,7 +222,7 @@ static int exynos5_get_pmu_base_addr(u64
>>> >> *power_base_addr) {
>>> >>           return -ENXIO;
>>> >>       }
>>> >>
>>> >> -    rc = dt_device_get_address(node, 0, power_base_addr, &size);
>>> >> +    rc = dt_device_get_address(node, 0, power_base_addr, size);
>>> >>       if ( rc )
>>> >>       {
>>> >>           dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXX-pmu\"\n");
>>> >> @@ -198,23 +230,31 @@ static int exynos5_get_pmu_base_addr(u64
>>> >> *power_base_addr) {
>>> >>       }
>>> >>
>>> >>       dprintk(XENLOG_DEBUG, "power_base_addr: %016llx size: %016llx\n",
>>> >> -            *power_base_addr, size);
>>> >> +            *power_base_addr, *size);
>>> >>
>>> >>       return 0;
>>> >>   }
>>> >>
>>> >> +static void exynos_smc(u32 cmd, u32 arg1, u32 arg2, u32 arg3)
>>> >> +{
>>> >> +    asm(
>>> >> +        "dsb;"
>>> >> +        "smc    #0;"
>>> >> +    );
>>> >> +}
>>> >> +
>>> >
>>> >
>>> > The compiler may decide to inline the function. This will end up to the
>>> > command register not in register r0.
>>> >
>>> > Give a look to __invoke_psci_fn_smc in xen/arch/arm/psci.c. It might be
>>> > worth to introduce an SMC helper.
>>> >
>>> OK, will check that out.
>>>
>>> >>   static int exynos5_cpu_up(int cpu)
>>> >>   {
>>> >>       u64 power_base_addr;
>>> >> +    u64 size;
>>> >>       void __iomem *power;
>>> >>       int rc;
>>> >>
>>> >> -    rc = exynos5_get_pmu_base_addr(&power_base_addr);
>>> >> +    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
>>> >>       if ( rc )
>>> >>           return rc;
>>> >>
>>> >> -    power = ioremap_nocache(power_base_addr +
>>> >> -                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
>>> >> +    power = ioremap_nocache(power_base_addr, size);
>>> >>       if ( !power )
>>> >>       {
>>> >>           dprintk(XENLOG_ERR, "Unable to map power MMIO\n");
>>> >> @@ -230,22 +270,23 @@ static int exynos5_cpu_up(int cpu)
>>> >>
>>> >>       iounmap(power);
>>> >>
>>> >> +    exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0);
>>> >> +
>>> >
>>> >
>>> > The call is not done unconditionally on Linux. It's only done when the
>>> > secure firmware is present.
>>> >
>>> You are right again. I shall update the comment, and probably do the
>>> call only if its under secure firmware.
>>>
>>> >>       return cpu_up_send_sgi(cpu);
>>> >>   }
>>> >>
>>> >>   static void exynos5_reset(void)
>>> >>   {
>>> >>       u64 power_base_addr;
>>> >> +    u64 size;
>>> >>       void __iomem *pmu;
>>> >>       int rc;
>>> >>
>>> >> -    BUILD_BUG_ON(EXYNOS5_SWRESET >= PAGE_SIZE);
>>> >> -
>>> >> -    rc = exynos5_get_pmu_base_addr(&power_base_addr);
>>> >> +    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
>>> >>       if ( rc )
>>> >>           return;
>>> >>
>>> >> -    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
>>> >> +    pmu = ioremap_nocache(power_base_addr, size);
>>> >>       if ( !pmu )
>>> >>       {
>>> >>           dprintk(XENLOG_ERR, "Unable to map PMU\n");
>>> >> @@ -264,6 +305,7 @@ static const struct dt_device_match
>>> >> exynos5_blacklist_dev[] __initconst =
>>> >>        * This is result to random freeze.
>>> >>        */
>>> >>       DT_MATCH_COMPATIBLE("samsung,exynos4210-mct"),
>>> >> +    DT_MATCH_COMPATIBLE("samsung,secure-firmware"),
>>> >
>>> >
>>> > Can you add a comment explaining why we blacklist the secure firmware?
>>> >
>>> I shall add your comment in.
>>> Thanks!
>>> - Suriyan
>>>
>>> > Regards,
>>> >
>>> > --
>>> > Julien Grall
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
>>
>>

Re: [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware

[Julien Grall]

Hi Suriyan,

On 17/09/14 15:17, Suriyan Ramasami wrote:
>> The code path is the same for the XU and the Arndale for the reset.
>> Let me check into this.
>
>
> This also has been confirmed to happen on the Odroid XU.
> I found that when __initconst is used for exynos_dt_pmu_matches[],
> when the reset code is executed, the memory address for
> exynos_dt_pmu_matches  when accessed generates a DATA abort. I wonder
> if the area where ever the init variables are, are invalidated when
> the dom0 is exiting? I am not sure.
>
> If I remove __initconst from exynos_dt_pmu_mathces[] then there is no
> such issue.
>
> I am wondering if someone has some expert comments on the use of
> __initconst and how that would effect the reset code?

All __initconst variable will live to a separate section. once Xen has 
finished to boot, the section will be freed and Xen may use the memory 
for itself or the guest.

If you plan to use the variables during the reset, then you should drop 
the __initconst.

Regards,

-- 
Julien Grall

Re: [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware

[Suriyan Ramasami]

On Wed, Sep 17, 2014 at 3:23 PM, Julien Grall <julien.grall@linaro.org> wrote:
> Hi Suriyan,
>
> On 17/09/14 15:17, Suriyan Ramasami wrote:
>>>
>>> The code path is the same for the XU and the Arndale for the reset.
>>> Let me check into this.
>>
>>
>>
>> This also has been confirmed to happen on the Odroid XU.
>> I found that when __initconst is used for exynos_dt_pmu_matches[],
>> when the reset code is executed, the memory address for
>> exynos_dt_pmu_matches  when accessed generates a DATA abort. I wonder
>> if the area where ever the init variables are, are invalidated when
>> the dom0 is exiting? I am not sure.
>>
>> If I remove __initconst from exynos_dt_pmu_mathces[] then there is no
>> such issue.
>>
>> I am wondering if someone has some expert comments on the use of
>> __initconst and how that would effect the reset code?
>
>
> All __initconst variable will live to a separate section. once Xen has
> finished to boot, the section will be freed and Xen may use the memory for
> itself or the guest.
>
> If you plan to use the variables during the reset, then you should drop the
> __initconst.
>
Thank you Julien for the explanation. I shall drop __initconst for
exynos_dt_pmu_mathces[] as it is being reused after boot.

> Regards,
>
> --
> Julien Grall

Re: [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware

[Ian Campbell]

On Fri, 2014-09-12 at 16:01 -0700, Suriyan Ramasami wrote:

Mostly looks good, couple of comments below. I'll also try and give it a
spin on arndale when I'm back in the office early next week.

> +/* This corresponds to CONFIG_NR_CPUS in linux config */
> +#define EXYNOS_CONFIG_NR_CPUS       0x08

This doesn't appear to be used, which is good because it would be wrong
to hardcode such things into Xen.
> @@ -135,7 +168,7 @@ static int __init exynos5_smp_init(void)
>  static int exynos_cpu_power_state(void __iomem *power, int cpu)
>  {
>      return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
> -                       S5P_CORE_LOCAL_PWR_EN;
> +           S5P_CORE_LOCAL_PWR_EN;

Please avoid spurious whitespace changes (especially since this one is
wrong...)

> +static void exynos_smc(u32 cmd, u32 arg1, u32 arg2, u32 arg3)
> +{
> +    asm(
> +        "dsb;"
> +        "smc    #0;"
> +    );

I don't think this will work reliably in the face of compiler
optimisations. You need something like __invoke_psci_fn_smc. In fact it
would probably be best to refactor that into a common function for
calling into firmware (which looks like it might be a case of renaming
the existing fn and moving it somewhere more appropriate).

Ian.

Re: [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware

[Suriyan Ramasami]

On Thu, Sep 18, 2014 at 11:30 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Fri, 2014-09-12 at 16:01 -0700, Suriyan Ramasami wrote:
>
> Mostly looks good, couple of comments below. I'll also try and give it a
> spin on arndale when I'm back in the office early next week.
>
>> +/* This corresponds to CONFIG_NR_CPUS in linux config */
>> +#define EXYNOS_CONFIG_NR_CPUS       0x08
>
> This doesn't appear to be used, which is good because it would be wrong
> to hardcode such things into Xen.

I shall remove this #define.

>> @@ -135,7 +168,7 @@ static int __init exynos5_smp_init(void)
>>  static int exynos_cpu_power_state(void __iomem *power, int cpu)
>>  {
>>      return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
>> -                       S5P_CORE_LOCAL_PWR_EN;
>> +           S5P_CORE_LOCAL_PWR_EN;
>
> Please avoid spurious whitespace changes (especially since this one is
> wrong...)
>

Julien had commented on this too. My response was:
"We are anding the result of the readl, and hence as its outside of the
readl (and not a parameter to it), I aligned it as such. Is that not
right? Cause, if I align it under the ( of readl, it will appear as if
it was a parameter to readl. Please let me know."


>> +static void exynos_smc(u32 cmd, u32 arg1, u32 arg2, u32 arg3)
>> +{
>> +    asm(
>> +        "dsb;"
>> +        "smc    #0;"
>> +    );
>
> I don't think this will work reliably in the face of compiler
> optimisations. You need something like __invoke_psci_fn_smc. In fact it
> would probably be best to refactor that into a common function for
> calling into firmware (which looks like it might be a case of renaming
> the existing fn and moving it somewhere more appropriate).
>

Julien had commented on this too, and he too recommended I take a look
at __invoke_psci_fn_smc in xen/arch/arm/psci.c

I had taken these into consideration and submitted a v3 of this patch.

Thanks
- Suriyan

> Ian.
>

Re: [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware

[Ian Campbell]

On Thu, 2014-09-18 at 11:44 -0700, Suriyan Ramasami wrote:
> >> @@ -135,7 +168,7 @@ static int __init exynos5_smp_init(void)
> >>  static int exynos_cpu_power_state(void __iomem *power, int cpu)
> >>  {
> >>      return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
> >> -                       S5P_CORE_LOCAL_PWR_EN;
> >> +           S5P_CORE_LOCAL_PWR_EN;
> >
> > Please avoid spurious whitespace changes (especially since this one is
> > wrong...)
> >
> 
> Julien had commented on this too. My response was:
> "We are anding the result of the readl, and hence as its outside of the
> readl (and not a parameter to it), I aligned it as such. Is that not
> right? Cause, if I align it under the ( of readl, it will appear as if
> it was a parameter to readl. Please let me know."

Hrm, ok. But it's still a spurious change as far as this commit goes, we
generally try and avoid such things (and this patch is already skirting
close to changing too much in one go)

> >> +static void exynos_smc(u32 cmd, u32 arg1, u32 arg2, u32 arg3)
> >> +{
> >> +    asm(
> >> +        "dsb;"
> >> +        "smc    #0;"
> >> +    );
> >
> > I don't think this will work reliably in the face of compiler
> > optimisations. You need something like __invoke_psci_fn_smc. In fact it
> > would probably be best to refactor that into a common function for
> > calling into firmware (which looks like it might be a case of renaming
> > the existing fn and moving it somewhere more appropriate).
> >
> 
> Julien had commented on this too, and he too recommended I take a look
> at __invoke_psci_fn_smc in xen/arch/arm/psci.c
> 
> I had taken these into consideration and submitted a v3 of this patch.

Oh, I seem to have missed that, sorry.