From: Anshuman Khandual <anshuman.khandual@arm.com>

Commit 5b39db6037e7 ("arm64: el2_setup.h: Rename some labels to be more
diff-friendly") reworked the labels in __init_el2_fgt to say what's
skipped rather than what the target location is. The exception was
"set_fgt_" which is where registers are written. In reviewing the BRBE
additions, Will suggested "set_debug_fgt_" where HDFGxTR_EL2 are
written. Doing that would partially revert commit 5b39db6037e7 undoing
the goal of minimizing additions here, but it would follow the
convention for labels where registers are written.

So let's do both. Branches that skip something go to a "skip" label and
places that set registers have a "set" label. This results in some
double labels, but it makes things entirely consistent.

While we're here, the SME skip label was incorrectly named, so fix it.

Reported-by: Will Deacon <will@kernel.org>
Cc: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
This one can be applied even if the rest of the series is not.

v22:
 - New patch
---
 arch/arm64/include/asm/el2_setup.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
index ebceaae3c749..30f57b0334a3 100644
--- a/arch/arm64/include/asm/el2_setup.h
+++ b/arch/arm64/include/asm/el2_setup.h
@@ -204,19 +204,21 @@
 	orr	x0, x0, #(1 << 62)
 
 .Lskip_spe_fgt_\@:
+
+.Lset_debug_fgt_\@:
 	msr_s	SYS_HDFGRTR_EL2, x0
 	msr_s	SYS_HDFGWTR_EL2, x0
 
 	mov	x0, xzr
 	mrs	x1, id_aa64pfr1_el1
 	ubfx	x1, x1, #ID_AA64PFR1_EL1_SME_SHIFT, #4
-	cbz	x1, .Lskip_debug_fgt_\@
+	cbz	x1, .Lskip_sme_fgt_\@
 
 	/* Disable nVHE traps of TPIDR2 and SMPRI */
 	orr	x0, x0, #HFGxTR_EL2_nSMPRI_EL1_MASK
 	orr	x0, x0, #HFGxTR_EL2_nTPIDR2_EL0_MASK
 
-.Lskip_debug_fgt_\@:
+.Lskip_sme_fgt_\@:
 	mrs_s	x1, SYS_ID_AA64MMFR3_EL1
 	ubfx	x1, x1, #ID_AA64MMFR3_EL1_S1PIE_SHIFT, #4
 	cbz	x1, .Lskip_pie_fgt_\@
@@ -237,12 +239,14 @@
 	/* GCS depends on PIE so we don't check it if PIE is absent */
 	mrs_s	x1, SYS_ID_AA64PFR1_EL1
 	ubfx	x1, x1, #ID_AA64PFR1_EL1_GCS_SHIFT, #4
-	cbz	x1, .Lset_fgt_\@
+	cbz	x1, .Lskip_gce_fgt_\@
 
 	/* Disable traps of access to GCS registers at EL0 and EL1 */
 	orr	x0, x0, #HFGxTR_EL2_nGCS_EL1_MASK
 	orr	x0, x0, #HFGxTR_EL2_nGCS_EL0_MASK
 
+.Lskip_gce_fgt_\@:
+
 .Lset_fgt_\@:
 	msr_s	SYS_HFGRTR_EL2, x0
 	msr_s	SYS_HFGWTR_EL2, x0

-- 
2.47.2

On Tue, May 20, 2025 at 05:27:37PM -0500, Rob Herring (Arm) wrote:
> From: Anshuman Khandual <anshuman.khandual@arm.com>
> 
> Commit 5b39db6037e7 ("arm64: el2_setup.h: Rename some labels to be more
> diff-friendly") reworked the labels in __init_el2_fgt to say what's
> skipped rather than what the target location is. The exception was
> "set_fgt_" which is where registers are written. In reviewing the BRBE
> additions, Will suggested "set_debug_fgt_" where HDFGxTR_EL2 are
> written. Doing that would partially revert commit 5b39db6037e7 undoing
> the goal of minimizing additions here, but it would follow the
> convention for labels where registers are written.
> 
> So let's do both. Branches that skip something go to a "skip" label and
> places that set registers have a "set" label. This results in some
> double labels, but it makes things entirely consistent.
> 
> While we're here, the SME skip label was incorrectly named, so fix it.
> 
> Reported-by: Will Deacon <will@kernel.org>
> Cc: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> This one can be applied even if the rest of the series is not.
> 
> v22:
>  - New patch
> ---
>  arch/arm64/include/asm/el2_setup.h | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> index ebceaae3c749..30f57b0334a3 100644
> --- a/arch/arm64/include/asm/el2_setup.h
> +++ b/arch/arm64/include/asm/el2_setup.h
> @@ -204,19 +204,21 @@
>  	orr	x0, x0, #(1 << 62)
>  
>  .Lskip_spe_fgt_\@:
> +
> +.Lset_debug_fgt_\@:

Dangling label?  There doesn't seem to be any branch to it in this
series, unless I've missed something.

[...]

Cheers
---Dave

On Thu, May 22, 2025 at 11:15 AM Dave Martin <Dave.Martin@arm.com> wrote:
>
> On Tue, May 20, 2025 at 05:27:37PM -0500, Rob Herring (Arm) wrote:
> > From: Anshuman Khandual <anshuman.khandual@arm.com>
> >
> > Commit 5b39db6037e7 ("arm64: el2_setup.h: Rename some labels to be more
> > diff-friendly") reworked the labels in __init_el2_fgt to say what's
> > skipped rather than what the target location is. The exception was
> > "set_fgt_" which is where registers are written. In reviewing the BRBE
> > additions, Will suggested "set_debug_fgt_" where HDFGxTR_EL2 are
> > written. Doing that would partially revert commit 5b39db6037e7 undoing
> > the goal of minimizing additions here, but it would follow the
> > convention for labels where registers are written.
> >
> > So let's do both. Branches that skip something go to a "skip" label and
> > places that set registers have a "set" label. This results in some
> > double labels, but it makes things entirely consistent.
> >
> > While we're here, the SME skip label was incorrectly named, so fix it.
> >
> > Reported-by: Will Deacon <will@kernel.org>
> > Cc: Dave Martin <Dave.Martin@arm.com>
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> > This one can be applied even if the rest of the series is not.
> >
> > v22:
> >  - New patch
> > ---
> >  arch/arm64/include/asm/el2_setup.h | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> > index ebceaae3c749..30f57b0334a3 100644
> > --- a/arch/arm64/include/asm/el2_setup.h
> > +++ b/arch/arm64/include/asm/el2_setup.h
> > @@ -204,19 +204,21 @@
> >       orr     x0, x0, #(1 << 62)
> >
> >  .Lskip_spe_fgt_\@:
> > +
> > +.Lset_debug_fgt_\@:
>
> Dangling label?  There doesn't seem to be any branch to it in this
> series, unless I've missed something.

I tried to explain that in the commit message. To have both what you
wanted and what Will suggested, you end up with 2 labels in between
the last skip and setting registers.

Rob

Hi,

On Thu, May 22, 2025 at 12:20:35PM -0500, Rob Herring wrote:
> On Thu, May 22, 2025 at 11:15 AM Dave Martin <Dave.Martin@arm.com> wrote:
> >
> > On Tue, May 20, 2025 at 05:27:37PM -0500, Rob Herring (Arm) wrote:
> > > From: Anshuman Khandual <anshuman.khandual@arm.com>
> > >
> > > Commit 5b39db6037e7 ("arm64: el2_setup.h: Rename some labels to be more
> > > diff-friendly") reworked the labels in __init_el2_fgt to say what's
> > > skipped rather than what the target location is. The exception was
> > > "set_fgt_" which is where registers are written. In reviewing the BRBE
> > > additions, Will suggested "set_debug_fgt_" where HDFGxTR_EL2 are
> > > written. Doing that would partially revert commit 5b39db6037e7 undoing
> > > the goal of minimizing additions here, but it would follow the
> > > convention for labels where registers are written.
> > >
> > > So let's do both. Branches that skip something go to a "skip" label and
> > > places that set registers have a "set" label. This results in some
> > > double labels, but it makes things entirely consistent.
> > >
> > > While we're here, the SME skip label was incorrectly named, so fix it.
> > >
> > > Reported-by: Will Deacon <will@kernel.org>
> > > Cc: Dave Martin <Dave.Martin@arm.com>
> > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > ---
> > > This one can be applied even if the rest of the series is not.
> > >
> > > v22:
> > >  - New patch
> > > ---
> > >  arch/arm64/include/asm/el2_setup.h | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> > > index ebceaae3c749..30f57b0334a3 100644
> > > --- a/arch/arm64/include/asm/el2_setup.h
> > > +++ b/arch/arm64/include/asm/el2_setup.h
> > > @@ -204,19 +204,21 @@
> > >       orr     x0, x0, #(1 << 62)
> > >
> > >  .Lskip_spe_fgt_\@:
> > > +
> > > +.Lset_debug_fgt_\@:
> >
> > Dangling label?  There doesn't seem to be any branch to it in this
> > series, unless I've missed something.
> 
> I tried to explain that in the commit message. To have both what you
> wanted and what Will suggested, you end up with 2 labels in between
> the last skip and setting registers.

Hmm, I wasn't trying to advocate for adding dead labels in anticipation
of their use, just to avoid labels whose names conflict with an
anticipated future use.

I guess this is harmless, but I may look at this again as and when...

Cheers
---Dave

On Tue, May 20, 2025 at 05:27:37PM -0500, Rob Herring (Arm) wrote:
> From: Anshuman Khandual <anshuman.khandual@arm.com>
> 
> Commit 5b39db6037e7 ("arm64: el2_setup.h: Rename some labels to be more
> diff-friendly") reworked the labels in __init_el2_fgt to say what's
> skipped rather than what the target location is. The exception was
> "set_fgt_" which is where registers are written. In reviewing the BRBE
> additions, Will suggested "set_debug_fgt_" where HDFGxTR_EL2 are
> written. Doing that would partially revert commit 5b39db6037e7 undoing
> the goal of minimizing additions here, but it would follow the
> convention for labels where registers are written.
> 
> So let's do both. Branches that skip something go to a "skip" label and
> places that set registers have a "set" label. This results in some
> double labels, but it makes things entirely consistent.
> 
> While we're here, the SME skip label was incorrectly named, so fix it.
> 
> Reported-by: Will Deacon <will@kernel.org>
> Cc: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> This one can be applied even if the rest of the series is not.

Cheers, Rob. I'll grab this one.

Will