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

[Suriyan Ramasami <suriyan.r@gmail.com>]

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