Xen project Mailing List

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

To: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>

From: Suriyan Ramasami <suriyan.r@xxxxxxxxx>

Date: Wed, 17 Sep 2014 08:38:48 -0700

Cc: Keir Fraser <keir@xxxxxxx>, Ian Campbell <ian.campbell@xxxxxxxxxx>, Julien Grall <julien.grall@xxxxxxxxxx>, Tim Deegan <tim@xxxxxxx>, "xen-devel@xxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, ian.jackson@xxxxxxxxxx

Delivery-date: Wed, 17 Sep 2014 15:39:10 +0000

List-id: Xen developer discussion <xen-devel.lists.xen.org>

On Wed, Sep 17, 2014 at 1:37 AM, Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx> 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@xxxxxxxxx> > wrote: >> >> On Fri, Sep 12, 2014 at 4:52 PM, Julien Grall <julien.grall@xxxxxxxxxx> >> 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@xxxxxxxxxxxxx >> http://lists.xen.org/xen-devel > > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.