TF-Hafnium / hafnium

Read only mirror for Hafnium
https://git.trustedfirmware.org/hafnium/hafnium.git/about/
BSD 3-Clause "New" or "Revised" License
5 stars 4 forks source link

Please help clarify the vcpu state and interrupt-related issue. #5

Open mingxien opened 1 month ago

mingxien commented 1 month ago

There are two questions need your clarification. (Hafnium as SPMC)

  1. The vcpu->state of the second or subsequent boot SP will be set to VCPU_STATE_RUNNING(refer to https://github.com/TF-Hafnium/hafnium/blob/main/src/arch/aarch64/plat/ffa/spmc.c#L2112), but the vcpu->state of the first boot SP is set to VCPU_STATE_WAITING (refer to https://github.com/TF-Hafnium/hafnium/blob/main/src/arch/aarch64/plat/psci/spmc.c#L102 and https://github.com/TF-Hafnium/hafnium/blob/main/src/vcpu.c#L79). Is there any specific reason for this?"

  2. Hafnium will pre-enable the corresponding physical interrupts based on interrupt IDs defined in the SP DTS before SP boots (refer to https://github.com/TF-Hafnium/hafnium/blob/main/src/load.c#L154). Our experiments have shown that if a hardware interrupt is signaled when the SP is not yet ready to handle interrupt, the case leads to an ffa_error because the interrupt isn't correctly handled.(generally, the inetrrupts should be deactivated using HF_INTERRUPT_DEACTIVATE in the interrupt handler routine) (refer to https://github.com/TF-Hafnium/hafnium/blob/main/src/arch/aarch64/plat/ffa/spmc.c#L2409) Does this mean that the SP must always be ready to handle interrupts? However, there is a gap before the SP sets up the interrupt handling routine.

J-Alves commented 1 month ago

About issue number 1, this is a real bug that requires a fix from us. Thank you for bringing this to our attention.

Abou issue number 2, the error return you mentioned above was implemented to prevent an SP from going to a waiting state without handling interrupts. The current implementation does assume SP needs to be able to handle the interrupt in initialisation. Let me investigate further, to see if we can do something about this.

madhukar-Arm commented 1 month ago

Can you provide a little more information about your software stack and system configuration? Is the SP an S-EL0 or S-EL1 partition? What is the hardware source of the physical interrupt? What state is the SP's execution context in when the interrupt triggered? Whether or not physical interrupts are allowed to trigger whilst an SP's execution context is initialising itself is dependent on the platform design choice and not restricted by the FF-A specification. From a software perspective, Hafnium SPMC is the physical owner of GIC and hence owns/manages the physical interrupts. An SP is only exposed to a virtual interface and works with virtual interrupts. The life cycle of a physical interrupt is closely tied to the virtual interrupt injected to the SP's vcpu. Therefore, the check being done in SPMC in line 2409 is essential to maintain the system integrity.

mingxien commented 1 month ago

@J-Alves, thanks for your clarification.

@madhukar-Arm, the following explain the software stack and system configuration. The SP is an S-EL1 and Hafnium v2.10 is an S-EL2. The physical interrupt is triggered by firmware (running on a microcontroller) after writing to the hardware register, so the SP cannot explicitly know when the interrupt will be triggered. In our experiment, we set up the virtual interrupt routine during the SP initialising stage (state is RTM_FFA_RUN, rt_model is RTM_SP_INIT), but there is a chance that an interrupt is received before the setup is complete, causing the interrupt to not be handled correctly.

The SPMC compilation configurations are same as https://github.com/TF-Hafnium/hafnium-project-reference/blob/main/BUILD.gn#L272

The device node of SP DTS is shown as below.

device-regions {
        compatible = "arm,ffa-manifest-device-regions";
        hardware-reg {
            description = "my hardware";
            base-address = <0x00000000 0x12340010>;
            pages-count = <1>;
            attributes = <0x3>;
            interrupts = <666 0x900>;
        };
};
madhukar-Arm commented 1 month ago

Thanks for the information @mingxien Are you seeing this issue on a real software stack running on an Arm SoC or is it just an experiment with a software model (like FVP fast model)? I am asking because the properties of device-region node look fictitious. Does the interrupt triggered by the microcontroller need to be service immediately or can it be serviced once all Secure Partitions have completely initialized themselves properly and SPMC hands over the control to normal world software?

mingxien commented 1 month ago

This problem occurs on a real ARMv9 SoC. Due to proprietary reasons, I made modifiactions to the interrupt id, address, and naming, but attributes are genuine. The firmware running on the microcontroller triggers an interrupt and has a timeout mechanism. If the SP does not handle it within the specified time, the system will crash. By the time the normal world software takes over, some time may have already passed (and our understanding the interrupt will be triggered when NS-EL2 enables the interrupt or hands over control to EL1).

We have currently added a conditional check in sp_boot_next to allow the SP to continue execution after receiving and handling an interrupt during the booting stage. This workaround seems to be working well, but we are not sure if there will be any side effects.

static bool sp_boot_next(struct vcpu_locked current_locked, struct vcpu **next)
{
    static bool spmc_booted = false;
    struct vcpu *vcpu_next = NULL;
    struct vcpu *current = current_locked.vcpu;

+   if (spmc_booted || current->rt_model == RTM_SEC_INTERRUPT) {
-   if (spmc_booted) {
        return false;
    }
...
odeprez commented 1 month ago

Hi, on issue 1, the way I understand it is that the first execution context (EC0 or vCPU0) for each SPx is run sequentially, each in turn initialising itself (in which case the vCPU state is running) until SP vCPUx calls FFA_MSG_WAIT (vcpu state becomes waiting) e.g.

Initially, SP1 vCPU0 waiting SP2 vCPU0 waiting SP3 vCPU0 waiting

Boot process starts: SP1 vCPU0 running SP2 vCPU0 waiting SP3 vCPU0 waiting

SP1 vCPU0 waiting (called FFA_MSG_WAIT) SP2 vCPU0 running SP3 vCPU0 waiting

SP1 vCPU0 waiting SP2 vCPU0 waiting (called FFA_MSG_WAIT) SP3 vCPU0 running

SP1 vCPU0 waiting SP2 vCPU0 waiting SP3 vCPU0 waiting (called FFA_MSG_WAIT)

At this stage, all SPs are initialized, and boot process can continue to normal world. Are you observing something different?

On issue 2, afaiu Hafnium enables the physical interrupt in the GICD but the virtual interrupt is not automatically enabled in the SP (at the virtual interrupt controller level). It won't (or shouldn't!) be signaled to the SP until it calls HF_INTERRUPT_ENABLE and unmasks PSTATE.I. From the description of the external controller, it looks to behave like a watchdog that should be served very early even before normal world has booted, is this correct? Is it a platform requirement/expectation that such early interrupts are handled by the SP while it boots (it looks like yes based on your description)? Is there any chance that the platform starts the watchdog sequence only after all SPs have initialised (and possibly from when normal world is booted)?

odeprez commented 1 month ago

Actually I understand the issue might be:

Boot process starts: SP1 vCPU0 waiting ... runs on the PE while vCPU is in waiting state SP2 vCPU0 waiting SP3 vCPU0 waiting ...

Is this correct?

mingxien commented 1 month ago

Hi @odeprez,

About the issue 1, what you mentioned in the latest comment is what we observed. the SP1 vCPU0 is in a waiting state instead of a running state when boot process starts.

We did not directly print out the state of all SPx vCPUs, but based on the trace code (refer to https://github.com/TF-Hafnium/hafnium/blob/main/src/arch/aarch64/plat/psci/spmc.c#L102) and the fact that when we trigger an interrupt during the first SP boot process, the Hafnium interrupt handler enters the VCPU_STATE_WAITING case, it indicates that the first boot SP should be in a waiting state (refer to https://github.com/TF-Hafnium/hafnium/blob/main/src/arch/aarch64/plat/ffa/spmc.c#L1478). Additionally, we tried using a second SP to register and receive interrupts, which then enters the running state case.

About the issue 2, yes, when an interrupt is triggered while the SP is initializing, Hafnium resumes the SP to continue initialization because the SP is in a running state. However, after initialization is complete and ffa_msg_wait is called, some errors occur (for example, if there is only one SP, an ffa_error will occur because the interrupt has not been deactivated, refer to https://github.com/TF-Hafnium/hafnium/blob/main/src/arch/aarch64/plat/ffa/spmc.c#L2410).

It is similar to a watchdog function. We register and enable the interrupt during SP initialization, and certain settings in the SP interrupt handler must be completed immediately during the SP boot process. We cannot wait until all SPs are initialized or until the normal world sofrware takes over.

odeprez commented 1 month ago

Hi,

About the issue 1, what you mentioned in the latest comment is what we observed. the SP1 vCPU0 is in a waiting state instead of a running state when boot process starts.

Ok. We reproduced/acked the issue and attempted to prepare a fix but it's not fully ready yet https://review.trustedfirmware.org/c/hafnium/hafnium/+/30042

We did not directly print out the state of all SPx vCPUs, but based on the trace code (refer to https://github.com/TF-Hafnium/hafnium/blob/main/src/arch/aarch64/plat/psci/spmc.c#L102) and the fact that when we trigger an interrupt during the first SP boot process, the Hafnium interrupt handler enters the VCPU_STATE_WAITING case, it indicates that the first boot SP should be in a waiting state (refer to https://github.com/TF-Hafnium/hafnium/blob/main/src/arch/aarch64/plat/ffa/spmc.c#L1478). Additionally, we tried using a second SP to register and receive interrupts, which then enters the running state case.

ok this analysis makes sense

About the issue 2, yes, when an interrupt is triggered while the SP is initializing, Hafnium resumes the SP to continue initialization because the SP is in a running state. However, after initialization is complete and ffa_msg_wait is called, some errors occur (for example, if there is only one SP, an ffa_error will occur because the interrupt has not been deactivated, refer to https://github.com/TF-Hafnium/hafnium/blob/main/src/arch/aarch64/plat/ffa/spmc.c#L2410).

Ok to be clear, we don't validate this case of a physical interrupt occurring while the SP boots. We anticipate probable issues that you're currently experiencing. This use case has never really been considered or supported so far. On a high level it seems the immediate problem is that FFA_MSG_WAIT has two conflated purposes: signalling end of SP initialisation and signalling interrupt handling completion.

It is similar to a watchdog function. We register and enable the interrupt during SP initialisation, and certain settings in the SP interrupt handler must be completed immediately during the SP boot process. We cannot wait until all SPs are initialised or until the normal world software takes over.

Ok. Sounds a new 'requirement' to hafnium, just to say that this might not be properly supported out of the box and requires a bit of design/revisit.

mingxien commented 1 month ago

Hi, Thank you for helping to fix the issue 1.

In summary, issue 2 is a new requirement for Hafnium that is not yet supported and will require a reassessment and redesign.

We plan to adopt a workaround in sp_boot_next() where if the SP calls FFA_MSG_WAIT due to completing an interrupt handling, it will directly resume the preempted initializing SP without searching for the next booting SP. This is to avoid SP1 registering and enabling interrupts during its initialization phase, and then SP2 entering its initialization process, but receiving an interrupt and being preempted by SP1 to handle the interrupt. Could you give us some suggestions and review this workaround to see if it is suitable as a temporary solution? (internal use only, no PR)

static bool sp_boot_next(struct vcpu_locked current_locked, struct vcpu **next)
{
    static bool spmc_booted = false;
    struct vcpu *vcpu_next = NULL;
    struct vcpu *current = current_locked.vcpu;

+   if (spmc_booted || current->rt_model == RTM_SEC_INTERRUPT) {
-   if (spmc_booted) {
        return false;
    }
...
J-Alves commented 1 month ago

Hi, Could you try the patch below: https://review.trustedfirmware.org/c/hafnium/hafnium/+/30042 Let me know if it works for you. It should fix issue 1 reported here. Issue 2 is still working in progress. Cheers

J-Alves commented 4 weeks ago

Hi,

The patch provided in the previous comment just got merged.

Best regards, João Alves

mingxien commented 4 weeks ago

Hi @J-Alves, Sorry, we had a holiday for few days. I tested the patch today and everything works well. Thanks.

J-Alves commented 2 weeks ago

Hi, @mingxien about issue 1... https://review.trustedfirmware.org/q/topic:%22ja/interrupt_rtm_init%22

Above there is a patch to avoid enabling the interrupt at the system loading phase. Instead, it is enabled when the SP invokes the HF_INTERRUPT_ENABLE. There is also a test, checking this helps with handling the interrupt smoothly whilst vCPU has its runtime model to RTM_SP_INIT.

Could you fetch the above changes and let us know if they help? Cheers

mingxien commented 2 weeks ago

Hi @J-Alves, It works. The interrupt is received until SP invokes the HF_INTERRUPT_ENABLE. The patch can resolve physical interrupts triggered while the SP is in the initialisation phase to avoid FFA_ERROR. Thanks.