Re: [PATCH 03/12] KVM: arm64: Report hardware dirty status of stage2 PTE if coverred
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
>> ---
>> 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(_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(_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)
>> {
>>
>
> .
>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 03/12] KVM: arm64: Report hardware dirty status of stage2 PTE if coverred
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
---
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(_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(_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)
{
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH 03/12] KVM: arm64: Report hardware dirty status of stage2 PTE if coverred
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
---
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.
+ */
+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(_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(_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)
{
--
2.19.1
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
