Buggy branch in __init_el2_gicv3?

Buggy branch in __init_el2_gicv3?

[Joakim Tjernlund]

In init_el2_state calls __init_el2_gicv3 which looks:

/* GICv3 system register access */
.macro __init_el2_gicv3
	mrs	x0, id_aa64pfr0_el1
	ubfx	x0, x0, #ID_AA64PFR0_GIC_SHIFT, #4
	cbz	x0, .Lskip_gicv3_\@

	mrs_s	x0, SYS_ICC_SRE_EL2
	orr	x0, x0, #ICC_SRE_EL2_SRE	// Set ICC_SRE_EL2.SRE==1
	orr	x0, x0, #ICC_SRE_EL2_ENABLE	// Set ICC_SRE_EL2.Enable==1
	msr_s	SYS_ICC_SRE_EL2, x0
	isb					// Make sure SRE is now set
	mrs_s	x0, SYS_ICC_SRE_EL2		// Read SRE back,
	tbz     x0, #0, 1f
//	tbz	x0, #0, .Lskip_gicv3_\@		// and check that it sticks
	msr_s	SYS_ICH_HCR_EL2, xzr		// Reset ICC_HCR_EL2 to defaults
.Lskip_gicv3_\@:
.endm

Note the tbz     x0, #0, 1f, this instruction causes my A53 CPU to jump far out of the macro and make the
CPU go into EL0, failing with an exception a bit later. I believe the 1f is a bug and if I replace it with
 tbz	x0, #0, .Lskip_gicv3_\@
the CPU continue too boot.
Why this is trigged is still a mystery to me, any pointers welcome :)

 Jocke
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Buggy branch in __init_el2_gicv3?

[Mark Rutland]

On Mon, Feb 14, 2022 at 12:34:12PM +0000, Joakim Tjernlund wrote:
> In init_el2_state calls __init_el2_gicv3 which looks:
> 
> /* GICv3 system register access */
> .macro __init_el2_gicv3
> 	mrs	x0, id_aa64pfr0_el1
> 	ubfx	x0, x0, #ID_AA64PFR0_GIC_SHIFT, #4
> 	cbz	x0, .Lskip_gicv3_\@
> 
> 	mrs_s	x0, SYS_ICC_SRE_EL2
> 	orr	x0, x0, #ICC_SRE_EL2_SRE	// Set ICC_SRE_EL2.SRE==1
> 	orr	x0, x0, #ICC_SRE_EL2_ENABLE	// Set ICC_SRE_EL2.Enable==1
> 	msr_s	SYS_ICC_SRE_EL2, x0
> 	isb					// Make sure SRE is now set
> 	mrs_s	x0, SYS_ICC_SRE_EL2		// Read SRE back,
> 	tbz     x0, #0, 1f
> //	tbz	x0, #0, .Lskip_gicv3_\@		// and check that it sticks
> 	msr_s	SYS_ICH_HCR_EL2, xzr		// Reset ICC_HCR_EL2 to defaults
> .Lskip_gicv3_\@:
> .endm
> 
> Note the tbz     x0, #0, 1f, this instruction causes my A53 CPU to jump far out of the macro and make the
> CPU go into EL0, failing with an exception a bit later. I believe the 1f is a bug and if I replace it with
>  tbz	x0, #0, .Lskip_gicv3_\@
> the CPU continue too boot.

You're right, that should be targetting `.Lskip_gicv3_\@`. It looks like that
was left behind in commit:

  114945d84a30a5fe ("arm64: Fix labels in el2_setup macros")

Marc?

> Why this is trigged is still a mystery to me, any pointers welcome :)

I believe this happens if EL3 has ICC_SRE_EL3.SRE == 0b0; in that case
ICC_SRE_EL2.SRE is RAZ/WI. i.e. your EL3 firmware isn't using GICv3 and hasn't
enabled it for lower ELs.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Buggy branch in __init_el2_gicv3?

[Joakim Tjernlund]

On Mon, 2022-02-14 at 14:56 +0000, Mark Rutland wrote:
> On Mon, Feb 14, 2022 at 12:34:12PM +0000, Joakim Tjernlund wrote:
> > In init_el2_state calls __init_el2_gicv3 which looks:
> > 
> > /* GICv3 system register access */
> > .macro __init_el2_gicv3
> > 	mrs	x0, id_aa64pfr0_el1
> > 	ubfx	x0, x0, #ID_AA64PFR0_GIC_SHIFT, #4
> > 	cbz	x0, .Lskip_gicv3_\@
> > 
> > 	mrs_s	x0, SYS_ICC_SRE_EL2
> > 	orr	x0, x0, #ICC_SRE_EL2_SRE	// Set ICC_SRE_EL2.SRE==1
> > 	orr	x0, x0, #ICC_SRE_EL2_ENABLE	// Set ICC_SRE_EL2.Enable==1
> > 	msr_s	SYS_ICC_SRE_EL2, x0
> > 	isb					// Make sure SRE is now set
> > 	mrs_s	x0, SYS_ICC_SRE_EL2		// Read SRE back,
> > 	tbz     x0, #0, 1f
> > //	tbz	x0, #0, .Lskip_gicv3_\@		// and check that it sticks
> > 	msr_s	SYS_ICH_HCR_EL2, xzr		// Reset ICC_HCR_EL2 to defaults
> > .Lskip_gicv3_\@:
> > .endm
> > 
> > Note the tbz     x0, #0, 1f, this instruction causes my A53 CPU to jump far out of the macro and make the
> > CPU go into EL0, failing with an exception a bit later. I believe the 1f is a bug and if I replace it with
> >  tbz	x0, #0, .Lskip_gicv3_\@
> > the CPU continue too boot.
> 
> You're right, that should be targetting `.Lskip_gicv3_\@`. It looks like that
> was left behind in commit:
> 
>   114945d84a30a5fe ("arm64: Fix labels in el2_setup macros")
> 
> Marc?
> 
> > Why this is trigged is still a mystery to me, any pointers welcome :)
> 
> I believe this happens if EL3 has ICC_SRE_EL3.SRE == 0b0; in that case
> ICC_SRE_EL2.SRE is RAZ/WI. i.e. your EL3 firmware isn't using GICv3 and hasn't
> enabled it for lower ELs.

Correct, I am now trying to enable GIC but I thing I note is that we have a GIC500v4
and u-boot(my EL3 FW) only has GICV3 as far as I can see. Will that be a problem or is GICv3 and v4 compatible
as in this respect ?

 Jocke
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Buggy branch in __init_el2_gicv3?

[Marc Zyngier]

On 2022-02-14 14:56, Mark Rutland wrote:
> On Mon, Feb 14, 2022 at 12:34:12PM +0000, Joakim Tjernlund wrote:
>> In init_el2_state calls __init_el2_gicv3 which looks:
>> 
>> /* GICv3 system register access */
>> .macro __init_el2_gicv3
>> 	mrs	x0, id_aa64pfr0_el1
>> 	ubfx	x0, x0, #ID_AA64PFR0_GIC_SHIFT, #4
>> 	cbz	x0, .Lskip_gicv3_\@
>> 
>> 	mrs_s	x0, SYS_ICC_SRE_EL2
>> 	orr	x0, x0, #ICC_SRE_EL2_SRE	// Set ICC_SRE_EL2.SRE==1
>> 	orr	x0, x0, #ICC_SRE_EL2_ENABLE	// Set ICC_SRE_EL2.Enable==1
>> 	msr_s	SYS_ICC_SRE_EL2, x0
>> 	isb					// Make sure SRE is now set
>> 	mrs_s	x0, SYS_ICC_SRE_EL2		// Read SRE back,
>> 	tbz     x0, #0, 1f
>> //	tbz	x0, #0, .Lskip_gicv3_\@		// and check that it sticks
>> 	msr_s	SYS_ICH_HCR_EL2, xzr		// Reset ICC_HCR_EL2 to defaults
>> .Lskip_gicv3_\@:
>> .endm
>> 
>> Note the tbz     x0, #0, 1f, this instruction causes my A53 CPU to 
>> jump far out of the macro and make the
>> CPU go into EL0, failing with an exception a bit later. I believe the 
>> 1f is a bug and if I replace it with
>>  tbz	x0, #0, .Lskip_gicv3_\@
>> the CPU continue too boot.
> 
> You're right, that should be targetting `.Lskip_gicv3_\@`. It looks 
> like that
> was left behind in commit:
> 
>   114945d84a30a5fe ("arm64: Fix labels in el2_setup macros")
> 
> Marc?

Yup, that's a glaring bug. My bad. Thanks for pointing that out.

Joakim, can you please write a patch and cc me?

Cheers,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Buggy branch in __init_el2_gicv3?

[Joakim Tjernlund]

Just sent a patch, hope I got the commit msg right.

________________________________________
From: Marc Zyngier <maz@kernel.org>
Sent: 14 February 2022 17:26
To: Mark Rutland
Cc: Joakim Tjernlund; linux-arm-kernel@lists.infradead.org
Subject: Re: Buggy branch in __init_el2_gicv3?

On 2022-02-14 14:56, Mark Rutland wrote:
> On Mon, Feb 14, 2022 at 12:34:12PM +0000, Joakim Tjernlund wrote:
>> In init_el2_state calls __init_el2_gicv3 which looks:
>>
>> /* GICv3 system register access */
>> .macro __init_el2_gicv3
>>      mrs     x0, id_aa64pfr0_el1
>>      ubfx    x0, x0, #ID_AA64PFR0_GIC_SHIFT, #4
>>      cbz     x0, .Lskip_gicv3_\@
>>
>>      mrs_s   x0, SYS_ICC_SRE_EL2
>>      orr     x0, x0, #ICC_SRE_EL2_SRE        // Set ICC_SRE_EL2.SRE==1
>>      orr     x0, x0, #ICC_SRE_EL2_ENABLE     // Set ICC_SRE_EL2.Enable==1
>>      msr_s   SYS_ICC_SRE_EL2, x0
>>      isb                                     // Make sure SRE is now set
>>      mrs_s   x0, SYS_ICC_SRE_EL2             // Read SRE back,
>>      tbz     x0, #0, 1f
>> //   tbz     x0, #0, .Lskip_gicv3_\@         // and check that it sticks
>>      msr_s   SYS_ICH_HCR_EL2, xzr            // Reset ICC_HCR_EL2 to defaults
>> .Lskip_gicv3_\@:
>> .endm
>>
>> Note the tbz     x0, #0, 1f, this instruction causes my A53 CPU to
>> jump far out of the macro and make the
>> CPU go into EL0, failing with an exception a bit later. I believe the
>> 1f is a bug and if I replace it with
>>  tbz x0, #0, .Lskip_gicv3_\@
>> the CPU continue too boot.
>
> You're right, that should be targetting `.Lskip_gicv3_\@`. It looks
> like that
> was left behind in commit:
>
>   114945d84a30a5fe ("arm64: Fix labels in el2_setup macros")
>
> Marc?

Yup, that's a glaring bug. My bad. Thanks for pointing that out.

Joakim, can you please write a patch and cc me?

Cheers,

         M.
--
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Buggy branch in __init_el2_gicv3?

[Marc Zyngier]

On Mon, 14 Feb 2022 15:14:41 +0000,
Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:
> 
> On Mon, 2022-02-14 at 14:56 +0000, Mark Rutland wrote:
> > On Mon, Feb 14, 2022 at 12:34:12PM +0000, Joakim Tjernlund wrote:
> > > In init_el2_state calls __init_el2_gicv3 which looks:
> > > 
> > > /* GICv3 system register access */
> > > .macro __init_el2_gicv3
> > > 	mrs	x0, id_aa64pfr0_el1
> > > 	ubfx	x0, x0, #ID_AA64PFR0_GIC_SHIFT, #4
> > > 	cbz	x0, .Lskip_gicv3_\@
> > > 
> > > 	mrs_s	x0, SYS_ICC_SRE_EL2
> > > 	orr	x0, x0, #ICC_SRE_EL2_SRE	// Set ICC_SRE_EL2.SRE==1
> > > 	orr	x0, x0, #ICC_SRE_EL2_ENABLE	// Set ICC_SRE_EL2.Enable==1
> > > 	msr_s	SYS_ICC_SRE_EL2, x0
> > > 	isb					// Make sure SRE is now set
> > > 	mrs_s	x0, SYS_ICC_SRE_EL2		// Read SRE back,
> > > 	tbz     x0, #0, 1f
> > > //	tbz	x0, #0, .Lskip_gicv3_\@		// and check that it sticks
> > > 	msr_s	SYS_ICH_HCR_EL2, xzr		// Reset ICC_HCR_EL2 to defaults
> > > .Lskip_gicv3_\@:
> > > .endm
> > > 
> > > Note the tbz     x0, #0, 1f, this instruction causes my A53 CPU to jump far out of the macro and make the
> > > CPU go into EL0, failing with an exception a bit later. I believe the 1f is a bug and if I replace it with
> > >  tbz	x0, #0, .Lskip_gicv3_\@
> > > the CPU continue too boot.
> > 
> > You're right, that should be targetting `.Lskip_gicv3_\@`. It looks like that
> > was left behind in commit:
> > 
> >   114945d84a30a5fe ("arm64: Fix labels in el2_setup macros")
> > 
> > Marc?
> > 
> > > Why this is trigged is still a mystery to me, any pointers welcome :)
> > 
> > I believe this happens if EL3 has ICC_SRE_EL3.SRE == 0b0; in that
> > case ICC_SRE_EL2.SRE is RAZ/WI. i.e. your EL3 firmware isn't using
> > GICv3 and hasn't enabled it for lower ELs.
> 
> Correct, I am now trying to enable GIC but I thing I note is that we
> have a GIC500v4 and u-boot(my EL3 FW) only has GICV3 as far as I can
> see. Will that be a problem or is GICv3 and v4 compatible as in this
> respect ?

Hmmm. There is no such thing as a GIC500v4. GIC500 is a GICv3
implementation, and the most recent revision seems to be r1p1.

Note that it doesn't matter much, as GICv3 and GICv4 have programming
models that are largely identical, and the differences are only
meaningful to a hypervisor. Linux has support for both anyway.

Are you actually using u-boot as your EL3 firmware? That's... an
interesting choice. Last time I checked, the GICv3 support was pretty
bad (as in totally broken). Something like TF-A would seem like a
better choice when it comes to architectural support).

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel