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

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

[Anshuman Khandual]

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.

This series applies on v6.16-rc1

Changes in V3:

- Split out the self test changes into a separate patch per Mark
- Added RB tag from Ada

Changes in V2:

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

- Changed reg, val width to u64 in cortex_a76_erratum_1463225_svc_handler() per Ada
- Changed mdscr register width to uint64_t in enable_monitor_debug_exceptions() and
  install_ss() per Ada
    
Changes in V1:

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

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ada Couprie Diaz <ada.coupriediaz@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Anshuman Khandual (2):
  arm64/debug: Drop redundant DBG_MDSCR_* macros
  KVM: selftests: Change MDSCR_EL1 register holding variables as uint64_t

 arch/arm64/include/asm/assembler.h            |  4 ++--
 arch/arm64/include/asm/debug-monitors.h       |  6 -----
 arch/arm64/kernel/debug-monitors.c            | 22 +++++++++----------
 arch/arm64/kernel/entry-common.c              |  4 ++--
 .../selftests/kvm/arm64/debug-exceptions.c    |  4 ++--
 5 files changed, 17 insertions(+), 23 deletions(-)

-- 
2.25.1

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

[Anshuman Khandual]

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.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/assembler.h      |  4 ++--
 arch/arm64/include/asm/debug-monitors.h |  6 ------
 arch/arm64/kernel/debug-monitors.c      | 22 +++++++++++-----------
 arch/arm64/kernel/entry-common.c        |  4 ++--
 4 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index ad63457a05c5..f229d96616e5 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -53,7 +53,7 @@
 	.macro	disable_step_tsk, flgs, tmp
 	tbz	\flgs, #TIF_SINGLESTEP, 9990f
 	mrs	\tmp, mdscr_el1
-	bic	\tmp, \tmp, #DBG_MDSCR_SS
+	bic	\tmp, \tmp, #MDSCR_EL1_SS
 	msr	mdscr_el1, \tmp
 	isb	// Take effect before a subsequent clear of DAIF.D
 9990:
@@ -63,7 +63,7 @@
 	.macro	enable_step_tsk, flgs, tmp
 	tbz	\flgs, #TIF_SINGLESTEP, 9990f
 	mrs	\tmp, mdscr_el1
-	orr	\tmp, \tmp, #DBG_MDSCR_SS
+	orr	\tmp, \tmp, #MDSCR_EL1_SS
 	msr	mdscr_el1, \tmp
 9990:
 	.endm
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 8f6ba31b8658..1f37dd01482b 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -13,14 +13,8 @@
 #include <asm/ptrace.h>
 
 /* Low-level stepping controls. */
-#define DBG_MDSCR_SS		(1 << 0)
 #define DBG_SPSR_SS		(1 << 21)
 
-/* MDSCR_EL1 enabling bits */
-#define DBG_MDSCR_KDE		(1 << 13)
-#define DBG_MDSCR_MDE		(1 << 15)
-#define DBG_MDSCR_MASK		~(DBG_MDSCR_KDE | DBG_MDSCR_MDE)
-
 #define	DBG_ESR_EVT(x)		(((x) >> 27) & 0x7)
 
 /* AArch64 */
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 58f047de3e1c..08f1d02507cd 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -34,7 +34,7 @@ u8 debug_monitors_arch(void)
 /*
  * MDSCR access routines.
  */
-static void mdscr_write(u32 mdscr)
+static void mdscr_write(u64 mdscr)
 {
 	unsigned long flags;
 	flags = local_daif_save();
@@ -43,7 +43,7 @@ static void mdscr_write(u32 mdscr)
 }
 NOKPROBE_SYMBOL(mdscr_write);
 
-static u32 mdscr_read(void)
+static u64 mdscr_read(void)
 {
 	return read_sysreg(mdscr_el1);
 }
@@ -79,16 +79,16 @@ static DEFINE_PER_CPU(int, kde_ref_count);
 
 void enable_debug_monitors(enum dbg_active_el el)
 {
-	u32 mdscr, enable = 0;
+	u64 mdscr, enable = 0;
 
 	WARN_ON(preemptible());
 
 	if (this_cpu_inc_return(mde_ref_count) == 1)
-		enable = DBG_MDSCR_MDE;
+		enable = MDSCR_EL1_MDE;
 
 	if (el == DBG_ACTIVE_EL1 &&
 	    this_cpu_inc_return(kde_ref_count) == 1)
-		enable |= DBG_MDSCR_KDE;
+		enable |= MDSCR_EL1_KDE;
 
 	if (enable && debug_enabled) {
 		mdscr = mdscr_read();
@@ -100,16 +100,16 @@ NOKPROBE_SYMBOL(enable_debug_monitors);
 
 void disable_debug_monitors(enum dbg_active_el el)
 {
-	u32 mdscr, disable = 0;
+	u64 mdscr, disable = 0;
 
 	WARN_ON(preemptible());
 
 	if (this_cpu_dec_return(mde_ref_count) == 0)
-		disable = ~DBG_MDSCR_MDE;
+		disable = ~MDSCR_EL1_MDE;
 
 	if (el == DBG_ACTIVE_EL1 &&
 	    this_cpu_dec_return(kde_ref_count) == 0)
-		disable &= ~DBG_MDSCR_KDE;
+		disable &= ~MDSCR_EL1_KDE;
 
 	if (disable) {
 		mdscr = mdscr_read();
@@ -415,7 +415,7 @@ void kernel_enable_single_step(struct pt_regs *regs)
 {
 	WARN_ON(!irqs_disabled());
 	set_regs_spsr_ss(regs);
-	mdscr_write(mdscr_read() | DBG_MDSCR_SS);
+	mdscr_write(mdscr_read() | MDSCR_EL1_SS);
 	enable_debug_monitors(DBG_ACTIVE_EL1);
 }
 NOKPROBE_SYMBOL(kernel_enable_single_step);
@@ -423,7 +423,7 @@ NOKPROBE_SYMBOL(kernel_enable_single_step);
 void kernel_disable_single_step(void)
 {
 	WARN_ON(!irqs_disabled());
-	mdscr_write(mdscr_read() & ~DBG_MDSCR_SS);
+	mdscr_write(mdscr_read() & ~MDSCR_EL1_SS);
 	disable_debug_monitors(DBG_ACTIVE_EL1);
 }
 NOKPROBE_SYMBOL(kernel_disable_single_step);
@@ -431,7 +431,7 @@ NOKPROBE_SYMBOL(kernel_disable_single_step);
 int kernel_active_single_step(void)
 {
 	WARN_ON(!irqs_disabled());
-	return mdscr_read() & DBG_MDSCR_SS;
+	return mdscr_read() & MDSCR_EL1_SS;
 }
 NOKPROBE_SYMBOL(kernel_active_single_step);
 
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 7c1970b341b8..171f93f2494b 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -344,7 +344,7 @@ static DEFINE_PER_CPU(int, __in_cortex_a76_erratum_1463225_wa);
 
 static void cortex_a76_erratum_1463225_svc_handler(void)
 {
-	u32 reg, val;
+	u64 reg, val;
 
 	if (!unlikely(test_thread_flag(TIF_SINGLESTEP)))
 		return;
@@ -354,7 +354,7 @@ static void cortex_a76_erratum_1463225_svc_handler(void)
 
 	__this_cpu_write(__in_cortex_a76_erratum_1463225_wa, 1);
 	reg = read_sysreg(mdscr_el1);
-	val = reg | DBG_MDSCR_SS | DBG_MDSCR_KDE;
+	val = reg | MDSCR_EL1_SS | MDSCR_EL1_KDE;
 	write_sysreg(val, mdscr_el1);
 	asm volatile("msr daifclr, #8");
 	isb();
-- 
2.25.1

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

[Mark Rutland]

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.

Those are logically distinct changes, and AFAICT the latter is a
requirement for using extended breakpoints, where it would be clearer to
have that change as part of the series adding that support, with an
explanation as to why we care.

Mark.

> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Reviewed-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/include/asm/assembler.h      |  4 ++--
>  arch/arm64/include/asm/debug-monitors.h |  6 ------
>  arch/arm64/kernel/debug-monitors.c      | 22 +++++++++++-----------
>  arch/arm64/kernel/entry-common.c        |  4 ++--
>  4 files changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index ad63457a05c5..f229d96616e5 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -53,7 +53,7 @@
>  	.macro	disable_step_tsk, flgs, tmp
>  	tbz	\flgs, #TIF_SINGLESTEP, 9990f
>  	mrs	\tmp, mdscr_el1
> -	bic	\tmp, \tmp, #DBG_MDSCR_SS
> +	bic	\tmp, \tmp, #MDSCR_EL1_SS
>  	msr	mdscr_el1, \tmp
>  	isb	// Take effect before a subsequent clear of DAIF.D
>  9990:
> @@ -63,7 +63,7 @@
>  	.macro	enable_step_tsk, flgs, tmp
>  	tbz	\flgs, #TIF_SINGLESTEP, 9990f
>  	mrs	\tmp, mdscr_el1
> -	orr	\tmp, \tmp, #DBG_MDSCR_SS
> +	orr	\tmp, \tmp, #MDSCR_EL1_SS
>  	msr	mdscr_el1, \tmp
>  9990:
>  	.endm
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 8f6ba31b8658..1f37dd01482b 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -13,14 +13,8 @@
>  #include <asm/ptrace.h>
>  
>  /* Low-level stepping controls. */
> -#define DBG_MDSCR_SS		(1 << 0)
>  #define DBG_SPSR_SS		(1 << 21)
>  
> -/* MDSCR_EL1 enabling bits */
> -#define DBG_MDSCR_KDE		(1 << 13)
> -#define DBG_MDSCR_MDE		(1 << 15)
> -#define DBG_MDSCR_MASK		~(DBG_MDSCR_KDE | DBG_MDSCR_MDE)
> -
>  #define	DBG_ESR_EVT(x)		(((x) >> 27) & 0x7)
>  
>  /* AArch64 */
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 58f047de3e1c..08f1d02507cd 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -34,7 +34,7 @@ u8 debug_monitors_arch(void)
>  /*
>   * MDSCR access routines.
>   */
> -static void mdscr_write(u32 mdscr)
> +static void mdscr_write(u64 mdscr)
>  {
>  	unsigned long flags;
>  	flags = local_daif_save();
> @@ -43,7 +43,7 @@ static void mdscr_write(u32 mdscr)
>  }
>  NOKPROBE_SYMBOL(mdscr_write);
>  
> -static u32 mdscr_read(void)
> +static u64 mdscr_read(void)
>  {
>  	return read_sysreg(mdscr_el1);
>  }
> @@ -79,16 +79,16 @@ static DEFINE_PER_CPU(int, kde_ref_count);
>  
>  void enable_debug_monitors(enum dbg_active_el el)
>  {
> -	u32 mdscr, enable = 0;
> +	u64 mdscr, enable = 0;
>  
>  	WARN_ON(preemptible());
>  
>  	if (this_cpu_inc_return(mde_ref_count) == 1)
> -		enable = DBG_MDSCR_MDE;
> +		enable = MDSCR_EL1_MDE;
>  
>  	if (el == DBG_ACTIVE_EL1 &&
>  	    this_cpu_inc_return(kde_ref_count) == 1)
> -		enable |= DBG_MDSCR_KDE;
> +		enable |= MDSCR_EL1_KDE;
>  
>  	if (enable && debug_enabled) {
>  		mdscr = mdscr_read();
> @@ -100,16 +100,16 @@ NOKPROBE_SYMBOL(enable_debug_monitors);
>  
>  void disable_debug_monitors(enum dbg_active_el el)
>  {
> -	u32 mdscr, disable = 0;
> +	u64 mdscr, disable = 0;
>  
>  	WARN_ON(preemptible());
>  
>  	if (this_cpu_dec_return(mde_ref_count) == 0)
> -		disable = ~DBG_MDSCR_MDE;
> +		disable = ~MDSCR_EL1_MDE;
>  
>  	if (el == DBG_ACTIVE_EL1 &&
>  	    this_cpu_dec_return(kde_ref_count) == 0)
> -		disable &= ~DBG_MDSCR_KDE;
> +		disable &= ~MDSCR_EL1_KDE;
>  
>  	if (disable) {
>  		mdscr = mdscr_read();
> @@ -415,7 +415,7 @@ void kernel_enable_single_step(struct pt_regs *regs)
>  {
>  	WARN_ON(!irqs_disabled());
>  	set_regs_spsr_ss(regs);
> -	mdscr_write(mdscr_read() | DBG_MDSCR_SS);
> +	mdscr_write(mdscr_read() | MDSCR_EL1_SS);
>  	enable_debug_monitors(DBG_ACTIVE_EL1);
>  }
>  NOKPROBE_SYMBOL(kernel_enable_single_step);
> @@ -423,7 +423,7 @@ NOKPROBE_SYMBOL(kernel_enable_single_step);
>  void kernel_disable_single_step(void)
>  {
>  	WARN_ON(!irqs_disabled());
> -	mdscr_write(mdscr_read() & ~DBG_MDSCR_SS);
> +	mdscr_write(mdscr_read() & ~MDSCR_EL1_SS);
>  	disable_debug_monitors(DBG_ACTIVE_EL1);
>  }
>  NOKPROBE_SYMBOL(kernel_disable_single_step);
> @@ -431,7 +431,7 @@ NOKPROBE_SYMBOL(kernel_disable_single_step);
>  int kernel_active_single_step(void)
>  {
>  	WARN_ON(!irqs_disabled());
> -	return mdscr_read() & DBG_MDSCR_SS;
> +	return mdscr_read() & MDSCR_EL1_SS;
>  }
>  NOKPROBE_SYMBOL(kernel_active_single_step);
>  
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 7c1970b341b8..171f93f2494b 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -344,7 +344,7 @@ static DEFINE_PER_CPU(int, __in_cortex_a76_erratum_1463225_wa);
>  
>  static void cortex_a76_erratum_1463225_svc_handler(void)
>  {
> -	u32 reg, val;
> +	u64 reg, val;
>  
>  	if (!unlikely(test_thread_flag(TIF_SINGLESTEP)))
>  		return;
> @@ -354,7 +354,7 @@ static void cortex_a76_erratum_1463225_svc_handler(void)
>  
>  	__this_cpu_write(__in_cortex_a76_erratum_1463225_wa, 1);
>  	reg = read_sysreg(mdscr_el1);
> -	val = reg | DBG_MDSCR_SS | DBG_MDSCR_KDE;
> +	val = reg | MDSCR_EL1_SS | MDSCR_EL1_KDE;
>  	write_sysreg(val, mdscr_el1);
>  	asm volatile("msr daifclr, #8");
>  	isb();
> -- 
> 2.25.1
> 

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

[Anshuman Khandual]

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;
      |             ^

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.

arch/arm64/kvm/debug.c: u64 mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1) & ~(MDSCR_EL1_SS |
arch/arm64/kvm/debug.c:         mdscr |= MDSCR_EL1_SS;
arch/arm64/kvm/debug.c:         mdscr |= MDSCR_EL1_MDE | MDSCR_EL1_KDE;
arch/arm64/kvm/debug.c: vcpu->arch.external_mdscr_el1 = mdscr;
arch/arm64/kvm/debug.c: u64 mdscr;
arch/arm64/kvm/debug.c:         setup_external_mdscr(vcpu);
arch/arm64/kvm/debug.c:         mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
arch/arm64/kvm/debug.c:         if (mdscr & (MDSCR_EL1_KDE | MDSCR_EL1_MDE))

> 
> Those are logically distinct changes, and AFAICT the latter is a
> requirement for using extended breakpoints, where it would be clearer to
> have that change as part of the series adding that support, with an
> explanation as to why we care.
> 
> Mark.
> 
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Reviewed-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/include/asm/assembler.h      |  4 ++--
>>  arch/arm64/include/asm/debug-monitors.h |  6 ------
>>  arch/arm64/kernel/debug-monitors.c      | 22 +++++++++++-----------
>>  arch/arm64/kernel/entry-common.c        |  4 ++--
>>  4 files changed, 15 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
>> index ad63457a05c5..f229d96616e5 100644
>> --- a/arch/arm64/include/asm/assembler.h
>> +++ b/arch/arm64/include/asm/assembler.h
>> @@ -53,7 +53,7 @@
>>  	.macro	disable_step_tsk, flgs, tmp
>>  	tbz	\flgs, #TIF_SINGLESTEP, 9990f
>>  	mrs	\tmp, mdscr_el1
>> -	bic	\tmp, \tmp, #DBG_MDSCR_SS
>> +	bic	\tmp, \tmp, #MDSCR_EL1_SS
>>  	msr	mdscr_el1, \tmp
>>  	isb	// Take effect before a subsequent clear of DAIF.D
>>  9990:
>> @@ -63,7 +63,7 @@
>>  	.macro	enable_step_tsk, flgs, tmp
>>  	tbz	\flgs, #TIF_SINGLESTEP, 9990f
>>  	mrs	\tmp, mdscr_el1
>> -	orr	\tmp, \tmp, #DBG_MDSCR_SS
>> +	orr	\tmp, \tmp, #MDSCR_EL1_SS
>>  	msr	mdscr_el1, \tmp
>>  9990:
>>  	.endm
>> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
>> index 8f6ba31b8658..1f37dd01482b 100644
>> --- a/arch/arm64/include/asm/debug-monitors.h
>> +++ b/arch/arm64/include/asm/debug-monitors.h
>> @@ -13,14 +13,8 @@
>>  #include <asm/ptrace.h>
>>  
>>  /* Low-level stepping controls. */
>> -#define DBG_MDSCR_SS		(1 << 0)
>>  #define DBG_SPSR_SS		(1 << 21)
>>  
>> -/* MDSCR_EL1 enabling bits */
>> -#define DBG_MDSCR_KDE		(1 << 13)
>> -#define DBG_MDSCR_MDE		(1 << 15)
>> -#define DBG_MDSCR_MASK		~(DBG_MDSCR_KDE | DBG_MDSCR_MDE)
>> -
>>  #define	DBG_ESR_EVT(x)		(((x) >> 27) & 0x7)
>>  
>>  /* AArch64 */
>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>> index 58f047de3e1c..08f1d02507cd 100644
>> --- a/arch/arm64/kernel/debug-monitors.c
>> +++ b/arch/arm64/kernel/debug-monitors.c
>> @@ -34,7 +34,7 @@ u8 debug_monitors_arch(void)
>>  /*
>>   * MDSCR access routines.
>>   */
>> -static void mdscr_write(u32 mdscr)
>> +static void mdscr_write(u64 mdscr)
>>  {
>>  	unsigned long flags;
>>  	flags = local_daif_save();
>> @@ -43,7 +43,7 @@ static void mdscr_write(u32 mdscr)
>>  }
>>  NOKPROBE_SYMBOL(mdscr_write);
>>  
>> -static u32 mdscr_read(void)
>> +static u64 mdscr_read(void)
>>  {
>>  	return read_sysreg(mdscr_el1);
>>  }
>> @@ -79,16 +79,16 @@ static DEFINE_PER_CPU(int, kde_ref_count);
>>  
>>  void enable_debug_monitors(enum dbg_active_el el)
>>  {
>> -	u32 mdscr, enable = 0;
>> +	u64 mdscr, enable = 0;
>>  
>>  	WARN_ON(preemptible());
>>  
>>  	if (this_cpu_inc_return(mde_ref_count) == 1)
>> -		enable = DBG_MDSCR_MDE;
>> +		enable = MDSCR_EL1_MDE;
>>  
>>  	if (el == DBG_ACTIVE_EL1 &&
>>  	    this_cpu_inc_return(kde_ref_count) == 1)
>> -		enable |= DBG_MDSCR_KDE;
>> +		enable |= MDSCR_EL1_KDE;
>>  
>>  	if (enable && debug_enabled) {
>>  		mdscr = mdscr_read();
>> @@ -100,16 +100,16 @@ NOKPROBE_SYMBOL(enable_debug_monitors);
>>  
>>  void disable_debug_monitors(enum dbg_active_el el)
>>  {
>> -	u32 mdscr, disable = 0;
>> +	u64 mdscr, disable = 0;
>>  
>>  	WARN_ON(preemptible());
>>  
>>  	if (this_cpu_dec_return(mde_ref_count) == 0)
>> -		disable = ~DBG_MDSCR_MDE;
>> +		disable = ~MDSCR_EL1_MDE;
>>  
>>  	if (el == DBG_ACTIVE_EL1 &&
>>  	    this_cpu_dec_return(kde_ref_count) == 0)
>> -		disable &= ~DBG_MDSCR_KDE;
>> +		disable &= ~MDSCR_EL1_KDE;
>>  
>>  	if (disable) {
>>  		mdscr = mdscr_read();
>> @@ -415,7 +415,7 @@ void kernel_enable_single_step(struct pt_regs *regs)
>>  {
>>  	WARN_ON(!irqs_disabled());
>>  	set_regs_spsr_ss(regs);
>> -	mdscr_write(mdscr_read() | DBG_MDSCR_SS);
>> +	mdscr_write(mdscr_read() | MDSCR_EL1_SS);
>>  	enable_debug_monitors(DBG_ACTIVE_EL1);
>>  }
>>  NOKPROBE_SYMBOL(kernel_enable_single_step);
>> @@ -423,7 +423,7 @@ NOKPROBE_SYMBOL(kernel_enable_single_step);
>>  void kernel_disable_single_step(void)
>>  {
>>  	WARN_ON(!irqs_disabled());
>> -	mdscr_write(mdscr_read() & ~DBG_MDSCR_SS);
>> +	mdscr_write(mdscr_read() & ~MDSCR_EL1_SS);
>>  	disable_debug_monitors(DBG_ACTIVE_EL1);
>>  }
>>  NOKPROBE_SYMBOL(kernel_disable_single_step);
>> @@ -431,7 +431,7 @@ NOKPROBE_SYMBOL(kernel_disable_single_step);
>>  int kernel_active_single_step(void)
>>  {
>>  	WARN_ON(!irqs_disabled());
>> -	return mdscr_read() & DBG_MDSCR_SS;
>> +	return mdscr_read() & MDSCR_EL1_SS;
>>  }
>>  NOKPROBE_SYMBOL(kernel_active_single_step);
>>  
>> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
>> index 7c1970b341b8..171f93f2494b 100644
>> --- a/arch/arm64/kernel/entry-common.c
>> +++ b/arch/arm64/kernel/entry-common.c
>> @@ -344,7 +344,7 @@ static DEFINE_PER_CPU(int, __in_cortex_a76_erratum_1463225_wa);
>>  
>>  static void cortex_a76_erratum_1463225_svc_handler(void)
>>  {
>> -	u32 reg, val;
>> +	u64 reg, val;
>>  
>>  	if (!unlikely(test_thread_flag(TIF_SINGLESTEP)))
>>  		return;
>> @@ -354,7 +354,7 @@ static void cortex_a76_erratum_1463225_svc_handler(void)
>>  
>>  	__this_cpu_write(__in_cortex_a76_erratum_1463225_wa, 1);
>>  	reg = read_sysreg(mdscr_el1);
>> -	val = reg | DBG_MDSCR_SS | DBG_MDSCR_KDE;
>> +	val = reg | MDSCR_EL1_SS | MDSCR_EL1_KDE;
>>  	write_sysreg(val, mdscr_el1);
>>  	asm volatile("msr daifclr, #8");
>>  	isb();
>> -- 
>> 2.25.1
>>

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

[Mark Rutland]

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.

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

[Anshuman Khandual]

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.

[PATCH V3 2/2] KVM: selftests: Change MDSCR_EL1 register holding variables as uint64_t

[Anshuman Khandual]

Change MDSCR_EL1 register holding local variables as uint64_t that reflects
its true register width as well.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <oliver.upton@linux.dev>
Cc: Joey Gouly <joey.gouly@arm.com>
Cc: kvm@vger.kernel.org
Cc: kvmarm@lists.linux.dev
Cc: linux-kernel@vger.kernel.org
Cc: linux-kselftest@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Reviewed-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 tools/testing/selftests/kvm/arm64/debug-exceptions.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/arm64/debug-exceptions.c b/tools/testing/selftests/kvm/arm64/debug-exceptions.c
index c7fb55c9135b..e34963956fbc 100644
--- a/tools/testing/selftests/kvm/arm64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/arm64/debug-exceptions.c
@@ -140,7 +140,7 @@ static void enable_os_lock(void)
 
 static void enable_monitor_debug_exceptions(void)
 {
-	uint32_t mdscr;
+	uint64_t mdscr;
 
 	asm volatile("msr daifclr, #8");
 
@@ -223,7 +223,7 @@ void install_hw_bp_ctx(uint8_t addr_bp, uint8_t ctx_bp, uint64_t addr,
 
 static void install_ss(void)
 {
-	uint32_t mdscr;
+	uint64_t mdscr;
 
 	asm volatile("msr daifclr, #8");
 
-- 
2.25.1

Re: [PATCH V3 2/2] KVM: selftests: Change MDSCR_EL1 register holding variables as uint64_t

[Marc Zyngier]

On Tue, 10 Jun 2025 06:31:28 +0100,
Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
> Change MDSCR_EL1 register holding local variables as uint64_t that reflects
> its true register width as well.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Cc: Joey Gouly <joey.gouly@arm.com>
> Cc: kvm@vger.kernel.org
> Cc: kvmarm@lists.linux.dev
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-kselftest@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Reviewed-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  tools/testing/selftests/kvm/arm64/debug-exceptions.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/arm64/debug-exceptions.c b/tools/testing/selftests/kvm/arm64/debug-exceptions.c
> index c7fb55c9135b..e34963956fbc 100644
> --- a/tools/testing/selftests/kvm/arm64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/arm64/debug-exceptions.c
> @@ -140,7 +140,7 @@ static void enable_os_lock(void)
>  
>  static void enable_monitor_debug_exceptions(void)
>  {
> -	uint32_t mdscr;
> +	uint64_t mdscr;
>  
>  	asm volatile("msr daifclr, #8");
>  
> @@ -223,7 +223,7 @@ void install_hw_bp_ctx(uint8_t addr_bp, uint8_t ctx_bp, uint64_t addr,
>  
>  static void install_ss(void)
>  {
> -	uint32_t mdscr;
> +	uint64_t mdscr;
>  
>  	asm volatile("msr daifclr, #8");
>  

Why change this in the place that matters *the least*?

arch/arm64/kernel/debug-monitors.c is full of 32bit manipulation of
this register, and that's only one example of it. So if you are going
to change this, please do it fully, not as a random change in a random
file.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH V3 2/2] KVM: selftests: Change MDSCR_EL1 register holding variables as uint64_t

[Anshuman Khandual]

On 10/06/25 10:31 PM, Marc Zyngier wrote:
> On Tue, 10 Jun 2025 06:31:28 +0100,
> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
>>
>> Change MDSCR_EL1 register holding local variables as uint64_t that reflects
>> its true register width as well.
>>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Oliver Upton <oliver.upton@linux.dev>
>> Cc: Joey Gouly <joey.gouly@arm.com>
>> Cc: kvm@vger.kernel.org
>> Cc: kvmarm@lists.linux.dev
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-kselftest@vger.kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Reviewed-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  tools/testing/selftests/kvm/arm64/debug-exceptions.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/arm64/debug-exceptions.c b/tools/testing/selftests/kvm/arm64/debug-exceptions.c
>> index c7fb55c9135b..e34963956fbc 100644
>> --- a/tools/testing/selftests/kvm/arm64/debug-exceptions.c
>> +++ b/tools/testing/selftests/kvm/arm64/debug-exceptions.c
>> @@ -140,7 +140,7 @@ static void enable_os_lock(void)
>>  
>>  static void enable_monitor_debug_exceptions(void)
>>  {
>> -	uint32_t mdscr;
>> +	uint64_t mdscr;
>>  
>>  	asm volatile("msr daifclr, #8");
>>  
>> @@ -223,7 +223,7 @@ void install_hw_bp_ctx(uint8_t addr_bp, uint8_t ctx_bp, uint64_t addr,
>>  
>>  static void install_ss(void)
>>  {
>> -	uint32_t mdscr;
>> +	uint64_t mdscr;
>>  
>>  	asm volatile("msr daifclr, #8");
>>  
> 
> Why change this in the place that matters *the least*?
> 
> arch/arm64/kernel/debug-monitors.c is full of 32bit manipulation of
> this register, and that's only one example of it. So if you are going
> to change this, please do it fully, not as a random change in a random
> file.

The first patch in this series changes mdscr system register to 64 bit
in the mentioned file (i.e arch/arm64/kernel/debug-monitors.c). 

> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH V3 2/2] KVM: selftests: Change MDSCR_EL1 register holding variables as uint64_t

[Mark Rutland]

On Wed, Jun 11, 2025 at 09:15:10AM +0530, Anshuman Khandual wrote:
> 
> 
> On 10/06/25 10:31 PM, Marc Zyngier wrote:
> > On Tue, 10 Jun 2025 06:31:28 +0100,
> > Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> >>
> >> Change MDSCR_EL1 register holding local variables as uint64_t that reflects
> >> its true register width as well.
> >>
> >> Cc: Marc Zyngier <maz@kernel.org>
> >> Cc: Oliver Upton <oliver.upton@linux.dev>
> >> Cc: Joey Gouly <joey.gouly@arm.com>
> >> Cc: kvm@vger.kernel.org
> >> Cc: kvmarm@lists.linux.dev
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: linux-kselftest@vger.kernel.org
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> Reviewed-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> >> ---
> >>  tools/testing/selftests/kvm/arm64/debug-exceptions.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/kvm/arm64/debug-exceptions.c b/tools/testing/selftests/kvm/arm64/debug-exceptions.c
> >> index c7fb55c9135b..e34963956fbc 100644
> >> --- a/tools/testing/selftests/kvm/arm64/debug-exceptions.c
> >> +++ b/tools/testing/selftests/kvm/arm64/debug-exceptions.c
> >> @@ -140,7 +140,7 @@ static void enable_os_lock(void)
> >>  
> >>  static void enable_monitor_debug_exceptions(void)
> >>  {
> >> -	uint32_t mdscr;
> >> +	uint64_t mdscr;
> >>  
> >>  	asm volatile("msr daifclr, #8");
> >>  
> >> @@ -223,7 +223,7 @@ void install_hw_bp_ctx(uint8_t addr_bp, uint8_t ctx_bp, uint64_t addr,
> >>  
> >>  static void install_ss(void)
> >>  {
> >> -	uint32_t mdscr;
> >> +	uint64_t mdscr;
> >>  
> >>  	asm volatile("msr daifclr, #8");
> >>  
> > 
> > Why change this in the place that matters *the least*?
> > 
> > arch/arm64/kernel/debug-monitors.c is full of 32bit manipulation of
> > this register, and that's only one example of it. So if you are going
> > to change this, please do it fully, not as a random change in a random
> > file.
> 
> The first patch in this series changes mdscr system register to 64 bit
> in the mentioned file (i.e arch/arm64/kernel/debug-monitors.c). 

You did not Cc Marc on oatch 1 or the cover letter. KVM folk are only
Cc'd on patch 2.

Marc, for context the series is:

  https://lore.kernel.org/linux-arm-kernel/20250610053128.4118784-1-anshuman.khandual@arm.com/

... and I've asked Anshuman to better describe the rationale.

Mark.

Re: [PATCH V3 2/2] KVM: selftests: Change MDSCR_EL1 register holding variables as uint64_t

[Marc Zyngier]

On Wed, 11 Jun 2025 04:45:10 +0100,
Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
> 
> 
> On 10/06/25 10:31 PM, Marc Zyngier wrote:
> > On Tue, 10 Jun 2025 06:31:28 +0100,
> > Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> >>
> >> Change MDSCR_EL1 register holding local variables as uint64_t that reflects
> >> its true register width as well.
> >>
> >> Cc: Marc Zyngier <maz@kernel.org>
> >> Cc: Oliver Upton <oliver.upton@linux.dev>
> >> Cc: Joey Gouly <joey.gouly@arm.com>
> >> Cc: kvm@vger.kernel.org
> >> Cc: kvmarm@lists.linux.dev
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: linux-kselftest@vger.kernel.org
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> Reviewed-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> >> ---
> >>  tools/testing/selftests/kvm/arm64/debug-exceptions.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/kvm/arm64/debug-exceptions.c b/tools/testing/selftests/kvm/arm64/debug-exceptions.c
> >> index c7fb55c9135b..e34963956fbc 100644
> >> --- a/tools/testing/selftests/kvm/arm64/debug-exceptions.c
> >> +++ b/tools/testing/selftests/kvm/arm64/debug-exceptions.c
> >> @@ -140,7 +140,7 @@ static void enable_os_lock(void)
> >>  
> >>  static void enable_monitor_debug_exceptions(void)
> >>  {
> >> -	uint32_t mdscr;
> >> +	uint64_t mdscr;
> >>  
> >>  	asm volatile("msr daifclr, #8");
> >>  
> >> @@ -223,7 +223,7 @@ void install_hw_bp_ctx(uint8_t addr_bp, uint8_t ctx_bp, uint64_t addr,
> >>  
> >>  static void install_ss(void)
> >>  {
> >> -	uint32_t mdscr;
> >> +	uint64_t mdscr;
> >>  
> >>  	asm volatile("msr daifclr, #8");
> >>  
> > 
> > Why change this in the place that matters *the least*?
> > 
> > arch/arm64/kernel/debug-monitors.c is full of 32bit manipulation of
> > this register, and that's only one example of it. So if you are going
> > to change this, please do it fully, not as a random change in a random
> > file.
> 
> The first patch in this series changes mdscr system register to 64 bit
> in the mentioned file (i.e arch/arm64/kernel/debug-monitors.c).

Then please Cc me on the series, and not patches at random. Context matters.

	M.

-- 
Without deviation from the norm, progress is not possible.