Re: [PATCH 1/3] arm64/cpufeature: Add field details for ID_AA64DFR1_EL1 register

Re: [PATCH 1/3] arm64/cpufeature: Add field details for ID_AA64DFR1_EL1 register

[Mark Rutland]

On Wed, Oct 23, 2024 at 11:18:12AM +0530, Anshuman Khandual wrote:
> 
> 
> On 10/22/24 21:26, Mark Rutland wrote:
> > On Tue, Oct 01, 2024 at 10:06:00AM +0530, Anshuman Khandual wrote:
> >> This adds required field details for ID_AA64DFR1_EL1, and also drops dummy
> >> ftr_raz[] array which is now redundant. These register fields will be used
> >> to enable increased breakpoint and watchpoint registers via FEAT_Debugv8p9
> >> later.

> >> +static const struct arm64_ftr_bits ftr_id_aa64dfr1[] = {
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABL_CMPs_SHIFT, 8, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_DPFZS_SHIFT, 4, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_EBEP_SHIFT, 4, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ITE_SHIFT, 4, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABLE_SHIFT, 4, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_PMICNTR_SHIFT, 4, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SPMU_SHIFT, 4, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_CTX_CMPs_SHIFT, 8, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_WRPs_SHIFT, 8, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_BRPs_SHIFT, 8, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SYSPMUID_SHIFT, 8, 0),
> >> +	ARM64_FTR_END,
> >> +};
> >> +
> > 
> > Is there some general principle that has been applied here? e.g. is this
> > STRICT unless we know of variation in practice?
> 
> Yes, that's correct. STRICT unless there is a known variation in practice.

Please mention that somewhere, e.g. in the commit message.

> > e.g. it seems a bit odd that ABLE cannot vary while the number of
> > breakpoints can...
> But all these (ABL_CMPs, CTX_CMPs, WRPs, BRPs) are marked as FTR_NONSTRICT.
> Would not that allow ABL_CMPs to vary as well ?

I asked about ABLE, not ABL_CMPs.

ABL_CMPs is marked as FTR_NONSTRICT, but ABLE is marked as FTR_STRICT.

> Although the existing break-point numbers are currently marked FTR_STRICT,
> should they be changed first ?
> 
> static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
> 	...................
> 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_WRPs_SHIFT, 4, 0),
>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRPs_SHIFT, 4, 0),
> 	...................
> }

My point was that the above didn't seem to be logically consistent; I
think you didn't handle ABLE as you should have.

Mark.

Re: [PATCH 2/3] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[Mark Rutland]

On Wed, Oct 23, 2024 at 11:42:37AM +0530, Anshuman Khandual wrote:
> 
> 
> On 10/22/24 21:40, Mark Rutland wrote:
> > On Tue, Oct 01, 2024 at 10:06:01AM +0530, Anshuman Khandual wrote:
> >> Fine grained trap control for MDSELR_EL1 register needs to be configured in
> >> HDFGRTR2_EL2, and HDFGWTR2_EL2 registers when kernel enters at EL1, but EL2
> >> is also present. This adds a new helper __init_el2_fgt2() initializing this
> >> new FEAT_FGT2 based fine grained registers.
> >>
> >> MDCR_EL2.EBWE needs to be enabled for additional (beyond 16) breakpoint and
> >> watchpoint exceptions when kernel enters at EL1, but EL2 is also present.
> >> This updates __init_el2_debug() as required for FEAT_Debugv8p9.
> >>
> >> While here, also update booting.rst with MDCR_EL3 and SCR_EL3 requirements.
> > 
> > [...]
> > 
> >> +  For CPUs with FEAT_Debugv8p9 extension present:
> >> +
> >> +  - If the kernel is entered at EL1 and EL2 is present:
> >> +
> >> +    - HDFGRTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
> >> +    - HDFGWTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
> >> +    - MDCR_EL2.EBWE (bit 43) must be initialized to 0b1
> >> +
> >> +  - If EL3 is present:
> >> +
> >> +    - MDCR_EL3.TDA (bit 9) must be initialized to 0b0
> > 
> > AFAICT we need TDA==0 this regardless of FEAT_Debugv8p9 (and e.g. we need
> 
> That's because MDCR_EL3.TDA=0, enables access to many other debug registers
> beside FEAT_Debugv8p9, which are currently used and hence this MDCR_EL3.TDA
> =0 requirement is a not a new one but rather a missing one instead ?

Yes, that's why I said we need it regardless; it's an existing
requirement that wasn't documented.

> 
> > MDCR_EL3.TPM==0 where FEAT_PMUv3 is implemented), so we should probably
> > check if there's anything else we haven't yet documented in MDCR_EL3.
> 
> Will scan through MDCR_EL3 register and match it with existing documentation
> i.e Documentation/arch/arm64/booting.rst. If there are some missing MDCR_EL3
> fields which should be mentioned, will add them via a separate pre-requisite
> patch ?

Yes please.

Mark.

Re: [PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Mark Rutland]

On Wed, Oct 23, 2024 at 01:01:52PM +0530, Anshuman Khandual wrote:
> 
> 
> On 10/22/24 21:04, Mark Rutland wrote:
> > On Tue, Oct 01, 2024 at 10:06:02AM +0530, Anshuman Khandual wrote:

[...]

> > Wherever this lives it needs a comment explaining what it is doing and
> > why. I assume this is intended to protect the bank in sequences like:
> > 
> > 	MSR	MDSELR, <...>
> > 	ISB
> > 	MRS	<..._, BANKED_REGISTER
> 
> Correct, it is protecting the above sequence.
> 
> > 
> > ... but is theat suffucient for mutual exclusion against
> > exception handlers, or does that come from somewhere else?
> 
> Looking at all existing use cases for breakpoint/watchpoints, it should
> be sufficient to protect against mutual exclusion. But thinking, do you
> have a particular exception handler scenario in mind where this might
> still be problematic ? Will keep looking into it.

Where does the mutual exclusion come from for the existing sequences?
We should be able to descrive should be able to describe that in the
commit message or in a comment somewhere (or better, with some
assertions that get tested).

For example, what prevents watchpoint_handler() from firing in the
middle of arch_install_hw_breakpoint() or
arch_uninstall_hw_breakpoint()?

Is the existing code correct?

Mark.

Re: [PATCH 1/3] arm64/cpufeature: Add field details for ID_AA64DFR1_EL1 register

[Anshuman Khandual]

On 10/28/24 18:03, Mark Rutland wrote:
> On Wed, Oct 23, 2024 at 11:18:12AM +0530, Anshuman Khandual wrote:
>>
>>
>> On 10/22/24 21:26, Mark Rutland wrote:
>>> On Tue, Oct 01, 2024 at 10:06:00AM +0530, Anshuman Khandual wrote:
>>>> This adds required field details for ID_AA64DFR1_EL1, and also drops dummy
>>>> ftr_raz[] array which is now redundant. These register fields will be used
>>>> to enable increased breakpoint and watchpoint registers via FEAT_Debugv8p9
>>>> later.
> 
>>>> +static const struct arm64_ftr_bits ftr_id_aa64dfr1[] = {
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABL_CMPs_SHIFT, 8, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_DPFZS_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_EBEP_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ITE_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABLE_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_PMICNTR_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SPMU_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_CTX_CMPs_SHIFT, 8, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_WRPs_SHIFT, 8, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_BRPs_SHIFT, 8, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SYSPMUID_SHIFT, 8, 0),
>>>> +	ARM64_FTR_END,
>>>> +};
>>>> +
>>>
>>> Is there some general principle that has been applied here? e.g. is this
>>> STRICT unless we know of variation in practice?
>>
>> Yes, that's correct. STRICT unless there is a known variation in practice.
> 
> Please mention that somewhere, e.g. in the commit message.

Sure, will add that.

> 
>>> e.g. it seems a bit odd that ABLE cannot vary while the number of
>>> breakpoints can...
>> But all these (ABL_CMPs, CTX_CMPs, WRPs, BRPs) are marked as FTR_NONSTRICT.
>> Would not that allow ABL_CMPs to vary as well ?
> 
> I asked about ABLE, not ABL_CMPs.
> 
> ABL_CMPs is marked as FTR_NONSTRICT, but ABLE is marked as FTR_STRICT.

Ahh, that was my bad, completely missed.

> 
>> Although the existing break-point numbers are currently marked FTR_STRICT,
>> should they be changed first ?
>>
>> static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>> 	...................
>> 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
>>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_WRPs_SHIFT, 4, 0),
>>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRPs_SHIFT, 4, 0),
>> 	...................
>> }
> 
> My point was that the above didn't seem to be logically consistent; I
> think you didn't handle ABLE as you should have.

Agreed, will change ABLE as FTR_NONSTRICT instead.

But what about the ID_AA64DFR0_EL1_WRPs_SHIFT and ID_AA64DFR0_EL1_BRPs_SHIFT
which could have variations in different cpus on the same system ? So should
those be fixed as FTR_NONSTRICT first ?

I have posted V2 for this series earlier today, hence will accommodate all the
new comments here in V3 going forward.

https://lore.kernel.org/all/20241028053426.2486633-1-anshuman.khandual@arm.com/

Re: [PATCH 2/3] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[Anshuman Khandual]

On 10/28/24 18:05, Mark Rutland wrote:
> On Wed, Oct 23, 2024 at 11:42:37AM +0530, Anshuman Khandual wrote:
>>
>>
>> On 10/22/24 21:40, Mark Rutland wrote:
>>> On Tue, Oct 01, 2024 at 10:06:01AM +0530, Anshuman Khandual wrote:
>>>> Fine grained trap control for MDSELR_EL1 register needs to be configured in
>>>> HDFGRTR2_EL2, and HDFGWTR2_EL2 registers when kernel enters at EL1, but EL2
>>>> is also present. This adds a new helper __init_el2_fgt2() initializing this
>>>> new FEAT_FGT2 based fine grained registers.
>>>>
>>>> MDCR_EL2.EBWE needs to be enabled for additional (beyond 16) breakpoint and
>>>> watchpoint exceptions when kernel enters at EL1, but EL2 is also present.
>>>> This updates __init_el2_debug() as required for FEAT_Debugv8p9.
>>>>
>>>> While here, also update booting.rst with MDCR_EL3 and SCR_EL3 requirements.
>>>
>>> [...]
>>>
>>>> +  For CPUs with FEAT_Debugv8p9 extension present:
>>>> +
>>>> +  - If the kernel is entered at EL1 and EL2 is present:
>>>> +
>>>> +    - HDFGRTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
>>>> +    - HDFGWTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
>>>> +    - MDCR_EL2.EBWE (bit 43) must be initialized to 0b1
>>>> +
>>>> +  - If EL3 is present:
>>>> +
>>>> +    - MDCR_EL3.TDA (bit 9) must be initialized to 0b0
>>>
>>> AFAICT we need TDA==0 this regardless of FEAT_Debugv8p9 (and e.g. we need
>>
>> That's because MDCR_EL3.TDA=0, enables access to many other debug registers
>> beside FEAT_Debugv8p9, which are currently used and hence this MDCR_EL3.TDA
>> =0 requirement is a not a new one but rather a missing one instead ?
> 
> Yes, that's why I said we need it regardless; it's an existing
> requirement that wasn't documented.

Alright, got it.

> 
>>
>>> MDCR_EL3.TPM==0 where FEAT_PMUv3 is implemented), so we should probably
>>> check if there's anything else we haven't yet documented in MDCR_EL3.
>>
>> Will scan through MDCR_EL3 register and match it with existing documentation
>> i.e Documentation/arch/arm64/booting.rst. If there are some missing MDCR_EL3
>> fields which should be mentioned, will add them via a separate pre-requisite
>> patch ?
> 
> Yes please.
> 
> Mark.

Sure, will separate those changes in a pre-requisite patch as suggested.

Re: [PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Anshuman Khandual]

On 10/28/24 18:17, Mark Rutland wrote:
> On Wed, Oct 23, 2024 at 01:01:52PM +0530, Anshuman Khandual wrote:
>>
>>
>> On 10/22/24 21:04, Mark Rutland wrote:
>>> On Tue, Oct 01, 2024 at 10:06:02AM +0530, Anshuman Khandual wrote:
> 
> [...]
> 
>>> Wherever this lives it needs a comment explaining what it is doing and
>>> why. I assume this is intended to protect the bank in sequences like:
>>>
>>> 	MSR	MDSELR, <...>
>>> 	ISB
>>> 	MRS	<..._, BANKED_REGISTER
>>
>> Correct, it is protecting the above sequence.
>>
>>>
>>> ... but is theat suffucient for mutual exclusion against
>>> exception handlers, or does that come from somewhere else?
>>
>> Looking at all existing use cases for breakpoint/watchpoints, it should
>> be sufficient to protect against mutual exclusion. But thinking, do you
>> have a particular exception handler scenario in mind where this might
>> still be problematic ? Will keep looking into it.
> 
> Where does the mutual exclusion come from for the existing sequences?

Bank selection followed by indexed read/write, inherently requires mutual
exclusion (ensuring that both these steps executed together) in order to
prevent read/write into wrong registers. That being said, HW breakpoints
get used in multiple different places such as perf, ptrace, debug monitor
based single stepping etc calling platform functions which operate on the
HW breakpoint registers here.

preempt_disable()/enable() sequence in the very last leaf level helpers
such as [read|write]_wb_reg(), will ensure required mutual exclusion.

> We should be able to descrive should be able to describe that in the
> commit message or in a comment somewhere (or better, with some
> assertions that get tested).

Planning to add a comment - something like this both for read and write
helpers.
       /*
        * Bank selection in MDSELR_EL1, followed by indexed read from
        * [break|watch]point registers cannot be interrupted, as that
        * might cause misread from wrong targets. Hence this requires
        * mutual exclusion via preventing any preemption.
        */

But regarding adding assertions, could you give some more details and
it will be great to have some relevant examples as well.

> 
> For example, what prevents watchpoint_handler() from firing in the
> middle of arch_install_hw_breakpoint() or
> arch_uninstall_hw_breakpoint()?

If perf is the only user, watchpoint_handler() will not get triggered
without watchpoints being installed via arch_install_hw_breakpoint().
Similarly once they get uninstalled via arch_uninstall_hw_breakpoint()
there will not be active watchpoints to trigger the handler. Although
there are other users (ptrace, debug monitor etc) besides perf which
could also be active simultaneously and race with each other ? TBH, I
am not sure.

> 
> Is the existing code correct?

I have not tested the concurrency aspects of the HW breakpoints enough
to be able to answer that question. But if there is a particular concern
here, happy to look into that.

But wondering how does this new bank indexed read/write mechanism (after
taking care of the mutual exclusion in the leaf level helpers such as
[read| write]_wb_reg()) still makes the existing concurrency situation
worse off than earlier ?

Re: [PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Mark Rutland]

On Tue, Oct 29, 2024 at 01:06:38PM +0530, Anshuman Khandual wrote:
> On 10/28/24 18:17, Mark Rutland wrote:
> > On Wed, Oct 23, 2024 at 01:01:52PM +0530, Anshuman Khandual wrote:
> >> On 10/22/24 21:04, Mark Rutland wrote:

> >>> I assume this is intended to protect the bank in sequences like:
> >>>
> >>> 	MSR	MDSELR, <...>
> >>> 	ISB
> >>> 	MRS	<..._, BANKED_REGISTER
> >>
> >> Correct, it is protecting the above sequence.
> >>
> >>> ... but is theat suffucient for mutual exclusion against
> >>> exception handlers, or does that come from somewhere else?
> >>
> >> Looking at all existing use cases for breakpoint/watchpoints, it should
> >> be sufficient to protect against mutual exclusion. But thinking, do you
> >> have a particular exception handler scenario in mind where this might
> >> still be problematic ? Will keep looking into it.
> > 
> > Where does the mutual exclusion come from for the existing sequences?
> 
> Bank selection followed by indexed read/write, inherently requires mutual
> exclusion (ensuring that both these steps executed together) in order to
> prevent read/write into wrong registers. That being said, HW breakpoints
> get used in multiple different places such as perf, ptrace, debug monitor
> based single stepping etc calling platform functions which operate on the
> HW breakpoint registers here.

Yes; that's *why* I'm asking. 

> preempt_disable()/enable() sequence in the very last leaf level helpers
> such as [read|write]_wb_reg(), will ensure required mutual exclusion.

I do not believe that this assertion is correct.

I specifically gave the example of mutual exclusion against exception
handlers, and preempt_disable() ... preempt_disable() does not prevent
exceptions being taken, so disabling preemption *cannot* be sufficient
to provide mutual exclusion against exception handlers.

What prevents a race with an exception handler? e.g. 

* Does the structure of the code prevent that somehow?

* What context(s) does this code execute in?
  - Are debug exceptions always masked?
  - Do we disable breakpoints/watchpoints around (some) manipulation of
    the relevant registers?

> > We should be able to descrive should be able to describe that in the
> > commit message or in a comment somewhere (or better, with some
> > assertions that get tested).
> 
> Planning to add a comment - something like this both for read and write
> helpers.
>        /*
>         * Bank selection in MDSELR_EL1, followed by indexed read from
>         * [break|watch]point registers cannot be interrupted, as that
>         * might cause misread from wrong targets. Hence this requires
>         * mutual exclusion via preventing any preemption.
>         */

As above, I do not believe this is correct. At minimum, disabling
preemption is not the full story here.

> But regarding adding assertions, could you give some more details and
> it will be great to have some relevant examples as well.

I've given some suggestions above. Please go and read the code and
figure this out.

> > For example, what prevents watchpoint_handler() from firing in the
> > middle of arch_install_hw_breakpoint() or
> > arch_uninstall_hw_breakpoint()?
> 
> If perf is the only user, watchpoint_handler() will not get triggered
> without watchpoints being installed via arch_install_hw_breakpoint().
> Similarly once they get uninstalled via arch_uninstall_hw_breakpoint()
> there will not be active watchpoints to trigger the handler. Although
> there are other users (ptrace, debug monitor etc) besides perf which
> could also be active simultaneously and race with each other ? TBH, I
> am not sure.

Please go and read the code and figure this out.

> > Is the existing code correct?
> 
> I have not tested the concurrency aspects of the HW breakpoints enough
> to be able to answer that question. But if there is a particular concern
> here, happy to look into that.
> 
> But wondering how does this new bank indexed read/write mechanism (after
> taking care of the mutual exclusion in the leaf level helpers such as
> [read| write]_wb_reg()) still makes the existing concurrency situation
> worse off than earlier ?

Please go and read the code and figure this out.

Mark.