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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

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@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