On 2020-09-02 12:53, Alexandru Elisei wrote:
Hi Marc,
On 9/2/20 12:10 PM, Marc Zyngier wrote:
On 2020-09-02 11:59, Alexandru Elisei wrote:
Hi,
On 8/22/20 3:44 AM, Gavin Shan wrote:
Depending on the kernel configuration, PUD_SIZE could be equal to
PMD_SIZE. For example, both of them are 512MB with the following
kernel configuration. In this case, both PUD and PMD are folded
to PGD.
CONFIG_ARM64_64K_PAGES y
CONFIG_ARM64_VA_BITS 42
CONFIG_PGTABLE_LEVELS 2
With the above configuration, the stage2 PUD is used to backup the
512MB huge page when the stage2 mapping is built. During the
mapping,
the PUD and its subordinate levels of page table entries are
unmapped
if the PUD is present and not huge page sensitive in
stage2_set_pud_huge().
Unfornately, the @addr isn't aligned to S2_PUD_SIZE and wrong page
table
entries are zapped. It eventually leads to PUD's present bit can't
be
cleared successfully and infinite loop in stage2_set_pud_huge().
This fixes the issue by checking with S2_{PUD, PMD}_SIZE instead of
{PUD, PMD}_SIZE to determine if stage2 PUD or PMD is used to back
the
huge page. For this particular case, the stage2 PMD entry should be
used to backup the 512MB huge page with stage2_set_pmd_huge().
I can reproduce this on my rockpro64 using kvmtool.
I see two issues here: first, PUD_SIZE = 512MB, but S2_PUD_SIZE = 4TB
(checked
using printk), and second, stage2_set_pud_huge() hangs. I'm working
on
debugging them.
I have this as an immediate fix for the set_pud_huge hang, tested
on Seattle with 64k/42bits.
I can't wait to see the back of this code...
The problem is in stage2_set_pud_huge(), because kvm_stage2_has_pmd()
returns
false (CONFIG_PGTABLE_LEVELS = 2):
pudp = stage2_get_pud(mmu, cache, addr);
VM_BUG_ON(!pudp);
old_pud = *pudp;
[..]
// Returns 1 because !kvm_stage2_has_pmd()
if (stage2_pud_present(kvm, old_pud)) {
/*
* If we already have table level mapping for this block, unmap
* the range for this block and retry.
*/
if (!stage2_pud_huge(kvm, old_pud)) { // Always true because
!kvm_stage2_has_pmd()
unmap_stage2_range(mmu, addr & S2_PUD_MASK, S2_PUD_SIZE);
goto retry;
}
And we end up jumping back to retry forever. IMO, in user_mem_abort(),
if PUD_SIZE
== PMD_SIZE, we should try to map PMD_SIZE instead of PUD_SIZE. Maybe
something
like this?
Err... If PUD_SIZE == PMD_SIZE, what difference does it make to map
one or the other?
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index ba00bcc0c884..178267dec511 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1886,8 +1886,10 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
phys_addr_t fault_ipa,
* As for PUD huge maps, we must make sure that we have at
least
* 3 levels, i.e, PMD is not folded.
*/
- if (vma_pagesize == PMD_SIZE ||
- (vma_pagesize == PUD_SIZE && kvm_stage2_has_pmd(kvm)))
+ if (vma_pagesize == PUD_SIZE && !kvm_stage2_has_pmd(kvm))
+ vma_pagesize = PMD_SIZE;
+
+ if (vma_pagesize == PUD_SIZE || vma_pagesize == PUD_SIZE)
gfn = (fault_ipa & huge_page_mask(hstate_vma(vma))) >>
PAGE_SHIFT;
mmap_read_unlock(current->mm);
I don't think this solves anything in the 2 level case. The gist of
the issue is that if we go on the PUD path, we end-up computing the
wrong offset for the entry and either loop or do something silly.
In either case, we read/write something we have no business touching.
Gavin sidesteps the problem by using a fantasy PUD size, while I
catch it by explicitly checking for the PMD == PUD case.
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
