Messages in this thread | | | Subject | Re: [PATCH 12/17] kvm-arm: Add explicit hyp page table modifiers | From | Suzuki K Poulose <> | Date | Fri, 8 Apr 2016 16:22:56 +0100 |
| |
On 08/04/16 16:09, Marc Zyngier wrote: > On 08/04/16 14:15, Christoffer Dall wrote: >> On Mon, Apr 04, 2016 at 05:26:12PM +0100, Suzuki K Poulose wrote: >>> We have common routines to modify hyp and stage2 page tables >>> based on the 'kvm' parameter. For a smoother transition to >>> using separate routines for each, duplicate the routines >>> and modify the copy to work on hyp. >>> >>> Marks the forked routines with _hyp_ and gets rid of the >>> kvm parameter which is no longer needed and is NULL for hyp. >>> Also, gets rid of calls to kvm_tlb_flush_by_vmid_ipa() calls >>> from the hyp versions. Uses explicit host page table accessors >>> instead of the kvm_* page table helpers. >>> >>> Suggested-by: Christoffer Dall <christoffer.dall@linaro.org> >>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> +static void unmap_hyp_ptes(pmd_t *pmd, phys_addr_t addr, phys_addr_t end) >>> +{ >>> + pte_t *pte, *start_pte; >>> + >>> + start_pte = pte = pte_offset_kernel(pmd, addr); >>> + do { >>> + if (!pte_none(*pte)) { >>> + pte_t old_pte = *pte; >>> + >>> + kvm_set_pte(pte, __pte(0)); >>> + >>> + /* XXX: Do we need to invalidate the cache for device mappings ? */ >> >> no, we will not be swapping out any pages mapped in Hyp mode so you can >> get rid of both of the following two lines.
OK, will remove this hunk.
>> >>> + if (!kvm_is_device_pfn(pte_pfn(old_pte))) >>> + kvm_flush_dcache_pte(old_pte); >>> + >>> + put_page(virt_to_page(pte)); >>> + } >>> + } while (pte++, addr += PAGE_SIZE, addr != end); >>> + >>> + if (hyp_pte_table_empty(start_pte)) >>> + clear_hyp_pmd_entry(pmd); >>> +} >>> + >>> +static void unmap_hyp_pmds(pud_t *pud, phys_addr_t addr, phys_addr_t end) >>> +{ >>> + phys_addr_t next; >>> + pmd_t *pmd, *start_pmd; >>> + >>> + start_pmd = pmd = pmd_offset(pud, addr); >>> + do { >>> + next = pmd_addr_end(addr, end); >>> + if (!pmd_none(*pmd)) { >>> + if (pmd_thp_or_huge(*pmd)) { >> >> do we ever actually map anything with section mappings in the Hyp >> mappings? > > No, this is purely a page mapping so far. On my system, the HYP text is > just over 4 pages big (4k pages), so the incentive is pretty low, unless > we can demonstrate some big gains due to the reduced TLB impact.
>>> +static void unmap_hyp_puds(pgd_t *pgd, phys_addr_t addr, phys_addr_t end) >>> +{ >>> + phys_addr_t next; >>> + pud_t *pud, *start_pud; >>> + >>> + start_pud = pud = pud_offset(pgd, addr); >>> + do { >>> + next = pud_addr_end(addr, end); >>> + if (!pud_none(*pud)) { >>> + if (pud_huge(*pud)) { >> >> do we ever actually map anything with huge pud >> mappings for the Hyp space? > > Same thing. Looks like there is some potential simplification here.
Right, we don't map anything with section mapping. I can clean these up.
>>> +static void unmap_hyp_range(pgd_t *pgdp, phys_addr_t start, u64 size) >>> +{ >>> + pgd_t *pgd; >>> + phys_addr_t addr = start, end = start + size; >>> + phys_addr_t next; >>> + >>> + pgd = pgdp + pgd_index(addr); >>> + do { >>> + next = pgd_addr_end(addr, end); >>> + if (!pgd_none(*pgd)) >>> + unmap_hyp_puds(pgd, addr, next); >>> + } while (pgd++, addr = next, addr != end); >> >> shouldn't we flush the EL2 (hyp) TLB here, strictly speaking? >> >> Or do we rely on all mappings ever created/torn down here to always have >> the same VA/PA relationship? Since we didn't flush the EL2 TLB in the >> existing code, that indeed does seem to be the case. > > Actually, we never unmap anything from HYP.
Except for the kvm tearing down where we clean up all the hyp table.
> Once a structure (kvm, vcpu)is mapped there, it stays forever, whatever happens > to the VM (that's because we'd otherwise have to refcount the number of objects in a page, > and I'm lazy...).
Thats one of my TODO list if there is sufficient interest in getting that done.
Thanks Suzuki
| |