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@xxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> ---
>   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