Before replying to your email, I want to highlight a concern: I did not receive the original email from Pawandeep (it seems there may be an issue with the mailing list).

Regarding the technical point when the system does not include an S-EL2 component, there is no need to save or restore the icc_sre_el2_val register.

There is a patch currently under review [1] that moves the GIC save/restore logic to be invoked from the SPM dispatcher (when S-EL2 is present). As a result, configurations running at S-EL1 will not trigger this path.

[1] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/45798/1

Hi Oza,

On 02/12/2025 23:07, Pawandeep Oza (QUIC) via TF-A wrote:
>
> Please have a look at following patch, where on our platform we try to maintain single image of TFA (for custom CPU and Cortex A55)
> Cortex A55 does not have Secure EL2 implemented, while on the other hand our custom CPU has secure EL2 (and we run Hafnium there)
>
> On Cortex A55 ARM AEM model:
> write_icc_sre_el2(read_el2_ctx_common(ctx, icc_sre_el2));
>
> the context was set before as
> u_register_t icc_sre_el2_val = ICC_SRE_DIB_BIT | ICC_SRE_DFB_BIT |
>                    ICC_SRE_EN_BIT | ICC_SRE_SRE_BIT;
>
> and setting ICC_SRE_DIB_BIT | ICC_SRE_DFB_BIT causes crash since secure EL2 is not implemented.

Just to clarify, is it the bits that are being set or the entire register? What's the exact crash you're seeing?

> resulting into following patch which resolves the issue.  seeking feedback/discussion if I can post it to upstream TFA,

You can always post to upstream for review and is usually the best place to consider changes.

> let me know if I am missing something here.
>
>
>     lib/el3_runtime: set NS bit if secure el2 is not implemented
>    
>     before setting icc_sre_el2 set NS bit for non-secure context so that
>     the ICC_SRE_DIB_BIT and ICC_SRE_DFB_BIT are preserved
>    
>     Signed-off-by: Oza Pawandeep <quic_poza@quicinc.com>
>
> diff --git a/lib/el3_runtime/aarch64/context_mgmt.c b/lib/el3_runtime/aarch64/context_mgmt.c
> index e31255868..5100f2f00 100644
> --- a/lib/el3_runtime/aarch64/context_mgmt.c
> +++ b/lib/el3_runtime/aarch64/context_mgmt.c
> @@ -1411,7 +1411,18 @@ static void el2_sysregs_context_restore_gic(el2_sysregs_t *ctx, uint32_t securit
>         u_register_t scr_el3 = read_scr_el3();
> 
>  #if defined(SPD_spmd) && SPMD_SPM_AT_SEL2
> -       write_icc_sre_el2(read_el2_ctx_common(ctx, icc_sre_el2));
> +       if (is_feat_sel2_supported()) {
> +               write_icc_sre_el2(read_el2_ctx_common(ctx, icc_sre_el2));
> +       } else {
> +               write_scr_el3(scr_el3 | SCR_NS_BIT);
> +               isb();
> +

I suspect a more correct approach is to ensure that the NS bit has been set for EL3 execution. The problem with this is that the SPMD_SPM_AT_SEL2 flag means that there will be an SPM and it will be SEL2. Would a simple `if (is_feat_sel2_supported()) write_icc_sre_el2() else nop` work? The gicv3 driver should have already set gic registers and since they won't change that would be safe.

> +               write_icc_sre_el2(read_el2_ctx_common(ctx, icc_sre_el2));
> +
> +               write_scr_el3(scr_el3);
> +               isb();
> +       }
> +
>  #else
>         write_scr_el3(scr_el3 | SCR_NS_BIT);
>         isb();
>
>
> Regards,
> Oza.

Thanks,
Boyan

--
TF-A mailing list -- tf-a@lists.trustedfirmware.org
To unsubscribe send an email to tf-a-leave@lists.trustedfirmware.org