public inbox for linux-arm-kernel@lists.infradead.org
 help / color / mirror / Atom feed
* [boot-wrapper] aarch64: Enable access into FEAT_SPE_FDS register from EL2 and below
@ 2025-06-04 11:46 Anshuman Khandual
  2025-06-04 11:57 ` James Clark
  0 siblings, 1 reply; 5+ messages in thread
From: Anshuman Khandual @ 2025-06-04 11:46 UTC (permalink / raw)
  To: linux-arm-kernel; +Cc: Anshuman Khandual, James Clark, Mark Rutland, Mark Brown

FEAT_SPE_FDS adds system register PMSDSFR_EL1. But accessing that system
register from EL2 and below exception levels, will trap into EL3 unless
MDCR_EL3.EnPMS3 is set.

Enable access to FEAT_SPE_FDS registers when they are implemented.

Cc: James Clark <james.clark@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/aarch64/include/asm/cpu.h | 4 ++++
 arch/aarch64/init.c            | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
index 2b3a659..ac50474 100644
--- a/arch/aarch64/include/asm/cpu.h
+++ b/arch/aarch64/include/asm/cpu.h
@@ -55,6 +55,7 @@
 #define MDCR_EL3_NSTB_NS_NOTRAP			(UL(3) << 24)
 #define MDCR_EL3_SBRBE_NOTRAP_NOPROHIBIT	(UL(3) << 32)
 #define MDCR_EL3_ENPMSN				BIT(36)
+#define MDCR_EL3_ENPMS3				BIT(42)
 #define MDCR_EL3_EBWE				BIT(43)
 #define MDCR_EL3_EnPM2				BIT(7)
 
@@ -185,6 +186,9 @@
 
 #define SCTLR_EL1_CP15BEN	(1 << 5)
 
+#define PMSIDR_EL1		s3_0_c9_c9_7
+#define PMSIDR_EL1_FDS		BIT(7)
+
 #ifdef KERNEL_32
 /*
  * When booting a 32-bit kernel, EL1 uses AArch32 and registers which are
diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
index e1640a9..b6f740c 100644
--- a/arch/aarch64/init.c
+++ b/arch/aarch64/init.c
@@ -146,6 +146,9 @@ static void cpu_init_el3(void)
 	if (mrs_field(ID_AA64DFR0_EL1, PMSVER) >= 3)
 		mdcr |= MDCR_EL3_ENPMSN;
 
+	if (mrs_field(PMSIDR_EL1, FDS))
+		mdcr |= MDCR_EL3_ENPMS3;
+
 	if (mrs_field(ID_AA64DFR0_EL1, TRACEBUFFER))
 		mdcr |= MDCR_EL3_NSTB_NS_NOTRAP;
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [boot-wrapper] aarch64: Enable access into FEAT_SPE_FDS register from EL2 and below
  2025-06-04 11:46 [boot-wrapper] aarch64: Enable access into FEAT_SPE_FDS register from EL2 and below Anshuman Khandual
@ 2025-06-04 11:57 ` James Clark
  2025-06-05  3:15   ` Anshuman Khandual
  0 siblings, 1 reply; 5+ messages in thread
From: James Clark @ 2025-06-04 11:57 UTC (permalink / raw)
  To: Anshuman Khandual, linux-arm-kernel; +Cc: Mark Rutland, Mark Brown



On 04/06/2025 12:46 pm, Anshuman Khandual wrote:
> FEAT_SPE_FDS adds system register PMSDSFR_EL1. But accessing that system
> register from EL2 and below exception levels, will trap into EL3 unless
> MDCR_EL3.EnPMS3 is set.
> 
> Enable access to FEAT_SPE_FDS registers when they are implemented.
> 
> Cc: James Clark <james.clark@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>   arch/aarch64/include/asm/cpu.h | 4 ++++
>   arch/aarch64/init.c            | 3 +++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
> index 2b3a659..ac50474 100644
> --- a/arch/aarch64/include/asm/cpu.h
> +++ b/arch/aarch64/include/asm/cpu.h
> @@ -55,6 +55,7 @@
>   #define MDCR_EL3_NSTB_NS_NOTRAP			(UL(3) << 24)
>   #define MDCR_EL3_SBRBE_NOTRAP_NOPROHIBIT	(UL(3) << 32)
>   #define MDCR_EL3_ENPMSN				BIT(36)
> +#define MDCR_EL3_ENPMS3				BIT(42)
>   #define MDCR_EL3_EBWE				BIT(43)
>   #define MDCR_EL3_EnPM2				BIT(7)
>   
> @@ -185,6 +186,9 @@
>   
>   #define SCTLR_EL1_CP15BEN	(1 << 5)
>   
> +#define PMSIDR_EL1		s3_0_c9_c9_7
> +#define PMSIDR_EL1_FDS		BIT(7)
> +
>   #ifdef KERNEL_32
>   /*
>    * When booting a 32-bit kernel, EL1 uses AArch32 and registers which are
> diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
> index e1640a9..b6f740c 100644
> --- a/arch/aarch64/init.c
> +++ b/arch/aarch64/init.c
> @@ -146,6 +146,9 @@ static void cpu_init_el3(void)
>   	if (mrs_field(ID_AA64DFR0_EL1, PMSVER) >= 3)
>   		mdcr |= MDCR_EL3_ENPMSN;
>   
> +	if (mrs_field(PMSIDR_EL1, FDS))
> +		mdcr |= MDCR_EL3_ENPMS3;
> +

You are only allowed to read PMSIDR if SPE is implemented otherwise it's 
undef so it needs another check.

It would be much easier to unconditionally set all the known MDCR bits. 
If the feature is implemented then setting them to 1 disables the traps, 
which we want, and if it's not implemented it does nothing which we also 
want.

Doing them all conditionally is quite error prone and extra work for 
nothing. In TFA it's already done unconditionally:

*
* MDCR_EL3.EnPMSN (ARM v8.7) and MDCR_EL3.EnPMS3: Do not trap access to
* PMSNEVFR_EL1 or PMSDSFR_EL1 register at NS-EL1 or NS-EL2 to EL3 if 
FEAT_SPEv1p2
* or FEAT_SPE_FDS are implemented. Setting these bits to 1 doesn't have any
* effect on it when the features aren't implemented.
*/
mdcr_el3_val |= MDCR_NSPB(MDCR_NSPB_EL1) | MDCR_EnPMSN_BIT | 
MDCR_EnPMS3_BIT;

>   	if (mrs_field(ID_AA64DFR0_EL1, TRACEBUFFER))
>   		mdcr |= MDCR_EL3_NSTB_NS_NOTRAP;
>   



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [boot-wrapper] aarch64: Enable access into FEAT_SPE_FDS register from EL2 and below
  2025-06-04 11:57 ` James Clark
@ 2025-06-05  3:15   ` Anshuman Khandual
  2025-06-05  9:49     ` James Clark
  0 siblings, 1 reply; 5+ messages in thread
From: Anshuman Khandual @ 2025-06-05  3:15 UTC (permalink / raw)
  To: James Clark, linux-arm-kernel; +Cc: Mark Rutland, Mark Brown



On 6/4/25 17:27, James Clark wrote:
> 
> 
> On 04/06/2025 12:46 pm, Anshuman Khandual wrote:
>> FEAT_SPE_FDS adds system register PMSDSFR_EL1. But accessing that system
>> register from EL2 and below exception levels, will trap into EL3 unless
>> MDCR_EL3.EnPMS3 is set.
>>
>> Enable access to FEAT_SPE_FDS registers when they are implemented.
>>
>> Cc: James Clark <james.clark@linaro.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Mark Brown <broonie@kernel.org>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>   arch/aarch64/include/asm/cpu.h | 4 ++++
>>   arch/aarch64/init.c            | 3 +++
>>   2 files changed, 7 insertions(+)
>>
>> diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
>> index 2b3a659..ac50474 100644
>> --- a/arch/aarch64/include/asm/cpu.h
>> +++ b/arch/aarch64/include/asm/cpu.h
>> @@ -55,6 +55,7 @@
>>   #define MDCR_EL3_NSTB_NS_NOTRAP            (UL(3) << 24)
>>   #define MDCR_EL3_SBRBE_NOTRAP_NOPROHIBIT    (UL(3) << 32)
>>   #define MDCR_EL3_ENPMSN                BIT(36)
>> +#define MDCR_EL3_ENPMS3                BIT(42)
>>   #define MDCR_EL3_EBWE                BIT(43)
>>   #define MDCR_EL3_EnPM2                BIT(7)
>>   @@ -185,6 +186,9 @@
>>     #define SCTLR_EL1_CP15BEN    (1 << 5)
>>   +#define PMSIDR_EL1        s3_0_c9_c9_7
>> +#define PMSIDR_EL1_FDS        BIT(7)
>> +
>>   #ifdef KERNEL_32
>>   /*
>>    * When booting a 32-bit kernel, EL1 uses AArch32 and registers which are
>> diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
>> index e1640a9..b6f740c 100644
>> --- a/arch/aarch64/init.c
>> +++ b/arch/aarch64/init.c
>> @@ -146,6 +146,9 @@ static void cpu_init_el3(void)
>>       if (mrs_field(ID_AA64DFR0_EL1, PMSVER) >= 3)
>>           mdcr |= MDCR_EL3_ENPMSN;
>>   +    if (mrs_field(PMSIDR_EL1, FDS))
>> +        mdcr |= MDCR_EL3_ENPMS3;
>> +
> 
> You are only allowed to read PMSIDR if SPE is implemented otherwise it's undef so it needs another check.

The same is true for the earlier bit MDCR_EL3_ENPMSN as well. FEAT_SPE_FnE should
have been checked from PMSIDR_EL1.FnE but the feature is guaranteed to be present
when FEAT_SPEv1p2 is present. Hence PMSVER >= 3 check alone is sufficient here.

But for for FEAT_SPE_FDS feature, ARM DDI 0487 L.A is not very much clear on its
dependency for FEAT_SPE and its versions. It says the following but not the other
way around.

"If FEAT_SPE_FDS is implemented, then FEAT_SPEv1p4 is implemented"

Even PMSIDR_EL1.FDS field is valid only when FEAT_SPEv1p4 is implemented. Hence
you are right - we need to check both before enabling MDCR_EL3_ENPMS3.

        if ((mrs_field(ID_AA64DFR0_EL1, PMSVER) >= 5) &&
            (mrs_field(PMSIDR_EL1, FDS)))
                        mdcr |= MDCR_EL3_ENPMS3;

> 
> It would be much easier to unconditionally set all the known MDCR bits. If the feature is implemented then setting them to 1 disables the traps, which we want, and if it's not implemented it does nothing which we also want.
> 
> Doing them all conditionally is quite error prone and extra work for nothing. In TFA it's already done unconditionally:

But leaving all MDCR_EL3 fields set when corresponding features are not implemented
goes against "don't set register fields when not required even when ignored in HW"
principle and hence might not be ideal.

But will leave it upto Mark and others. Dropping all these conditionality while
setting MDCR_EL3 fields will be a subsequent patch later as if changes existing
fields.

> 
> *
> * MDCR_EL3.EnPMSN (ARM v8.7) and MDCR_EL3.EnPMS3: Do not trap access to
> * PMSNEVFR_EL1 or PMSDSFR_EL1 register at NS-EL1 or NS-EL2 to EL3 if FEAT_SPEv1p2
> * or FEAT_SPE_FDS are implemented. Setting these bits to 1 doesn't have any
> * effect on it when the features aren't implemented.
> */
> mdcr_el3_val |= MDCR_NSPB(MDCR_NSPB_EL1) | MDCR_EnPMSN_BIT | MDCR_EnPMS3_BIT;
> 
>>       if (mrs_field(ID_AA64DFR0_EL1, TRACEBUFFER))
>>           mdcr |= MDCR_EL3_NSTB_NS_NOTRAP;
>>   
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [boot-wrapper] aarch64: Enable access into FEAT_SPE_FDS register from EL2 and below
  2025-06-05  3:15   ` Anshuman Khandual
@ 2025-06-05  9:49     ` James Clark
  2025-06-06  4:45       ` Anshuman Khandual
  0 siblings, 1 reply; 5+ messages in thread
From: James Clark @ 2025-06-05  9:49 UTC (permalink / raw)
  To: Anshuman Khandual, linux-arm-kernel; +Cc: Mark Rutland, Mark Brown



On 05/06/2025 4:15 am, Anshuman Khandual wrote:
> 
> 
> On 6/4/25 17:27, James Clark wrote:
>>
>>
>> On 04/06/2025 12:46 pm, Anshuman Khandual wrote:
>>> FEAT_SPE_FDS adds system register PMSDSFR_EL1. But accessing that system
>>> register from EL2 and below exception levels, will trap into EL3 unless
>>> MDCR_EL3.EnPMS3 is set.
>>>
>>> Enable access to FEAT_SPE_FDS registers when they are implemented.
>>>
>>> Cc: James Clark <james.clark@linaro.org>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Mark Brown <broonie@kernel.org>
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>>    arch/aarch64/include/asm/cpu.h | 4 ++++
>>>    arch/aarch64/init.c            | 3 +++
>>>    2 files changed, 7 insertions(+)
>>>
>>> diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
>>> index 2b3a659..ac50474 100644
>>> --- a/arch/aarch64/include/asm/cpu.h
>>> +++ b/arch/aarch64/include/asm/cpu.h
>>> @@ -55,6 +55,7 @@
>>>    #define MDCR_EL3_NSTB_NS_NOTRAP            (UL(3) << 24)
>>>    #define MDCR_EL3_SBRBE_NOTRAP_NOPROHIBIT    (UL(3) << 32)
>>>    #define MDCR_EL3_ENPMSN                BIT(36)
>>> +#define MDCR_EL3_ENPMS3                BIT(42)
>>>    #define MDCR_EL3_EBWE                BIT(43)
>>>    #define MDCR_EL3_EnPM2                BIT(7)
>>>    @@ -185,6 +186,9 @@
>>>      #define SCTLR_EL1_CP15BEN    (1 << 5)
>>>    +#define PMSIDR_EL1        s3_0_c9_c9_7
>>> +#define PMSIDR_EL1_FDS        BIT(7)
>>> +
>>>    #ifdef KERNEL_32
>>>    /*
>>>     * When booting a 32-bit kernel, EL1 uses AArch32 and registers which are
>>> diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
>>> index e1640a9..b6f740c 100644
>>> --- a/arch/aarch64/init.c
>>> +++ b/arch/aarch64/init.c
>>> @@ -146,6 +146,9 @@ static void cpu_init_el3(void)
>>>        if (mrs_field(ID_AA64DFR0_EL1, PMSVER) >= 3)
>>>            mdcr |= MDCR_EL3_ENPMSN;
>>>    +    if (mrs_field(PMSIDR_EL1, FDS))
>>> +        mdcr |= MDCR_EL3_ENPMS3;
>>> +
>>
>> You are only allowed to read PMSIDR if SPE is implemented otherwise it's undef so it needs another check.
> 
> The same is true for the earlier bit MDCR_EL3_ENPMSN as well. FEAT_SPE_FnE should
> have been checked from PMSIDR_EL1.FnE but the feature is guaranteed to be present
> when FEAT_SPEv1p2 is present. Hence PMSVER >= 3 check alone is sufficient here.
> 

Exactly, ENPMSN availability can be determined only on the SPE version, 
avoiding the read of PMSIDR. So it's not really the same, as you mention.

> But for for FEAT_SPE_FDS feature, ARM DDI 0487 L.A is not very much clear on its
> dependency for FEAT_SPE and its versions. It says the following but not the other
> way around.
> 
> "If FEAT_SPE_FDS is implemented, then FEAT_SPEv1p4 is implemented"
> 
> Even PMSIDR_EL1.FDS field is valid only when FEAT_SPEv1p4 is implemented. Hence

That's not strictly true. For any implemented version of SPE it's valid 
and res0, so it behaves the same way in the end. The only thing that's 
not valid is reading it when SPE isn't implemented at all which would 
give you an undef.

> you are right - we need to check both before enabling MDCR_EL3_ENPMS3.
> 
>          if ((mrs_field(ID_AA64DFR0_EL1, PMSVER) >= 5) &&
>              (mrs_field(PMSIDR_EL1, FDS)))
>                          mdcr |= MDCR_EL3_ENPMS3;
> 

Yes, either that or PMSVER >= 1. Personally I would do >= 1 because it's 
allowed and then you don't need to change it again if FEAT_SPE_FDS is 
relaxed to previous versions.

>>
>> It would be much easier to unconditionally set all the known MDCR bits. If the feature is implemented then setting them to 1 disables the traps, which we want, and if it's not implemented it does nothing which we also want.
>>
>> Doing them all conditionally is quite error prone and extra work for nothing. In TFA it's already done unconditionally:
> 
> But leaving all MDCR_EL3 fields set when corresponding features are not implemented
> goes against "don't set register fields when not required even when ignored in HW"
> principle and hence might not be ideal.
> 

But having to do 3 versions of something just to set 1 bit correctly 
that could have been set unconditionally with the same end result 
probably means that a blanket application of that principle isn't ideal 
either.

> But will leave it upto Mark and others. Dropping all these conditionality while
> setting MDCR_EL3 fields will be a subsequent patch later as if changes existing
> fields.
> 

It's probably not worth the subsequent patch because it was only 
supposed to save time for this patch, but it might be interesting to 
discuss anyway.

>>
>> *
>> * MDCR_EL3.EnPMSN (ARM v8.7) and MDCR_EL3.EnPMS3: Do not trap access to
>> * PMSNEVFR_EL1 or PMSDSFR_EL1 register at NS-EL1 or NS-EL2 to EL3 if FEAT_SPEv1p2
>> * or FEAT_SPE_FDS are implemented. Setting these bits to 1 doesn't have any
>> * effect on it when the features aren't implemented.
>> */
>> mdcr_el3_val |= MDCR_NSPB(MDCR_NSPB_EL1) | MDCR_EnPMSN_BIT | MDCR_EnPMS3_BIT;
>>
>>>        if (mrs_field(ID_AA64DFR0_EL1, TRACEBUFFER))
>>>            mdcr |= MDCR_EL3_NSTB_NS_NOTRAP;
>>>    
>>



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [boot-wrapper] aarch64: Enable access into FEAT_SPE_FDS register from EL2 and below
  2025-06-05  9:49     ` James Clark
@ 2025-06-06  4:45       ` Anshuman Khandual
  0 siblings, 0 replies; 5+ messages in thread
From: Anshuman Khandual @ 2025-06-06  4:45 UTC (permalink / raw)
  To: James Clark, linux-arm-kernel; +Cc: Mark Rutland, Mark Brown

On 05/06/25 3:19 PM, James Clark wrote:
> 
> 
> On 05/06/2025 4:15 am, Anshuman Khandual wrote:
>>
>>
>> On 6/4/25 17:27, James Clark wrote:
>>>
>>>
>>> On 04/06/2025 12:46 pm, Anshuman Khandual wrote:
>>>> FEAT_SPE_FDS adds system register PMSDSFR_EL1. But accessing that system
>>>> register from EL2 and below exception levels, will trap into EL3 unless
>>>> MDCR_EL3.EnPMS3 is set.
>>>>
>>>> Enable access to FEAT_SPE_FDS registers when they are implemented.
>>>>
>>>> Cc: James Clark <james.clark@linaro.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Mark Brown <broonie@kernel.org>
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>> ---
>>>>    arch/aarch64/include/asm/cpu.h | 4 ++++
>>>>    arch/aarch64/init.c            | 3 +++
>>>>    2 files changed, 7 insertions(+)
>>>>
>>>> diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
>>>> index 2b3a659..ac50474 100644
>>>> --- a/arch/aarch64/include/asm/cpu.h
>>>> +++ b/arch/aarch64/include/asm/cpu.h
>>>> @@ -55,6 +55,7 @@
>>>>    #define MDCR_EL3_NSTB_NS_NOTRAP            (UL(3) << 24)
>>>>    #define MDCR_EL3_SBRBE_NOTRAP_NOPROHIBIT    (UL(3) << 32)
>>>>    #define MDCR_EL3_ENPMSN                BIT(36)
>>>> +#define MDCR_EL3_ENPMS3                BIT(42)
>>>>    #define MDCR_EL3_EBWE                BIT(43)
>>>>    #define MDCR_EL3_EnPM2                BIT(7)
>>>>    @@ -185,6 +186,9 @@
>>>>      #define SCTLR_EL1_CP15BEN    (1 << 5)
>>>>    +#define PMSIDR_EL1        s3_0_c9_c9_7
>>>> +#define PMSIDR_EL1_FDS        BIT(7)
>>>> +
>>>>    #ifdef KERNEL_32
>>>>    /*
>>>>     * When booting a 32-bit kernel, EL1 uses AArch32 and registers which are
>>>> diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
>>>> index e1640a9..b6f740c 100644
>>>> --- a/arch/aarch64/init.c
>>>> +++ b/arch/aarch64/init.c
>>>> @@ -146,6 +146,9 @@ static void cpu_init_el3(void)
>>>>        if (mrs_field(ID_AA64DFR0_EL1, PMSVER) >= 3)
>>>>            mdcr |= MDCR_EL3_ENPMSN;
>>>>    +    if (mrs_field(PMSIDR_EL1, FDS))
>>>> +        mdcr |= MDCR_EL3_ENPMS3;
>>>> +
>>>
>>> You are only allowed to read PMSIDR if SPE is implemented otherwise it's undef so it needs another check.
>>
>> The same is true for the earlier bit MDCR_EL3_ENPMSN as well. FEAT_SPE_FnE should
>> have been checked from PMSIDR_EL1.FnE but the feature is guaranteed to be present
>> when FEAT_SPEv1p2 is present. Hence PMSVER >= 3 check alone is sufficient here.
>>
> 
> Exactly, ENPMSN availability can be determined only on the SPE version, avoiding the read of PMSIDR. So it's not really the same, as you mention.
> 
>> But for for FEAT_SPE_FDS feature, ARM DDI 0487 L.A is not very much clear on its
>> dependency for FEAT_SPE and its versions. It says the following but not the other
>> way around.
>>
>> "If FEAT_SPE_FDS is implemented, then FEAT_SPEv1p4 is implemented"
>>
>> Even PMSIDR_EL1.FDS field is valid only when FEAT_SPEv1p4 is implemented. Hence
> 
> That's not strictly true. For any implemented version of SPE it's valid and res0, so it behaves the same way in the end. The only thing that's not valid is reading it when SPE isn't implemented at all which would give you an undef.
> 
>> you are right - we need to check both before enabling MDCR_EL3_ENPMS3.
>>
>>          if ((mrs_field(ID_AA64DFR0_EL1, PMSVER) >= 5) &&
>>              (mrs_field(PMSIDR_EL1, FDS)))
>>                          mdcr |= MDCR_EL3_ENPMS3;
>>
> 
> Yes, either that or PMSVER >= 1. Personally I would do >= 1 because it's allowed and then you don't need to change it again if FEAT_SPE_FDS is relaxed to previous versions.

Hmm if this conditional dependency can be relaxed for earlier SPE
versions later on then it makes sense to compare against 1 itself.

> 
>>>
>>> It would be much easier to unconditionally set all the known MDCR bits. If the feature is implemented then setting them to 1 disables the traps, which we want, and if it's not implemented it does nothing which we also want.
>>>
>>> Doing them all conditionally is quite error prone and extra work for nothing. In TFA it's already done unconditionally:
>>
>> But leaving all MDCR_EL3 fields set when corresponding features are not implemented
>> goes against "don't set register fields when not required even when ignored in HW"
>> principle and hence might not be ideal.
>>
> 
> But having to do 3 versions of something just to set 1 bit correctly that could have been set unconditionally with the same end result probably means that a blanket application of that principle isn't ideal either.
> 
>> But will leave it upto Mark and others. Dropping all these conditionality while
>> setting MDCR_EL3 fields will be a subsequent patch later as if changes existing
>> fields.
>>
> 
> It's probably not worth the subsequent patch because it was only supposed to save time for this patch, but it might be interesting to discuss anyway.> 
>>>
>>> *
>>> * MDCR_EL3.EnPMSN (ARM v8.7) and MDCR_EL3.EnPMS3: Do not trap access to
>>> * PMSNEVFR_EL1 or PMSDSFR_EL1 register at NS-EL1 or NS-EL2 to EL3 if FEAT_SPEv1p2
>>> * or FEAT_SPE_FDS are implemented. Setting these bits to 1 doesn't have any
>>> * effect on it when the features aren't implemented.
>>> */
>>> mdcr_el3_val |= MDCR_NSPB(MDCR_NSPB_EL1) | MDCR_EnPMSN_BIT | MDCR_EnPMS3_BIT;
>>>
>>>>        if (mrs_field(ID_AA64DFR0_EL1, TRACEBUFFER))
>>>>            mdcr |= MDCR_EL3_NSTB_NS_NOTRAP;
>>>>    
>>>
> 






^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-06-06  4:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-04 11:46 [boot-wrapper] aarch64: Enable access into FEAT_SPE_FDS register from EL2 and below Anshuman Khandual
2025-06-04 11:57 ` James Clark
2025-06-05  3:15   ` Anshuman Khandual
2025-06-05  9:49     ` James Clark
2025-06-06  4:45       ` Anshuman Khandual

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox