Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 03/12] KVM: arm64: Report hardware dirty status of stage2
 PTE if coverred
To:     Keqian Zhu <zhukeqian1@huawei.com>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu,
        kvm@vger.kernel.org
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        liangpeng10@huawei.com, Alexios Zavras <alexios.zavras@intel.com>,
        Mark Brown <broonie@kernel.org>, Marc Zyngier <maz@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Will Deacon <will@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>
References: <20200616093553.27512-1-zhukeqian1@huawei.com>
 <20200616093553.27512-4-zhukeqian1@huawei.com>
From:   Steven Price <steven.price@arm.com>
Message-ID: <a73952ac-5e81-6c05-9b21-734e25250845@arm.com>
Date:   Wed, 1 Jul 2020 12:28:08 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <20200616093553.27512-4-zhukeqian1@huawei.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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)
>   {
>