[PATCH 3/4] KVM: arm/arm64: Remove unnecessary CMOs when creating HYP page tables

[marc.zyngier@arm.com (Marc Zyngier)]

On 24/05/18 18:12, Mark Rutland wrote:
> On Thu, May 17, 2018 at 11:35:47AM +0100, Marc Zyngier wrote:
>> There is no need to perform cache maintenance operations when
>> creating the HYP page tables if we have the multiprocessing
>> extensions. ARMv7 mandates them with the virtualization support,
>> and ARMv8 just mandates them unconditionally.
>>
>> Let's remove these operations.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  virt/kvm/arm/mmu.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>> index ba66bf7ae299..acbfea09578c 100644
>> --- a/virt/kvm/arm/mmu.c
>> +++ b/virt/kvm/arm/mmu.c
>> @@ -578,7 +578,6 @@ static void create_hyp_pte_mappings(pmd_t *pmd, unsigned long start,
>>  		pte = pte_offset_kernel(pmd, addr);
>>  		kvm_set_pte(pte, pfn_pte(pfn, prot));
>>  		get_page(virt_to_page(pte));
>> -		kvm_flush_dcache_to_poc(pte, sizeof(*pte));
>>  		pfn++;
>>  	} while (addr += PAGE_SIZE, addr != end);
>>  }
>> @@ -605,7 +604,6 @@ static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start,
>>  			}
>>  			pmd_populate_kernel(NULL, pmd, pte);
>>  			get_page(virt_to_page(pmd));
>> -			kvm_flush_dcache_to_poc(pmd, sizeof(*pmd));
>>  		}
>>  
>>  		next = pmd_addr_end(addr, end);
>> @@ -638,7 +636,6 @@ static int create_hyp_pud_mappings(pgd_t *pgd, unsigned long start,
>>  			}
>>  			pud_populate(NULL, pud, pmd);
>>  			get_page(virt_to_page(pud));
>> -			kvm_flush_dcache_to_poc(pud, sizeof(*pud));
>>  		}
>>  
>>  		next = pud_addr_end(addr, end);
>> @@ -675,7 +672,6 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd,
>>  			}
>>  			pgd_populate(NULL, pgd, pud);
>>  			get_page(virt_to_page(pgd));
>> -			kvm_flush_dcache_to_poc(pgd, sizeof(*pgd));
>>  		}
>>  
>>  		next = pgd_addr_end(addr, end);
>> @@ -685,6 +681,7 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd,
>>  		pfn += (next - addr) >> PAGE_SHIFT;
>>  	} while (addr = next, addr != end);
>>  out:
>> +	dsb(ishst);
> 
> I think you need a dsb(ishst) wherever you had a
> kvm_flush_dcache_to_poc() previously.
> 
> Otherwise, the page table walker could see stale values. e.g. after you
> update a bunch of PTE entries and point a PMD entry at those, the PTW
> could see the updated PMD entry, but see stale PTE entries, which could
> be garbage.

That's a very good point. I initially only considered the EL2
initialisation (the EL2 MMU is not live yet), but failed to consider the
additional mappings at runtime (each time we instantiate a VM).

> That said, I think for ensuring the *order* those become visible in, you
> only need a dmb(ishst), and the ensure they *are visible* you need a
> DSB(ISHST) at the end.
And we definitely want the latter.

I'll respin this shortly.

Thanks,

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