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

[Mark Rutland <mark.rutland@arm.com>]

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.

More generally, if you need to make a change to avoid a compiler
warning, please describe that as part of the rationale.

> 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.

Mark.