OP-TEE / optee_os

Trusted side of the TEE
Other
1.56k stars 1.05k forks source link

Secure timer interrupt in zcu102 #6916

Closed chiunyi23 closed 1 month ago

chiunyi23 commented 3 months ago

Hello, I am working on adding a secure timer interrupt in OP-TEE OS on ZCU102 development board. I've see the issues #6570, #2925, and #1701. I followed the codes from #6570, but it seems I missed something to let it work correctly.

[Prerequisites] optee os: commit d0b74 Development Environment: Xilinx MPSoC UltraScale+ ZCU102

file: /optee_os/core/kernel/sub.mk

...
srcs-y += timer_interrupt.c
...

file: /optee_os/core/kernel/timer_interrupt.c

#define GIC_SPI_SEC_PHY_TIMER   29
#define TIMER_PERIOD_MS 1000

static enum itr_return timer_itr_cb(struct itr_handler *h __unused)
{
    /* Reset timer for next FIQ */
    generic_timer_handler(TIMER_PERIOD_MS);

    /* Collect entropy on each timer FIQ */
    DMSG("[DEBUG] interrupt");

    return ITRR_HANDLED;
}

static struct itr_handler timer_itr = {
    .it = GIC_SPI_SEC_PHY_TIMER,
    .flags = BIT(0),
    .handler = timer_itr_cb,
};

static TEE_Result init_timer_itr(void)
{
    itr_add(&timer_itr);
    itr_enable(timer_itr.it);

    /* Enable timer FIQ to fetch entropy required during boot */
    generic_timer_start(TIMER_PERIOD_MS);

    return TEE_SUCCESS;
}
service_init(init_timer_itr);

This is the result I got after running script/symbolize.py with the Call statck.

E/TC:0 00 Call stack:
E/TC:0 00  0x60014d98 itr_add_type_prio at /xxx/optee_os/core/kernel/interrupt.c:107
E/TC:0 00  0x60019c2c itr_add at /xxx/optee_os/core/include/kernel/interrupt.h:120
E/TC:0 00  0x60017408 call_initcalls at /xxx/optee_os/core/kernel/initcall.c:41
E/TC:0 00  0x60008780 boot_init_primary_late at /xxx/optee_os/core/arch/arm/kernel/boot.c:12            97
E/TC:0 00 Panic 'unhandled pageable abort' at core/arch/arm/kernel/abort.c:553 <abort_handler>
E/TC:0 00 TEE load address @ 0x60000000
E/TC:0 00 Call stack:
E/TC:0 00  0x600088a4 print_kernel_stack at /xxx/optee_os/core/arch/arm/kernel/unwind_arm64.            c:80
E/TC:0 00  0x60015c70 __do_panic at /xxx/optee_os/core/kernel/panic.c:24
E/TC:0 00  0x600081dc get_fault_type at /xxx/optee_os/core/arch/arm/kernel/abort.c:476
E/TC:0 00  0x6000515c el1_sync_abort at /xxx/optee_os/core/arch/arm/kernel/thread_a64.S:747

It seems like the problem is at interrupt.c line 107: itr_chip->ops->add(itr_chip, h->it, type, prio);

The complete function is shown below

void itr_add_type_prio(struct itr_handler *h, uint32_t type, uint32_t prio)
{
    struct itr_handler __maybe_unused *hdl = NULL;

    SLIST_FOREACH(hdl, &handlers, link)
        if (hdl->it == h->it)
            assert((hdl->flags & ITRF_SHARED) &&
                   (h->flags & ITRF_SHARED));

    itr_chip->ops->add(itr_chip, h->it, type, prio);
    SLIST_INSERT_HEAD(&handlers, h, link);
}

I can't figure out why this is happening; could anyone help with this?

jenswi-linaro commented 3 months ago

It looks like a data abort, a few lines further up in the log you will probably see that. I suspect that itr_chip is NULL, you can check in the disassembly exactly which instruction causes the data abort. Perhaps you haven't called itr_init()?

chiunyi23 commented 3 months ago

@jenswi-linaro Thank you for the quick response. I haven't called itr_init() because I saw it need a struct itr_chip parameter. And I am not sure where should I get this parameter from, or maybe I can just declare a itr_chip as the parameter? Also I'm not sure where is the proper place to put itr_init(). Thanks for the suggestion. I'll try adding itr_init() and itr_chip first.

jenswi-linaro commented 3 months ago

Look in one of the other platforms calling itr_init() and it will become clearer.

chiunyi23 commented 3 months ago

I referred to zynq7000 platform(link) and added itr_init(&gic_data.chip) in zynqmp platform(link) as follows.

file: optee_os/core/arch/arm/plat-zynqmp/main.c

void main_init_gic(void)
{
    vaddr_t gicc_base, gicd_base;

    gicc_base = (vaddr_t)phys_to_virt(GIC_BASE + GICC_OFFSET,
                      MEM_AREA_IO_SEC, 1);
    gicd_base = (vaddr_t)phys_to_virt(GIC_BASE + GICD_OFFSET,
                      MEM_AREA_IO_SEC, 1);
    /* On ARMv8, GIC configuration is initialized in ARM-TF */
    gic_init_base_addr(&gic_data, gicc_base, gicd_base);

    itr_init(&gic_data.chip);
}

And it seems to solve some problems, but it stopped at Copied FSBL image to DDR, and no E/TC logs were shown this time. Before this modification, the program would stop at INFO: BL31: Initializing BL32 and shown the E/TC logs. There is the booting log.

XPFW: Calling ROM PWRUP Handler..Done
XPFW: Calling ROM Isolation Handler..Done
XPFW: Calling ROM PWRUP Handler..Done
NOTICE:  ATF running on XCZU9EG/silicon v4/RTL5.1 at 0x1000
NOTICE:  BL31: v2.2(release):xilinx-v2020.2
NOTICE:  BL31: Built : 16:32:51, Jun 25 2024
INFO:    ARM GICv2 driver initialized
INFO:    BL31: Initializing runtime services
INFO:    BL31: PM Service Init Complete: API v1.1
INFO:    BL31: Initializing BL32
INFO:    BL31: Preparing for EL3 exit to normal world
INFO:    Entry point address = 0x8000000
INFO:    SPSR = 0x3c9
Copied FSBL image to DDR
jenswi-linaro commented 3 months ago

The line

INFO:    SPSR = 0x3c9

Looks like a TF-A log entry, but

Copied FSBL image to DDR

looks like something else. It's probably from the normal world boot loader. I can't help you any further.

github-actions[bot] commented 2 months ago

This issue has been marked as a stale issue because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this issue will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.