[RFC 0/4] arm64/mm: Clean up pte_dirty() state management

[RFC 0/4] arm64/mm: Clean up pte_dirty() state management

[Anshuman Khandual]

These pte_dirty() changes make things explicitly clear, while improving the
code readability. This optimizes HW dirty state transfer into SW dirty bit.
This also adds a new arm64 documentation explaining overall pte dirty state
management in detail. This series applies on the latest mainline kernel.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Ryan Roberts <ryan.roberts@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Hildenbrand <david@redhat.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-doc@vger.kernel.org

Anshuman Khandual (4):
  arm64/mm: Add SW and HW dirty state helpers
  arm64/mm: Call pte_sw_mkdirty() while preserving the HW dirty state
  arm64/mm: Add pte_preserve_hw_dirty()
  docs: arm64: Add help document for pte dirty state management

 Documentation/arch/arm64/index.rst     |  1 +
 Documentation/arch/arm64/pte-dirty.rst | 95 ++++++++++++++++++++++++++
 arch/arm64/include/asm/pgtable.h       | 66 ++++++++++++++----
 3 files changed, 147 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/arch/arm64/pte-dirty.rst

-- 
2.30.2

[RFC 1/4] arm64/mm: Add SW and HW dirty state helpers

[Anshuman Khandual]

This factors out low level SW and HW state changes i.e make and clear into
separate helpers making them explicit improving readability. This also adds
pte_rdonly() helper as well. No functional change is intended.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/pgtable.h | 52 ++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 0bd18de9fd97..fb03be697819 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -103,6 +103,7 @@ static inline pteval_t __phys_to_pte_val(phys_addr_t phys)
 #define pte_young(pte)		(!!(pte_val(pte) & PTE_AF))
 #define pte_special(pte)	(!!(pte_val(pte) & PTE_SPECIAL))
 #define pte_write(pte)		(!!(pte_val(pte) & PTE_WRITE))
+#define pte_rdonly(pte)		(!!(pte_val(pte) & PTE_RDONLY))
 #define pte_user(pte)		(!!(pte_val(pte) & PTE_USER))
 #define pte_user_exec(pte)	(!(pte_val(pte) & PTE_UXN))
 #define pte_cont(pte)		(!!(pte_val(pte) & PTE_CONT))
@@ -120,7 +121,7 @@ static inline pteval_t __phys_to_pte_val(phys_addr_t phys)
 	(__boundary - 1 < (end) - 1) ? __boundary : (end);			\
 })
 
-#define pte_hw_dirty(pte)	(pte_write(pte) && !(pte_val(pte) & PTE_RDONLY))
+#define pte_hw_dirty(pte)	(pte_write(pte) && !pte_rdonly(pte))
 #define pte_sw_dirty(pte)	(!!(pte_val(pte) & PTE_DIRTY))
 #define pte_dirty(pte)		(pte_sw_dirty(pte) || pte_hw_dirty(pte))
 
@@ -174,6 +175,39 @@ static inline pmd_t clear_pmd_bit(pmd_t pmd, pgprot_t prot)
 	return pmd;
 }
 
+static inline pte_t pte_hw_mkdirty(pte_t pte)
+{
+	if (pte_write(pte))
+		pte = clear_pte_bit(pte, __pgprot(PTE_RDONLY));
+
+	return pte;
+}
+
+static inline pte_t pte_sw_mkdirty(pte_t pte)
+{
+	return set_pte_bit(pte, __pgprot(PTE_DIRTY));
+}
+
+static inline __always_unused pte_t pte_hw_clr_dirty(pte_t pte)
+{
+	return set_pte_bit(pte, __pgprot(PTE_RDONLY));
+}
+
+static inline pte_t pte_sw_clr_dirty(pte_t pte)
+{
+	pte = clear_pte_bit(pte, __pgprot(PTE_DIRTY));
+
+	/*
+	 * Clearing the software dirty state requires clearing
+	 * the PTE_DIRTY bit along with setting the PTE_RDONLY
+	 * ensuring a page fault on subsequent write access.
+	 *
+	 * NOTE: Setting the PTE_RDONLY (as a coincident) also
+	 * implies clearing the HW dirty state.
+	 */
+	return set_pte_bit(pte, __pgprot(PTE_RDONLY));
+}
+
 static inline pmd_t set_pmd_bit(pmd_t pmd, pgprot_t prot)
 {
 	pmd_val(pmd) |= pgprot_val(prot);
@@ -189,19 +223,17 @@ static inline pte_t pte_mkwrite(pte_t pte)
 
 static inline pte_t pte_mkclean(pte_t pte)
 {
-	pte = clear_pte_bit(pte, __pgprot(PTE_DIRTY));
-	pte = set_pte_bit(pte, __pgprot(PTE_RDONLY));
-
-	return pte;
+	/*
+	 * Subsequent call to pte_hw_clr_dirty() is not required
+	 * because pte_sw_clr_dirty() in turn does that as well.
+	 */
+	return pte_sw_clr_dirty(pte);
 }
 
 static inline pte_t pte_mkdirty(pte_t pte)
 {
-	pte = set_pte_bit(pte, __pgprot(PTE_DIRTY));
-
-	if (pte_write(pte))
-		pte = clear_pte_bit(pte, __pgprot(PTE_RDONLY));
-
+	pte = pte_sw_mkdirty(pte);
+	pte = pte_hw_mkdirty(pte);
 	return pte;
 }
 
-- 
2.30.2

[RFC 2/4] arm64/mm: Call pte_sw_mkdirty() while preserving the HW dirty state

[Anshuman Khandual]

pte_mkdirty() creates dirty states both in SW and HW bits which is really
not required either in pte_wrprotect() or pte_modify() for preserving the
HW dirty state. Instead pte_sw_mkdirty() is sufficient.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/pgtable.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index fb03be697819..dd20b752ed48 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -244,7 +244,7 @@ static inline pte_t pte_wrprotect(pte_t pte)
 	 * clear), set the PTE_DIRTY bit.
 	 */
 	if (pte_hw_dirty(pte))
-		pte = pte_mkdirty(pte);
+		pte = pte_sw_mkdirty(pte);
 
 	pte = clear_pte_bit(pte, __pgprot(PTE_WRITE));
 	pte = set_pte_bit(pte, __pgprot(PTE_RDONLY));
@@ -855,7 +855,7 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
 			      PTE_ATTRINDX_MASK;
 	/* preserve the hardware dirty information */
 	if (pte_hw_dirty(pte))
-		pte = pte_mkdirty(pte);
+		pte = pte_sw_mkdirty(pte);
 	pte_val(pte) = (pte_val(pte) & ~mask) | (pgprot_val(newprot) & mask);
 	return pte;
 }
-- 
2.30.2

[RFC 3/4] arm64/mm: Add pte_preserve_hw_dirty()

[Anshuman Khandual]

Preserving the HW dirty state via SW PTE dirty bit, should be made explicit
ensuring greater clarity and readability. This adds pte_preserve_hw_dirty()
helper for that effect. No functional change is intended.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/pgtable.h | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index dd20b752ed48..5344e71a58b2 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -237,7 +237,7 @@ static inline pte_t pte_mkdirty(pte_t pte)
 	return pte;
 }
 
-static inline pte_t pte_wrprotect(pte_t pte)
+static inline pte_t pte_preserve_hw_dirty(pte_t pte)
 {
 	/*
 	 * If hardware-dirty (PTE_WRITE/DBM bit set and PTE_RDONLY
@@ -246,6 +246,12 @@ static inline pte_t pte_wrprotect(pte_t pte)
 	if (pte_hw_dirty(pte))
 		pte = pte_sw_mkdirty(pte);
 
+	return pte;
+}
+
+static inline pte_t pte_wrprotect(pte_t pte)
+{
+	pte = pte_preserve_hw_dirty(pte);
 	pte = clear_pte_bit(pte, __pgprot(PTE_WRITE));
 	pte = set_pte_bit(pte, __pgprot(PTE_RDONLY));
 	return pte;
@@ -853,9 +859,7 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
 	const pteval_t mask = PTE_USER | PTE_PXN | PTE_UXN | PTE_RDONLY |
 			      PTE_PROT_NONE | PTE_VALID | PTE_WRITE | PTE_GP |
 			      PTE_ATTRINDX_MASK;
-	/* preserve the hardware dirty information */
-	if (pte_hw_dirty(pte))
-		pte = pte_sw_mkdirty(pte);
+	pte = pte_preserve_hw_dirty(pte);
 	pte_val(pte) = (pte_val(pte) & ~mask) | (pgprot_val(newprot) & mask);
 	return pte;
 }
-- 
2.30.2

[RFC 4/4] docs: arm64: Add help document for pte dirty state management

[Anshuman Khandual]

PTE dirty state management is non-trivial on arm64 platform. This document
explains how both software and hardware come together in correctly tracking
PTE ditry state across various page table transactions.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 Documentation/arch/arm64/index.rst     |  1 +
 Documentation/arch/arm64/pte-dirty.rst | 95 ++++++++++++++++++++++++++
 2 files changed, 96 insertions(+)
 create mode 100644 Documentation/arch/arm64/pte-dirty.rst

diff --git a/Documentation/arch/arm64/index.rst b/Documentation/arch/arm64/index.rst
index d08e924204bf..522f887f2a60 100644
--- a/Documentation/arch/arm64/index.rst
+++ b/Documentation/arch/arm64/index.rst
@@ -22,6 +22,7 @@ ARM64 Architecture
     perf
     pointer-authentication
     ptdump
+    pte-dirty
     silicon-errata
     sme
     sve
diff --git a/Documentation/arch/arm64/pte-dirty.rst b/Documentation/arch/arm64/pte-dirty.rst
new file mode 100644
index 000000000000..a6401696f6a3
--- /dev/null
+++ b/Documentation/arch/arm64/pte-dirty.rst
@@ -0,0 +1,95 @@
+.. SPDX-License-Identifier: GPL-2.0
+=========================================
+Page Table Entry - Dirty State Management
+=========================================
+
+1. Introduction
+---------------
+
+arm64 platform defines pte_dirty() to determine if the pte has been dirtied
+i.e pte has been written info after the previous clean procedure. The dirty
+state tracking could be achieved, either via software or hardware pte dirty
+bit mechanism. On arm64 platform, pte_dirty() is implemented utilizing both
+software and hardware dirty bits, making it non intuitive unlike many other
+platforms.
+
+2. PTE Dirty Bits (SW and HW)
+-----------------------------
+Following are relevant PTE bit positions for dirty state tracking.
+
+- PTE_DIRTY is a software bit (55) in the PTE
+- PTE_RDONLY is a hardware bit (7) in the PTE
+- PTE_DBM is a hardware bit (51) in the PTE
+- PTE_WRITE is a hardware bit (51) in the PTE - share position with PTE_DBM
+
+3. PTE Dirty State Tracking
+---------------------------
+Without ARM64_HW_AFDBM enabled, PTE dirty state is tracked only in the SW.
+PTE is marked read-only in HW, subsequent write access generates page fault
+which can update the SW dirty bit and clear the read-only access in HW.
+
+With ARM64_HW_AFDBM enabled, PTE dirty state is tracked both in SW and HW.
+PTE is marked read-only in HW while also enabling DBM tracking. Any write
+access will clear the read-only bit while also preventing a page fault. As
+PTE_DBM and PTE_WRITE share the same bit position, a dirty non-writable PTE
+state cannot be tracked in hardware. This in turn necessitates dirty state
+tracking (ARM64_HW_AFDBM enabled) to accommodate both software and hardware
+PTE bits. This helps in avoiding a runtime check for ARM64_HW_AFDBM feature
+being enabled on a given implementation.
+
+Testing and clearing PTE dirty state is relatively simple -
+
+#define pte_hw_dirty(pte)	(pte_write(pte) && !pte_rdonly(pte))
+#define pte_sw_dirty(pte)	(!!(pte_val(pte) & PTE_DIRTY))
+#define pte_dirty(pte)		(pte_sw_dirty(pte) || pte_hw_dirty(pte))
+
+static inline pte_t pte_mkclean(pte_t pte)
+{
+	/*
+	 * Subsequent call to pte_hw_clr_dirty() is not required
+	 * because pte_sw_clr_dirty() in turn does that as well.
+	 */
+	return pte_sw_clr_dirty(pte);
+}
+
+But marking a dirty state, creating a write protected entry etc now becomes
+bit non-trivial in hardware. as PTE_RDONLY bit could only be cleared if the
+write bit is also set.
+
+static inline pte_t pte_hw_mkdirty(pte_t pte)
+{
+	if (pte_write(pte))
+		return clear_pte_bit(pte, __pgprot(PTE_RDONLY));
+
+	return pte;
+}
+
+Hence marking a dirty state triggers marking both SW and HW dirty bits, so
+that if the HW suppoprt is unavailable or insufficient (dirty non-writable)
+, SW mechanism would still put it in a dirty state.
+
+static inline pte_t pte_mkdirty(pte_t pte)
+{
+	pte = pte_sw_mkdirty(pte);
+	pte = pte_hw_mkdirty(pte);
+	return pte;
+}
+
+4. Preserving PTE HW Dirty State
+--------------------------------
+If for some reason HW dirty bits (PTE_WRITE, PTE_RDONLY) need to be cleared
+the dirty state must be transferred as SW dirty bit ensuring persistence of
+the dirty state across the operation.
+
+static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
+{
+        .....
+        pte = pte_preserve_hw_dirty(pte_t pte);
+        .....
+}
+
+static inline pte_t pte_wrprotect(pte_t pte)
+{
+	pte = pte_preserve_hw_dirty(pte_t pte);
+        .....
+}
-- 
2.30.2

Re: [RFC 1/4] arm64/mm: Add SW and HW dirty state helpers

[David Hildenbrand]

On 07.07.23 07:33, Anshuman Khandual wrote:
> This factors out low level SW and HW state changes i.e make and clear into
> separate helpers making them explicit improving readability. This also adds
> pte_rdonly() helper as well. No functional change is intended.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>   arch/arm64/include/asm/pgtable.h | 52 ++++++++++++++++++++++++++------
>   1 file changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 0bd18de9fd97..fb03be697819 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -103,6 +103,7 @@ static inline pteval_t __phys_to_pte_val(phys_addr_t phys)
>   #define pte_young(pte)		(!!(pte_val(pte) & PTE_AF))
>   #define pte_special(pte)	(!!(pte_val(pte) & PTE_SPECIAL))
>   #define pte_write(pte)		(!!(pte_val(pte) & PTE_WRITE))
> +#define pte_rdonly(pte)		(!!(pte_val(pte) & PTE_RDONLY))
>   #define pte_user(pte)		(!!(pte_val(pte) & PTE_USER))
>   #define pte_user_exec(pte)	(!(pte_val(pte) & PTE_UXN))
>   #define pte_cont(pte)		(!!(pte_val(pte) & PTE_CONT))
> @@ -120,7 +121,7 @@ static inline pteval_t __phys_to_pte_val(phys_addr_t phys)
>   	(__boundary - 1 < (end) - 1) ? __boundary : (end);			\
>   })
>   
> -#define pte_hw_dirty(pte)	(pte_write(pte) && !(pte_val(pte) & PTE_RDONLY))
> +#define pte_hw_dirty(pte)	(pte_write(pte) && !pte_rdonly(pte))
>   #define pte_sw_dirty(pte)	(!!(pte_val(pte) & PTE_DIRTY))
>   #define pte_dirty(pte)		(pte_sw_dirty(pte) || pte_hw_dirty(pte))
>   
> @@ -174,6 +175,39 @@ static inline pmd_t clear_pmd_bit(pmd_t pmd, pgprot_t prot)
>   	return pmd;
>   }
>   
> +static inline pte_t pte_hw_mkdirty(pte_t pte)

I'd have called this "pte_mkhw_dirty", similar to "pte_mksoft_dirty".

> +{
> +	if (pte_write(pte))
> +		pte = clear_pte_bit(pte, __pgprot(PTE_RDONLY));
> +
> +	return pte;
> +}
> +
> +static inline pte_t pte_sw_mkdirty(pte_t pte)

pte_mksw_dirty

> +{
> +	return set_pte_bit(pte, __pgprot(PTE_DIRTY));
> +}
> +
> +static inline __always_unused pte_t pte_hw_clr_dirty(pte_t pte)

pte_clear_hw_dirty (again, similar to pte_clear_soft_dirty )

> +{
> +	return set_pte_bit(pte, __pgprot(PTE_RDONLY));
> +}
> +
> +static inline pte_t pte_sw_clr_dirty(pte_t pte)

pte_clear_sw_dirty

> +{
> +	pte = clear_pte_bit(pte, __pgprot(PTE_DIRTY));
> +
> +	/*
> +	 * Clearing the software dirty state requires clearing
> +	 * the PTE_DIRTY bit along with setting the PTE_RDONLY
> +	 * ensuring a page fault on subsequent write access.
> +	 *
> +	 * NOTE: Setting the PTE_RDONLY (as a coincident) also
> +	 * implies clearing the HW dirty state.
> +	 */
> +	return set_pte_bit(pte, __pgprot(PTE_RDONLY));
> +}
> +
>   static inline pmd_t set_pmd_bit(pmd_t pmd, pgprot_t prot)
>   {
>   	pmd_val(pmd) |= pgprot_val(prot);
> @@ -189,19 +223,17 @@ static inline pte_t pte_mkwrite(pte_t pte)
>   
>   static inline pte_t pte_mkclean(pte_t pte)
>   {
> -	pte = clear_pte_bit(pte, __pgprot(PTE_DIRTY));
> -	pte = set_pte_bit(pte, __pgprot(PTE_RDONLY));
> -
> -	return pte;
> +	/*
> +	 * Subsequent call to pte_hw_clr_dirty() is not required
> +	 * because pte_sw_clr_dirty() in turn does that as well.
> +	 */
> +	return pte_sw_clr_dirty(pte);

Hm, I'm not sure if that simplifies things.

You call pte_sw_clr_dirty() and suddenly your hw dirty bit is clear?

In that case I think the current implementation is clearer: it doesn't 
provide primitives that don't make any sense.

>   }
>   
>   static inline pte_t pte_mkdirty(pte_t pte)
>   {
> -	pte = set_pte_bit(pte, __pgprot(PTE_DIRTY));
> -
> -	if (pte_write(pte))
> -		pte = clear_pte_bit(pte, __pgprot(PTE_RDONLY));
> -
> +	pte = pte_sw_mkdirty(pte);
> +	pte = pte_hw_mkdirty(pte);

That looks weird. Especially, pte_hw_mkdirty() only does something if 
pte_write().

Shouldn't pte_hw_mkdirty() bail out if it cannot do anything reasonable 
(IOW, !writable)?

>   	return pte;
>   }
>   

-- 
Cheers,

David / dhildenb

Re: [RFC 0/4] arm64/mm: Clean up pte_dirty() state management

[David Hildenbrand]

On 07.07.23 07:33, Anshuman Khandual wrote:
> These pte_dirty() changes make things explicitly clear, while improving the
> code readability. This optimizes HW dirty state transfer into SW dirty bit.
> This also adds a new arm64 documentation explaining overall pte dirty state
> management in detail. This series applies on the latest mainline kernel.
> 
>

I skimmed over most of the series, and I am not convinced that this is 
actually a cleanup. If we cannot really always differentiate between 
sw/hw clearing, why have separate primitives that give one the illusion 
that it could be done and that they are two different concepts?

Maybe there are other opinions, but at least for me this made the code 
harder to grasp.

I do appreciate a doc update, though :)

-- 
Cheers,

David / dhildenb

Re: [RFC 0/4] arm64/mm: Clean up pte_dirty() state management

[Anshuman Khandual]

On 7/7/23 17:41, David Hildenbrand wrote:
> On 07.07.23 07:33, Anshuman Khandual wrote:
>> These pte_dirty() changes make things explicitly clear, while improving the
>> code readability. This optimizes HW dirty state transfer into SW dirty bit.
>> This also adds a new arm64 documentation explaining overall pte dirty state
>> management in detail. This series applies on the latest mainline kernel.
>>
>>
> 
> I skimmed over most of the series, and I am not convinced that this is actually a cleanup. If we cannot really always differentiate between sw/hw clearing, why have separate primitives that give one the illusion that it could be done and that they are two different concepts?

These are indeed two different concepts working together, the current code just
obscures that. Without these primitives it's even hard to follow how the SW and
HW dirty parts are intertwined in implementing the generic pte_dirty() state.

The current code acknowledges these two different concepts in identifying them
i.e via pte_hw_dirty() and pte_sw_dirty().

#define pte_hw_dirty(pte)       (pte_write(pte) && !(pte_val(pte) & PTE_RDONLY))
#define pte_sw_dirty(pte)       (!!(pte_val(pte) & PTE_DIRTY))

But then falls short in demonstrating how these two states are being managed i.e
created and cleared. The first patch tries to clear the situation to some extent.

> 
> Maybe there are other opinions, but at least for me this made the code harder to grasp.
> 
> I do appreciate a doc update, though :)
These two changes also streamline HW dirty bit saving in SW dirty bit efficiently,
skipping the re-doing of HW dirty bits setting which might be cleared off. That is
the primary reason for saving it off in SW dirty bit in the first place.

  arm64/mm: Call pte_sw_mkdirty() while preserving the HW dirty state
  arm64/mm: Add pte_preserve_hw_dirty(

Regardless, I am still trying to understand how the first patch does not improve
the clarity in modifying the PTE dirty state.

Re: [RFC 1/4] arm64/mm: Add SW and HW dirty state helpers

[Anshuman Khandual]

On 7/7/23 17:39, David Hildenbrand wrote:
> On 07.07.23 07:33, Anshuman Khandual wrote:
>> This factors out low level SW and HW state changes i.e make and clear into
>> separate helpers making them explicit improving readability. This also adds
>> pte_rdonly() helper as well. No functional change is intended.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>   arch/arm64/include/asm/pgtable.h | 52 ++++++++++++++++++++++++++------
>>   1 file changed, 42 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index 0bd18de9fd97..fb03be697819 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -103,6 +103,7 @@ static inline pteval_t __phys_to_pte_val(phys_addr_t phys)
>>   #define pte_young(pte)        (!!(pte_val(pte) & PTE_AF))
>>   #define pte_special(pte)    (!!(pte_val(pte) & PTE_SPECIAL))
>>   #define pte_write(pte)        (!!(pte_val(pte) & PTE_WRITE))
>> +#define pte_rdonly(pte)        (!!(pte_val(pte) & PTE_RDONLY))
>>   #define pte_user(pte)        (!!(pte_val(pte) & PTE_USER))
>>   #define pte_user_exec(pte)    (!(pte_val(pte) & PTE_UXN))
>>   #define pte_cont(pte)        (!!(pte_val(pte) & PTE_CONT))
>> @@ -120,7 +121,7 @@ static inline pteval_t __phys_to_pte_val(phys_addr_t phys)
>>       (__boundary - 1 < (end) - 1) ? __boundary : (end);            \
>>   })
>>   -#define pte_hw_dirty(pte)    (pte_write(pte) && !(pte_val(pte) & PTE_RDONLY))
>> +#define pte_hw_dirty(pte)    (pte_write(pte) && !pte_rdonly(pte))
>>   #define pte_sw_dirty(pte)    (!!(pte_val(pte) & PTE_DIRTY))
>>   #define pte_dirty(pte)        (pte_sw_dirty(pte) || pte_hw_dirty(pte))
>>   @@ -174,6 +175,39 @@ static inline pmd_t clear_pmd_bit(pmd_t pmd, pgprot_t prot)
>>       return pmd;
>>   }
>>   +static inline pte_t pte_hw_mkdirty(pte_t pte)
> 
> I'd have called this "pte_mkhw_dirty", similar to "pte_mksoft_dirty".
> 
>> +{
>> +    if (pte_write(pte))
>> +        pte = clear_pte_bit(pte, __pgprot(PTE_RDONLY));
>> +
>> +    return pte;
>> +}
>> +
>> +static inline pte_t pte_sw_mkdirty(pte_t pte)
> 
> pte_mksw_dirty

Sure, will change them as pte_mkhw_dirty()/pte_mksw_dirty() instead.

> 
>> +{
>> +    return set_pte_bit(pte, __pgprot(PTE_DIRTY));
>> +}
>> +
>> +static inline __always_unused pte_t pte_hw_clr_dirty(pte_t pte)
> 
> pte_clear_hw_dirty (again, similar to pte_clear_soft_dirty )
> 
>> +{
>> +    return set_pte_bit(pte, __pgprot(PTE_RDONLY));
>> +}
>> +
>> +static inline pte_t pte_sw_clr_dirty(pte_t pte)
> 
> pte_clear_sw_dirty

Sure, will change them as pte_clear_hw_dirty()/pte_clear_sw_dirty() instead.

> 
>> +{
>> +    pte = clear_pte_bit(pte, __pgprot(PTE_DIRTY));
>> +
>> +    /*
>> +     * Clearing the software dirty state requires clearing
>> +     * the PTE_DIRTY bit along with setting the PTE_RDONLY
>> +     * ensuring a page fault on subsequent write access.
>> +     *
>> +     * NOTE: Setting the PTE_RDONLY (as a coincident) also
>> +     * implies clearing the HW dirty state.
>> +     */
>> +    return set_pte_bit(pte, __pgprot(PTE_RDONLY));
>> +}
>> +
>>   static inline pmd_t set_pmd_bit(pmd_t pmd, pgprot_t prot)
>>   {
>>       pmd_val(pmd) |= pgprot_val(prot);
>> @@ -189,19 +223,17 @@ static inline pte_t pte_mkwrite(pte_t pte)
>>     static inline pte_t pte_mkclean(pte_t pte)
>>   {
>> -    pte = clear_pte_bit(pte, __pgprot(PTE_DIRTY));
>> -    pte = set_pte_bit(pte, __pgprot(PTE_RDONLY));
>> -
>> -    return pte;
>> +    /*
>> +     * Subsequent call to pte_hw_clr_dirty() is not required
>> +     * because pte_sw_clr_dirty() in turn does that as well.
>> +     */
>> +    return pte_sw_clr_dirty(pte);
> 
> Hm, I'm not sure if that simplifies things.
> 
> You call pte_sw_clr_dirty() and suddenly your hw dirty bit is clear?

Because clearing HW dirty bit just needs setting PTE_RDONLY bit, which as
a coincidence is also required, after clearing the SW dirty bit to enable
a subsequent write fault. Here pte_sw_clr_dirty() just happen to contain
pte_hw_clr_dirty().

> 
> In that case I think the current implementation is clearer: it doesn't provide primitives that don't make any sense.

It actually does a SW dirty bit clearing which also takes care of HW dirty
bit clearing without saying so explicitly. These new helpers demonstrate
bit clearly what is happening.

> 
>>   }
>>     static inline pte_t pte_mkdirty(pte_t pte)
>>   {
>> -    pte = set_pte_bit(pte, __pgprot(PTE_DIRTY));
>> -
>> -    if (pte_write(pte))
>> -        pte = clear_pte_bit(pte, __pgprot(PTE_RDONLY));
>> -
>> +    pte = pte_sw_mkdirty(pte);
>> +    pte = pte_hw_mkdirty(pte);
> 
> That looks weird. Especially, pte_hw_mkdirty() only does something if pte_write().

pte_write() check asserts if DBM is implemented and being used before clearing
PTE_RDONLY making it a HW dirty state. If pte_write() is cleared, either DBM
is not implemented or it's a non-writable entry, either way dirty state cannot
be tracked in HW.

> 
> Shouldn't pte_hw_mkdirty() bail out if it cannot do anything reasonable (IOW, !writable)?

static inline pte_t pte_hw_mkdirty(pte_t pte)
{
	if (pte_write(pte))
		pte = clear_pte_bit(pte, __pgprot(PTE_RDONLY));

	return pte;
}

If pte_write() is not positive, it's in !writable state on DBM enabled systems.
Otherwise pte_write() state does not matter, as the bit position does not make
sense on non DBM enabled systems.

> 
>>       return pte;
>>   }
>>   
> 

Re: [RFC 0/4] arm64/mm: Clean up pte_dirty() state management

[David Hildenbrand]

On 10.07.23 04:20, Anshuman Khandual wrote:
> 
> 
> On 7/7/23 17:41, David Hildenbrand wrote:
>> On 07.07.23 07:33, Anshuman Khandual wrote:
>>> These pte_dirty() changes make things explicitly clear, while improving the
>>> code readability. This optimizes HW dirty state transfer into SW dirty bit.
>>> This also adds a new arm64 documentation explaining overall pte dirty state
>>> management in detail. This series applies on the latest mainline kernel.
>>>
>>>
>>
>> I skimmed over most of the series, and I am not convinced that this is actually a cleanup. If we cannot really always differentiate between sw/hw clearing, why have separate primitives that give one the illusion that it could be done and that they are two different concepts?
> 
> These are indeed two different concepts working together, the current code just
> obscures that. Without these primitives it's even hard to follow how the SW and
> HW dirty parts are intertwined in implementing the generic pte_dirty() state.
> 
> The current code acknowledges these two different concepts in identifying them
> i.e via pte_hw_dirty() and pte_sw_dirty().
> 
> #define pte_hw_dirty(pte)       (pte_write(pte) && !(pte_val(pte) & PTE_RDONLY))
> #define pte_sw_dirty(pte)       (!!(pte_val(pte) & PTE_DIRTY))
> 

^ these primitives make sense to me, but not the clearing part.

If there is only one way to clear both, then only have one primitive to 
clear both and state there, that separate clearing is impossible because 
both are intertwined.

-- 
Cheers,

David / dhildenb

Re: [RFC 0/4] arm64/mm: Clean up pte_dirty() state management

[Mark Rutland]

On Fri, Jul 07, 2023 at 11:03:27AM +0530, Anshuman Khandual wrote:
> These pte_dirty() changes make things explicitly clear, while improving the
> code readability. This optimizes HW dirty state transfer into SW dirty bit.
> This also adds a new arm64 documentation explaining overall pte dirty state
> management in detail. This series applies on the latest mainline kernel.

TBH, I think this is all swings and roundabouts, and I'm not sure this is
worthwhile. I appreciate that as-is some people find this confusing, but I
don't think the end result of this series is actually better, and it adds more
code/documentation to maintain.

In particular, I don't think that we should add Documentation/ files for this,
as it's very likely that won't be updated together with the code, and I think
it's more of a maintenance burden than a help. If we want some introductory
text to explain how the HW/SW dirty bits work, I think that should be a comment
block in <asm/pgtable.h>, clearly associated with the code.

Overall, I'd prefer to leave the code as-is.

Thanks,
Mark.

> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Ryan Roberts <ryan.roberts@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> 
> Anshuman Khandual (4):
>   arm64/mm: Add SW and HW dirty state helpers
>   arm64/mm: Call pte_sw_mkdirty() while preserving the HW dirty state
>   arm64/mm: Add pte_preserve_hw_dirty()
>   docs: arm64: Add help document for pte dirty state management
> 
>  Documentation/arch/arm64/index.rst     |  1 +
>  Documentation/arch/arm64/pte-dirty.rst | 95 ++++++++++++++++++++++++++
>  arch/arm64/include/asm/pgtable.h       | 66 ++++++++++++++----
>  3 files changed, 147 insertions(+), 15 deletions(-)
>  create mode 100644 Documentation/arch/arm64/pte-dirty.rst
> 
> -- 
> 2.30.2
> 

Re: [RFC 0/4] arm64/mm: Clean up pte_dirty() state management

[Anshuman Khandual]

On 7/10/23 16:55, Mark Rutland wrote:
> On Fri, Jul 07, 2023 at 11:03:27AM +0530, Anshuman Khandual wrote:
>> These pte_dirty() changes make things explicitly clear, while improving the
>> code readability. This optimizes HW dirty state transfer into SW dirty bit.
>> This also adds a new arm64 documentation explaining overall pte dirty state
>> management in detail. This series applies on the latest mainline kernel.
> TBH, I think this is all swings and roundabouts, and I'm not sure this is
> worthwhile. I appreciate that as-is some people find this confusing, but I

Current situation for pte_dirty() management is confusing when there are two
distinct mechanisms to track PTE dirty states, but both are forced to work
together because

- HW DBM cannot track non-writable dirty state (PTE_DBM == PTE_WRITE)
- Runtime check for HW DBM is avoided

> don't think the end result of this series is actually better, and it adds more
> code/documentation to maintain.

Agreed, it does add more code and documentation but still trying to understand
why it is not worthwhile. Regardless, following patch does optimize a situation
where we dont need to call pte_mkdirty() knowing it will be cleared afterwards.

[RFC 2/4] arm64/mm: Call pte_sw_mkdirty() while preserving the HW dirty state

> 
> In particular, I don't think that we should add Documentation/ files for this,
> as it's very likely that won't be updated together with the code, and I think
> it's more of a maintenance burden than a help. If we want some introductory

There are many documentation files such as Documentation/arm64/memory.rst which
needs to be updated when kernel virtual address layout changes again. I am just
wondering - should not there be any documentation for internal implementation
details, just because they need updating with code changes.

> text to explain how the HW/SW dirty bits work, I think that should be a comment
> block in <asm/pgtable.h>, clearly associated with the code.

Sure, will add that.

> 
> Overall, I'd prefer to leave the code as-is.
Even if we discount individual dirty clearing functions, why should not HW dirty
bit transfer to SW dirty be optimized and wrapped around in a helper.

Re: [RFC 0/4] arm64/mm: Clean up pte_dirty() state management

[Catalin Marinas]

On Wed, Jul 12, 2023 at 09:31:39AM +0530, Anshuman Khandual wrote:
> On 7/10/23 16:55, Mark Rutland wrote:
> > On Fri, Jul 07, 2023 at 11:03:27AM +0530, Anshuman Khandual wrote:
> >> These pte_dirty() changes make things explicitly clear, while improving the
> >> code readability. This optimizes HW dirty state transfer into SW dirty bit.
> >> This also adds a new arm64 documentation explaining overall pte dirty state
> >> management in detail. This series applies on the latest mainline kernel.
> > 
> > TBH, I think this is all swings and roundabouts, and I'm not sure this is
> > worthwhile. I appreciate that as-is some people find this confusing, but I

I'm pretty much on the same lines, though maybe I looked too much at
this code that I don't like any further changes to it ;).

> Current situation for pte_dirty() management is confusing when there are two
> distinct mechanisms to track PTE dirty states, but both are forced to work
> together because
> 
> - HW DBM cannot track non-writable dirty state (PTE_DBM == PTE_WRITE)
> - Runtime check for HW DBM is avoided

Depending on how you look at it, we can say that any writeable PTE (as
in page table permission, PTE_RDONLY cleared) is dirty and we only have
a software mechanism for tracking the dirty state. The DBM feature is
not actually giving us a dirty bit but an automated way to make a PTE
writeable on access (for some historical reasons like the SMMU not
having such mechanism in place).

Maybe we can clean the code a bit based on the above perspective. E.g.
instead of pte_hw_dirty() just have a !pte_hw_rdonly() macro. It may
help with the confusion of having two mechanisms.

OTOH, with PIE, we can have a true dirty bit but at that point we can
eliminate the pte_sw_dirty() use entirely and allow soft-dirty using the
current PTE_DIRTY (with some static labels based on the feature).

> > don't think the end result of this series is actually better, and it adds more
> > code/documentation to maintain.
> 
> Agreed, it does add more code and documentation but still trying to understand
> why it is not worthwhile. Regardless, following patch does optimize a situation
> where we dont need to call pte_mkdirty() knowing it will be cleared afterwards.
> 
> [RFC 2/4] arm64/mm: Call pte_sw_mkdirty() while preserving the HW dirty state

I wonder whether the compiler eliminates much of this duplication since
there are some checks for pte_write() before. We may be able to remove
some checks. For example, does pte_hw_dirty() actually need to check
pte_write()? A !PTE_RDONLY entry is dirty automatically since we can't
trap any write access to it (prior to PIE; I need to check Joey's
patches on how it treats writeable+clean PTEs; still on holiday).

As for the fourth patch, I'd rather add documentation in the header
file, it's more likely to be looked at and updated.

-- 
Catalin