From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marc Zyngier <marc.zyngier@arm.com>
Subject: Re: [PATCH 4/4] arm/arm64: KVM: use kernel mapping to perform invalidation
 on page fault
Date: Thu, 08 Jan 2015 13:07:43 +0000
Message-ID: <54AE811F.4020104@arm.com>
References: <1420718349-24152-1-git-send-email-marc.zyngier@arm.com> <1420718349-24152-5-git-send-email-marc.zyngier@arm.com> <CAFEAcA8s0JD04=3-H74fG3a97f+PCQj7WWuzXCm8svTk8j4VYw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: Christoffer Dall <christoffer.dall@linaro.org>,
	kvm-devel <kvm@vger.kernel.org>,
	"kvmarm@lists.cs.columbia.edu" <kvmarm@lists.cs.columbia.edu>
To: Peter Maydell <peter.maydell@linaro.org>
Return-path: <kvm-owner@vger.kernel.org>
Received: from foss-mx-na.foss.arm.com ([217.140.108.86]:57805 "EHLO
	foss-mx-na.foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754653AbbAHNHv (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 8 Jan 2015 08:07:51 -0500
In-Reply-To: <CAFEAcA8s0JD04=3-H74fG3a97f+PCQj7WWuzXCm8svTk8j4VYw@mail.gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 08/01/15 12:30, Peter Maydell wrote:
> On 8 January 2015 at 11:59, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> @@ -180,10 +177,32 @@ static inline void coherent_cache_guest_page(struct kvm_vcpu *vcpu, hva_t hva,
>>          *
>>          * VIVT caches are tagged using both the ASID and the VMID and doesn't
>>          * need any kind of flushing (DDI 0406C.b - Page B3-1392).
>> +        *
>> +        * We need to do this through a kernel mapping (using the
>> +        * user-space mapping has proved to be the wrong
>> +        * solution). For that, we need to kmap one page at a time,
>> +        * and iterate over the range.
>>          */
>> -       if (icache_is_pipt()) {
>> -               __cpuc_coherent_user_range(hva, hva + size);
>> -       } else if (!icache_is_vivt_asid_tagged()) {
>> +
>> +       VM_BUG_ON(size & PAGE_MASK);
>> +
>> +       while (size) {
>> +               void *va = kmap_atomic_pfn(pfn);
>> +
>> +               if (!vcpu_has_cache_enabled(vcpu) || ipa_uncached)
>> +                       kvm_flush_dcache_to_poc(va, PAGE_SIZE);
>> +
>> +               if (icache_is_pipt())
>> +                       __cpuc_coherent_user_range((unsigned long)va,
>> +                                                  (unsigned long)va + PAGE_SIZE);
>> +
>> +               size -= PAGE_SIZE;
>> +               pfn++;
>> +
>> +               kunmap_atomic(va);
>> +       }
> 
> If (vcpu_has_cache_enabled(vcpu) && !ipa_uncached && !icache_is_pipt())
> then we're going to run round this loop mapping and unmapping without
> actually doing anything. Is it worth hoisting that check out of the
> loop? (I think it's going to be the common case for a non-PIPT icache,
> right?)

Yup, that's a valid optimization.

>> +       if (!icache_is_pipt() && !icache_is_vivt_asid_tagged()) {
>>                 /* any kind of VIPT cache */
>>                 __flush_icache_all();
>>         }
> 
> Can you remind me why it's OK not to flush the icache for an
> ASID tagged VIVT icache? Making this page coherent might actually
> be revealing a change in the instructions associated with the VA,
> mightn't it?

ASID cached VIVT icaches are also VMID tagged. It is thus impossible for
stale cache lines to come with a new page. And if by synchronizing the
caches you obtain a different instruction stream, it means you've
restored the wrong page.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...