linux-kernel - Re: [PATCH V3 1/2] arm64/debug: Drop redundant DBG_MDSCR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aElRi0Usn8dH6O4m@J2N7QTR9R3>
Date: Wed, 11 Jun 2025 10:55:41 +0100
From: Mark Rutland <mark.rutland@....com>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: linux-arm-kernel@...ts.infradead.org,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Ada Couprie Diaz <ada.coupriediaz@....com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3 1/2] arm64/debug: Drop redundant DBG_MDSCR_* macros

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.