Re: [PATCH 4/4] arm/arm64: KVM: use kernel mapping to perform invalidation on page fault

[Marc Zyngier <marc.zyngier@arm.com>]

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...