On 06/08/13 21:49, Christoffer Dall wrote:
> On Tue, Aug 06, 2013 at 01:05:48PM +0100, Marc Zyngier wrote:
>> When using 64kB pages, we only have two levels of page tables,
>> meaning that PGD, PUD and PMD are fused. In this case, trying
>> to refcount PUDs and PMDs independantly is a a complete disaster,
>
> independently
>
>> as they are the same.
>>
>> We manage to get it right for the allocation (stage2_set_pte uses
>> {pmd,pud}_none), but the unmapping path clears both pud and pmd
>> refcounts, which fails spectacularly with 2-level page tables.
>>
>> The fix is to avoid calling clear_pud_entry when both the pmd and
>> pud pages are empty. For this, and instead of introducing another
>> pud_empty function, consolidate both pte_empty and pmd_empty into
>> page_empty (the code is actually identical) and use that to also
>> test the validity of the pud.
>>
>> Signed-off-by: Marc Zyngier <[email protected]>
>> ---
>> arch/arm/kvm/mmu.c | 22 ++++++++--------------
>> 1 file changed, 8 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index ca6bea4..7e1d899 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -85,6 +85,12 @@ static void *mmu_memory_cache_alloc(struct
>> kvm_mmu_memory_cache *mc)
>> return p;
>> }
>>
>> +static bool page_empty(void *ptr)
>> +{
>> + struct page *ptr_page = virt_to_page(ptr);
>> + return page_count(ptr_page) == 1;
>> +}
>> +
>> static void clear_pud_entry(struct kvm *kvm, pud_t *pud, phys_addr_t addr)
>> {
>> pmd_t *pmd_table = pmd_offset(pud, 0);
>> @@ -103,12 +109,6 @@ static void clear_pmd_entry(struct kvm *kvm, pmd_t
>> *pmd, phys_addr_t addr)
>> put_page(virt_to_page(pmd));
>> }
>>
>> -static bool pmd_empty(pmd_t *pmd)
>> -{
>> - struct page *pmd_page = virt_to_page(pmd);
>> - return page_count(pmd_page) == 1;
>> -}
>> -
>> static void clear_pte_entry(struct kvm *kvm, pte_t *pte, phys_addr_t addr)
>> {
>> if (pte_present(*pte)) {
>> @@ -118,12 +118,6 @@ static void clear_pte_entry(struct kvm *kvm, pte_t
>> *pte, phys_addr_t addr)
>> }
>> }
>>
>> -static bool pte_empty(pte_t *pte)
>> -{
>> - struct page *pte_page = virt_to_page(pte);
>> - return page_count(pte_page) == 1;
>> -}
>> -
>> static void unmap_range(struct kvm *kvm, pgd_t *pgdp,
>> unsigned long long start, u64 size)
>> {
>> @@ -153,10 +147,10 @@ static void unmap_range(struct kvm *kvm, pgd_t *pgdp,
>> range = PAGE_SIZE;
>>
>> /* If we emptied the pte, walk back up the ladder */
>> - if (pte_empty(pte)) {
>> + if (page_empty(pte)) {
>> clear_pmd_entry(kvm, pmd, addr);
>> range = PMD_SIZE;
>> - if (pmd_empty(pmd)) {
>> + if (page_empty(pmd) && !page_empty(pud)) {
>> clear_pud_entry(kvm, pud, addr);
>> range = PUD_SIZE;
>> }
>
> looks right, an alternative would be to check in clear_pud_entry if the
> entry actually had a value, but I don't think it's really clearer.
>
> However, this got me thinking a bit. What happens if we pass a non-pmd
> aligned address to unmap_range, and let's assume the size of the range
> is more than 2MB, won't we be leaking memory by incrementing with
> PMD_SIZE? (same argument goes for PUD_SIZE). See the patch below:
>
>
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index ca6bea4..80a83ec 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -132,37 +132,37 @@ static void unmap_range(struct kvm *kvm, pgd_t *pgdp,
> pmd_t *pmd;
> pte_t *pte;
> unsigned long long addr = start, end = start + size;
> - u64 range;
> + u64 next;
>
> while (addr < end) {
> pgd = pgdp + pgd_index(addr);
> pud = pud_offset(pgd, addr);
> if (pud_none(*pud)) {
> - addr += PUD_SIZE;
> + addr = pud_addr_end(addr, end);
> continue;
> }
>
> pmd = pmd_offset(pud, addr);
> if (pmd_none(*pmd)) {
> - addr += PMD_SIZE;
> + addr = pmd_addr_end(addr, end);
> continue;
> }
>
> pte = pte_offset_kernel(pmd, addr);
> clear_pte_entry(kvm, pte, addr);
> - range = PAGE_SIZE;
> + next = addr + PAGE_SIZE;
>
> /* If we emptied the pte, walk back up the ladder */
> if (pte_empty(pte)) {
> clear_pmd_entry(kvm, pmd, addr);
> - range = PMD_SIZE;
> + next = pmd_addr_end(addr, end);
> if (pmd_empty(pmd)) {
> clear_pud_entry(kvm, pud, addr);
> - range = PUD_SIZE;
> + next = pud_addr_end(addr, end);
> }
> }
>
> - addr += range;
> + addr = next;
> }
> }
That looks sensible. Would you prepare a patch on which I could rebase
the above?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
