|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware
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@xxxxxxxxx> wrote:
> 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.
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@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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |