Re: [PATCH V3 1/2] arm64/debug: Drop redundant DBG_MDSCR_* macros

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 11/06/25 3:25 PM, Mark Rutland wrote:
> On Wed, Jun 11, 2025 at 09:10:45AM +0530, Anshuman Khandual wrote:
>>
>>
>> On 10/06/25 10:43 PM, Mark Rutland wrote:
>>> On Tue, Jun 10, 2025 at 11:01:27AM +0530, Anshuman Khandual wrote:
>>>> MDSCR_EL1 has already been defined in tools sysreg format and hence can be
>>>> used in all debug monitor related call paths. Subsequently all DBG_MDSCR_*
>>>> macros become redundant and hence can be dropped off completely. While here
>>>> convert all variables handling MDSCR_EL1 register as u64 which reflects its
>>>> true width as well.
>>>
>>> I think that for now it'd be best to *only* change over to the
>>> generated MDSCR_EL1_* defintions, and leave the register sizes as-is.
>>
>> I had tried doing that originally but without changing mdscr register size,
>> there is a build warning because MDSCR_EL1_MDE is defined as GENMASK(15, 15)
>> which is represented as 'long unsigned int'.
>>
>> #define __GENMASK(h, l) (((~_UL(0)) << (l)) & (~_UL(0) >> (BITS_PER_LONG - 1 - (h))))
>>
>> arch/arm64/kernel/debug-monitors.c: In function ‘disable_debug_monitors’:
>> arch/arm64/kernel/debug-monitors.c:108:13: warning: conversion from ‘long unsigned int’ to ‘u32’ {aka ‘unsigned int’} changes value from ‘18446744073709518847’ to ‘4294934527’ [-Woverflow]
>>   108 |   disable = ~MDSCR_EL1_MDE;
>>       |             ^
> 
> Please mention that in the commit message. As-is, the commit message has
> no rationale for changing to u64.

Sure, agreed. I had missed that, it was my bad.> 
> More generally, if you need to make a change to avoid a compiler
> warning, please describe that as part of the rationale.
Makes sense, will do.

> 
>> MDSCR_EL1 is a 64 bit system register as per ARM DDI 0487 L.A (D24.3.20).
>> Representing it as u32 does not seem right irrespective of whether the
>> extended break point support is enabled or not. Besides even arm64 kvm
>> uses u64 for mdscr register.
> 
> Sure, but that wasn't my complaint.
> 
> My complaint was that it was a logically unrelated change, because you
> had provided no rationale as for why it was necessary to change to u64
> as a conseqeunce of changing to the generated MDSCR_EL1_* definitions.
> 
> Please also note that *almost all* system registers have the
> "${REGISTER} is a 64-bit register wording", including things like DAIF,
> SPSel, etc. It's necessary to consider the context of use.

Understood.

I will add a rationale in the commit message for the u64 changes along
with changes related to the generated MDSCR_EL1_* definitions and then
re-spin the series. Thanks for your review.

> 
> Mark.