| Message ID | 20200616093553.27512-4-zhukeqian1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Support stage2 hardware DBM | expand |
Hi, On 16/06/2020 10:35, Keqian Zhu wrote: > kvm_set_pte is called to replace a target PTE with a desired one. > We always do this without changing the desired one, but if dirty > status set by hardware is coverred, let caller know it. > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > --- > arch/arm64/kvm/mmu.c | 36 +++++++++++++++++++++++++++++++++++- > 1 file changed, 35 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 5ad87bce23c0..27407153121b 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -194,11 +194,45 @@ static void clear_stage2_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr > put_page(virt_to_page(pmd)); > } > > -static inline void kvm_set_pte(pte_t *ptep, pte_t new_pte) > +#ifdef CONFIG_ARM64_HW_AFDBM > +/** > + * @ret: true if dirty status set by hardware is coverred. NIT: s/coverred/covered/, this is in several places. > + */ > +static bool kvm_set_pte(pte_t *ptep, pte_t new_pte) > +{ > + pteval_t old_pteval, new_pteval, pteval; > + bool old_logging, new_no_write; > + > + old_logging = kvm_hw_dbm_enabled() && !pte_none(*ptep) && > + kvm_s2pte_dbm(ptep); > + new_no_write = pte_none(new_pte) || kvm_s2pte_readonly(&new_pte); > + > + if (!old_logging || !new_no_write) { > + WRITE_ONCE(*ptep, new_pte); > + dsb(ishst); > + return false; > + } > + > + new_pteval = pte_val(new_pte); > + pteval = READ_ONCE(pte_val(*ptep)); This usage of *ptep looks wrong - it's read twice using READ_ONCE (once in kvm_s2pte_dbm()) and once without any decoration (in the pte_none() call). Which looks a bit dodgy and at the very least needs some justification. AFAICT you would be better taking a local copy and using that rather than reading from memory repeatedly. > + do { > + old_pteval = pteval; > + pteval = cmpxchg_relaxed(&pte_val(*ptep), old_pteval, new_pteval); > + } while (pteval != old_pteval); This look appears to be reinventing xchg_relaxed(). Any reason we can't just use xchg_relaxed()? Also we had a dsb() after the WRITE_ONCE but you are using the _relaxed variant here. What is the justification for not having a barrier? > + > + return !kvm_s2pte_readonly(&__pte(pteval)); > +} > +#else > +/** > + * @ret: true if dirty status set by hardware is coverred. > + */ > +static inline bool kvm_set_pte(pte_t *ptep, pte_t new_pte) > { > WRITE_ONCE(*ptep, new_pte); > dsb(ishst); > + return false; > } > +#endif /* CONFIG_ARM64_HW_AFDBM */ You might be able to avoid this #ifdef by redefining old_logging as: old_logging = IS_ENABLED(CONFIG_ARM64_HW_AFDBM) && ... I *think* the compiler should be able to kill the dead code and leave you with just the above when the config symbol is off. Steve > > static inline void kvm_set_pmd(pmd_t *pmdp, pmd_t new_pmd) > { >
Hi Steven, On 2020/7/1 19:28, Steven Price wrote: > Hi, > > On 16/06/2020 10:35, Keqian Zhu wrote: >> kvm_set_pte is called to replace a target PTE with a desired one. >> We always do this without changing the desired one, but if dirty >> status set by hardware is coverred, let caller know it. >> >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >> --- >> arch/arm64/kvm/mmu.c | 36 +++++++++++++++++++++++++++++++++++- >> 1 file changed, 35 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c >> index 5ad87bce23c0..27407153121b 100644 >> --- a/arch/arm64/kvm/mmu.c >> +++ b/arch/arm64/kvm/mmu.c >> @@ -194,11 +194,45 @@ static void clear_stage2_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr >> put_page(virt_to_page(pmd)); >> } >> -static inline void kvm_set_pte(pte_t *ptep, pte_t new_pte) >> +#ifdef CONFIG_ARM64_HW_AFDBM >> +/** >> + * @ret: true if dirty status set by hardware is coverred. > > NIT: s/coverred/covered/, this is in several places. > OK. >> + */ >> +static bool kvm_set_pte(pte_t *ptep, pte_t new_pte) >> +{ >> + pteval_t old_pteval, new_pteval, pteval; >> + bool old_logging, new_no_write; >> + >> + old_logging = kvm_hw_dbm_enabled() && !pte_none(*ptep) && >> + kvm_s2pte_dbm(ptep); >> + new_no_write = pte_none(new_pte) || kvm_s2pte_readonly(&new_pte); >> + >> + if (!old_logging || !new_no_write) { >> + WRITE_ONCE(*ptep, new_pte); >> + dsb(ishst); >> + return false; >> + } >> + >> + new_pteval = pte_val(new_pte); >> + pteval = READ_ONCE(pte_val(*ptep)); > > This usage of *ptep looks wrong - it's read twice using READ_ONCE (once in kvm_s2pte_dbm()) and once without any decoration (in the pte_none() call). Which looks a bit dodgy and at the very least needs some justification. AFAICT you would be better taking a local copy and using that rather than reading from memory repeatedly. > oh yes. Here we can use a local copy to get higher performance. I am not sure here has problem about correctness. I *think* we should always acquire corresponding lock before manipulate PTs, so there is no other agent will update PTs during our several PTs access (except MMU, but it just modifies AF and [S2]AP) . But do I miss something? >> + do { >> + old_pteval = pteval; >> + pteval = cmpxchg_relaxed(&pte_val(*ptep), old_pteval, new_pteval); >> + } while (pteval != old_pteval); > This look appears to be reinventing xchg_relaxed(). Any reason we can't just use xchg_relaxed()? Also we had a dsb() after the WRITE_ONCE but you are using the _relaxed variant here. What is the justification for not having a barrier? > Aha, I have changed the code for several times and it is equal to xchg_relaxed now, thanks. I use _relaxd here because I am not clear about the reason that we use _relaxed in kvm_set_s2XXX_XXX and use dsb() in kvm_set_pte. But I will correct this in patch v2. >> + >> + return !kvm_s2pte_readonly(&__pte(pteval)); >> +} >> +#else >> +/** >> + * @ret: true if dirty status set by hardware is coverred. >> + */ >> +static inline bool kvm_set_pte(pte_t *ptep, pte_t new_pte) >> { >> WRITE_ONCE(*ptep, new_pte); >> dsb(ishst); >> + return false; >> } >> +#endif /* CONFIG_ARM64_HW_AFDBM */ > > You might be able to avoid this #ifdef by redefining old_logging as: > > old_logging = IS_ENABLED(CONFIG_ARM64_HW_AFDBM) && ... > > I *think* the compiler should be able to kill the dead code and leave you with just the above when the config symbol is off. > After my test, you are right ;-) . Thanks, Keqian > Steve > >> static inline void kvm_set_pmd(pmd_t *pmdp, pmd_t new_pmd) >> { >> > > . >
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index 5ad87bce23c0..27407153121b 100644 --- a/arch/arm64/kvm/mmu.c +++ b/arch/arm64/kvm/mmu.c @@ -194,11 +194,45 @@ static void clear_stage2_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr put_page(virt_to_page(pmd)); } -static inline void kvm_set_pte(pte_t *ptep, pte_t new_pte) +#ifdef CONFIG_ARM64_HW_AFDBM +/** + * @ret: true if dirty status set by hardware is coverred. + */ +static bool kvm_set_pte(pte_t *ptep, pte_t new_pte) +{ + pteval_t old_pteval, new_pteval, pteval; + bool old_logging, new_no_write; + + old_logging = kvm_hw_dbm_enabled() && !pte_none(*ptep) && + kvm_s2pte_dbm(ptep); + new_no_write = pte_none(new_pte) || kvm_s2pte_readonly(&new_pte); + + if (!old_logging || !new_no_write) { + WRITE_ONCE(*ptep, new_pte); + dsb(ishst); + return false; + } + + new_pteval = pte_val(new_pte); + pteval = READ_ONCE(pte_val(*ptep)); + do { + old_pteval = pteval; + pteval = cmpxchg_relaxed(&pte_val(*ptep), old_pteval, new_pteval); + } while (pteval != old_pteval); + + return !kvm_s2pte_readonly(&__pte(pteval)); +} +#else +/** + * @ret: true if dirty status set by hardware is coverred. + */ +static inline bool kvm_set_pte(pte_t *ptep, pte_t new_pte) { WRITE_ONCE(*ptep, new_pte); dsb(ishst); + return false; } +#endif /* CONFIG_ARM64_HW_AFDBM */ static inline void kvm_set_pmd(pmd_t *pmdp, pmd_t new_pmd) {
kvm_set_pte is called to replace a target PTE with a desired one. We always do this without changing the desired one, but if dirty status set by hardware is coverred, let caller know it. Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> --- arch/arm64/kvm/mmu.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)