Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230703135526.930004-1-npache@redhat.com> <c8bbc891-cd52-e34a-8ca7-76314e97cbe3@arm.com>
In-Reply-To: <c8bbc891-cd52-e34a-8ca7-76314e97cbe3@arm.com>
From:   Nico Pache <npache@redhat.com>
Date:   Tue, 4 Jul 2023 06:08:59 -0400
Message-ID: <CAA1CXcBWuMgMbBTLj9eYzW4wBxbJpa3FGZsbtiibrYODZQdg6A@mail.gmail.com>
Subject: Re: [RFC] arm64: properly define SOFT_DIRTY for arm64
To:     Anshuman Khandual <anshuman.khandual@arm.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        aquini@redhat.com, Andrew Morton <akpm@linux-foundation.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        David Hildenbrand <david@redhat.com>,
        Gerald Schaefer <gerald.schaefer@linux.ibm.com>,
        Liu Shixin <liushixin2@huawei.com>,
        Will Deacon <will@kernel.org>, Yu Zhao <yuzhao@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Anshuman,

Thanks for the explanation!

Is it possible to add the same DBM check I'm using
(!arch_has_hw_pte_young) in these pte helper functions to only clear
it when DBM is not present?

Cheers,
-- Nico

On Tue, Jul 4, 2023 at 6:01=E2=80=AFAM Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
> Hi Nico,
>
> On 7/3/23 19:25, Nico Pache wrote:
> > ARM64 has a soft-dirty bit (software dirty) but never properly defines
> > CONFIG_ARCH_HAS_SOFT_DIRTY or its necessary functions. This patch
> > introduces the ability to set/clear the soft dirty bit in a similar
> > manner as the other arches that utilize it.
> >
> > However, we must be careful... there are cases where the DBM bit is not
> > available and the software dirty bit plays a essential role in determin=
ing
> > whether or not a page is dirty. In these cases we must not allow the
> > user to clear the software dirty bit. We can test for these cases by
> > utilizing the arch_faults_on_old_pte() function which test the availabi=
lity
> > of DBM.
>
> The current soft-dirty bit is a SW PTE bit i.e PTE_DIRTY, tracking PTE
> dirtiness in absence of HW DBM support, although both these tracking
> methods are very much intertwined. Current pte helpers like pte_dirty(),
> pte_mkdirty(), pte_mkclean(), and pte_wrrotect() etc operate both on
> HW and SW dirty tracking bits irrespective of whether DBM is supported
> or not.
>
> For soft dirty to work, we need a software PTE bit which sticks around
> on the PTE entry until user space (only) clears it and above PTE_DIRTY
> bit cannot be used for that purpose as it could be cleared in the kernel.
>
> static inline pte_t pte_mkclean(pte_t pte)
> {
>         pte =3D clear_pte_bit(pte, __pgprot(PTE_DIRTY));
>         pte =3D set_pte_bit(pte, __pgprot(PTE_RDONLY));
>
>         return pte;
> }
>
> BTW arch_faults_on_old_pte() is no longer available mainline.
>
> - Anshuman
>
> >
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: David Hildenbrand <david@redhat.com>
> > Cc: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
> > Cc: Liu Shixin <liushixin2@huawei.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Yu Zhao <yuzhao@google.com>
> > Signed-off-by: Nico Pache <npache@redhat.com>
> > ---
> >  arch/arm64/Kconfig               |  1 +
> >  arch/arm64/include/asm/pgtable.h | 77 +++++++++++++++++++++++++++++++-
> >  2 files changed, 76 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 891ab530a665..4de491627f49 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -173,6 +173,7 @@ config ARM64
> >       select HAVE_ARCH_PREL32_RELOCATIONS
> >       select HAVE_ARCH_RANDOMIZE_KSTACK_OFFSET
> >       select HAVE_ARCH_SECCOMP_FILTER
> > +     select HAVE_ARCH_SOFT_DIRTY
> >       select HAVE_ARCH_STACKLEAK
> >       select HAVE_ARCH_THREAD_STRUCT_WHITELIST
> >       select HAVE_ARCH_TRACEHOOK
> > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/=
pgtable.h
> > index 0bd18de9fd97..a0a15ffa2417 100644
> > --- a/arch/arm64/include/asm/pgtable.h
> > +++ b/arch/arm64/include/asm/pgtable.h
> > @@ -121,8 +121,9 @@ static inline pteval_t __phys_to_pte_val(phys_addr_=
t phys)
> >  })
> >
> >  #define pte_hw_dirty(pte)    (pte_write(pte) && !(pte_val(pte) & PTE_R=
DONLY))
> > -#define pte_sw_dirty(pte)    (!!(pte_val(pte) & PTE_DIRTY))
> > -#define pte_dirty(pte)               (pte_sw_dirty(pte) || pte_hw_dirt=
y(pte))
> > +#define pte_soft_dirty(pte)  (!!(pte_val(pte) & PTE_DIRTY))
> > +#define pte_dirty(pte)               (pte_soft_dirty(pte) || pte_hw_di=
rty(pte))
> > +#define pte_swp_soft_dirty(pte)      pte_soft_dirty(pte)
> >
> >  #define pte_valid(pte)               (!!(pte_val(pte) & PTE_VALID))
> >  /*
> > @@ -1096,6 +1097,78 @@ static inline bool pud_sect_supported(void)
> >       return PAGE_SIZE =3D=3D SZ_4K;
> >  }
> >
> > +#ifdef CONFIG_ARM64_HW_AFDBM
> > +/*
> > + * if we have the DBM bit we can utilize the software dirty bit as
> > + * a mechanism to introduce the soft_dirty functionality; however, wit=
hout
> > + * it this bit is crucial to determining if a entry is dirty and we ca=
nnot
> > + * clear it via software. DBM can also be disabled or broken on some e=
arly
> > + * armv8 devices, so check its availability before modifying it.
> > + */
> > +static inline pte_t pte_clear_soft_dirty(pte_t pte)
> > +{
> > +     if (arch_faults_on_old_pte())
> > +             return pte;
> > +
> > +     return clear_pte_bit(pte, __pgprot(PTE_DIRTY));
> > +}
> > +
> > +static inline pte_t pte_mksoft_dirty(pte_t pte)
> > +{
> > +     if (arch_faults_on_old_pte())
> > +             return pte;
> > +
> > +     return set_pte_bit(pte, __pgprot(PTE_DIRTY));
> > +}
> > +
> > +static inline pte_t pte_swp_clear_soft_dirty(pte_t pte)
> > +{
> > +     if (arch_faults_on_old_pte())
> > +             return pte;
> > +
> > +     return clear_pte_bit(pte, __pgprot(PTE_DIRTY));
> > +}
> > +
> > +static inline pte_t pte_swp_mksoft_dirty(pte_t pte)
> > +{
> > +     if (arch_faults_on_old_pte())
> > +             return pte;
> > +
> > +     return set_pte_bit(pte, __pgprot(PTE_DIRTY));
> > +}
> > +
> > +static inline int pmd_soft_dirty(pmd_t pmd)
> > +{
> > +     return pte_soft_dirty(pmd_pte(pmd));
> > +}
> > +
> > +static inline pmd_t pmd_clear_soft_dirty(pmd_t pmd)
> > +{
> > +     return pte_pmd(pte_clear_soft_dirty(pmd_pte(pmd)));
> > +}
> > +
> > +static inline pmd_t pmd_mksoft_dirty(pmd_t pmd)
> > +{
> > +     return pte_pmd(pte_mksoft_dirty(pmd_pte(pmd)));
> > +}
> > +
> > +#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > +static inline int pmd_swp_soft_dirty(pmd_t pmd)
> > +{
> > +     return pmd_soft_dirty(pmd);
> > +}
> > +
> > +static inline pmd_t pmd_swp_clear_soft_dirty(pmd_t pmd)
> > +{
> > +     return pmd_clear_soft_dirty(pmd);
> > +}
> > +
> > +static inline pmd_t pmd_swp_mksoft_dirty(pmd_t pmd)
> > +{
> > +     return pmd_mksoft_dirty(pmd);
> > +}
> > +#endif /* CONFIG_ARCH_ENABLE_THP_MIGRATION */
> > +#endif /* CONFIG_ARM64_HW_AFDBM */
> >
> >  #define __HAVE_ARCH_PTEP_MODIFY_PROT_TRANSACTION
> >  #define ptep_modify_prot_start ptep_modify_prot_start
>