KVM: arm64: Fix protected mode handling of pages larger than 4kB

Since 3669ddd8fa8b5 ("KVM: arm64: Add a range to pkvm_mappings"), pKVM tracks the memory that has been mapped into a guest in a side data structure. Crucially, it uses it to find out whether a page has already been mapped, and therefore refuses to map it twice. So far, so good.

However, this very patch completely breaks non-4kB page support, with guests being unable to boot. The most obvious symptom is that we take the same fault repeatedly, and not making forward progress. A quick investigation shows that this is because of the above rejection code.

As it turns out, there are multiple issues at play:

- while the HPFAR_EL2 register gives you the faulting IPA minus the bottom 12 bits, it will still give you the extra bits that are part of the page offset for anything larger than 4kB, even for a level-3 mapping

- pkvm_kvm_pgtable_stage2_map() assumes that the address passed as a parameter is aligned to the size of the intended mapping

- the faulting address is only aligned for a non-page mapping

When the planets are suitably aligned (pun intended), the guest faults a page by accessing it past the bottom 4kB, and extra bits get set in the HPFAR_EL2 register. If this results in a page mapping (which is likely with large granule sizes), nothing aligns it further down, and pkvm_mapping_iter_first() finds an intersection that doesn't really exist. We assume this is a spurious fault and return -EAGAIN. And again.

This doesn't hit outside of the protected code, as the page table code always aligns the IPA down to a page boundary, hiding the issue for everyone else.

Fix it by always forcing the alignment on vma_pagesize, irrespective of the value of vma_pagesize.

Fixes 3669ddd8fa8b5 ("KVM: arm64: Add a range to pkvm_mappings")

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

---
 arch/arm64/kvm/mmu.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

arch/arm64/kvm/mmu.c +0 -3

index 8c5d259810b2f..aa587f2e28264 100644
@@ -1753,14 +1753,12 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
        }
 
        /*
-        * Both the canonical IPA and fault IPA must be hugepage-aligned to
-        * ensure we find the right PFN and lay down the mapping in the right
-        * place.

arch/arm64/kvm/mmu.c +0 -1

        gfn = ipa >> PAGE_SHIFT;
        mte_allowed = kvm_vma_mte_allowed(vma);
--

2.47.3

Hi Marc,

Quoted 13 lines · Marc Zyngier

Since 3669ddd8fa8b5 ("KVM: arm64: Add a range to pkvm_mappings"), pKVM tracks the memory that has been mapped into a guest in a side data structure. Crucially, it uses it to find out whether a page has already been mapped, and therefore refuses to map it twice. So far, so good.

However, this very patch completely breaks non-4kB page support, with guests being unable to boot. The most obvious symptom is that we take the same fault repeatedly, and not making forward progress. A quick investigation shows that this is because of the above rejection code.

Quoted 6 lines

As it turns out, there are multiple issues at play:

- while the HPFAR_EL2 register gives you the faulting IPA minus the bottom 12 bits, it will still give you the extra bits that are part of the page offset for anything larger than 4kB, even for a level-3 mapping

Matches the ARM ARM.

Quoted 2 lines

- pkvm_kvm_pgtable_stage2_map() assumes that the address passed as a parameter is aligned to the size of the intended mapping

nit: pkvm_kvm_pgtable_stage2_map() -> kvm_pgtable_stage2_map()

Quoted 41 lines

- the faulting address is only aligned for a non-page mapping

When the planets are suitably aligned (pun intended), the guest faults a page by accessing it past the bottom 4kB, and extra bits get set in the HPFAR_EL2 register. If this results in a page mapping (which is likely with large granule sizes), nothing aligns it further down, and pkvm_mapping_iter_first() finds an intersection that doesn't really exist. We assume this is a spurious fault and return -EAGAIN. And again.

This doesn't hit outside of the protected code, as the page table code always aligns the IPA down to a page boundary, hiding the issue for everyone else.

Fix it by always forcing the alignment on vma_pagesize, irrespective of the value of vma_pagesize.

Fixes 3669ddd8fa8b5 ("KVM: arm64: Add a range to pkvm_mappings")

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

---
 arch/arm64/kvm/mmu.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

arch/arm64/kvm/mmu.c +3 -4

index 8c5d259810b2f..aa587f2e28264 100644
@@ -1753,14 +1753,12 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
        }
 
        /*
-        * Both the canonical IPA and fault IPA must be hugepage-aligned to
-        * ensure we find the right PFN and lay down the mapping in the right
-        * place.
+        * Both the canonical IPA and fault IPA must be aligned to the
+        * mapping size to ensure we find the right PFN and lay down the
+        * mapping in the right place.
         */
-       if (vma_pagesize == PMD_SIZE || vma_pagesize == PUD_SIZE) {

arch/arm64/kvm/mmu.c static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, · line 1761

Quoted 5 lines

arch/arm64/kvm/mmu.c +2 -3

-               fault_ipa &= ~(vma_pagesize - 1);
-               ipa &= ~(vma_pagesize - 1);
-       }
+       fault_ipa &= ~(vma_pagesize - 1);
+       ipa &= ~(vma_pagesize - 1);

nit: Since we're changing this code anyway, should we use the ALIGN macros instead?

Reviewed-by Fuad Tabba <[email protected]>

and using 4, 16, and 64KB pages:

Tested-by Fuad Tabba <[email protected]>

Hi Fuad,

Quoted 26 lines · Fuad Tabba

Hi Marc,

Quoted 13 lines · Marc Zyngier

Since 3669ddd8fa8b5 ("KVM: arm64: Add a range to pkvm_mappings"), pKVM tracks the memory that has been mapped into a guest in a side data structure. Crucially, it uses it to find out whether a page has already been mapped, and therefore refuses to map it twice. So far, so good.

However, this very patch completely breaks non-4kB page support, with guests being unable to boot. The most obvious symptom is that we take the same fault repeatedly, and not making forward progress. A quick investigation shows that this is because of the above rejection code.

Quoted 6 lines

As it turns out, there are multiple issues at play:

- while the HPFAR_EL2 register gives you the faulting IPA minus the bottom 12 bits, it will still give you the extra bits that are part of the page offset for anything larger than 4kB, even for a level-3 mapping

Matches the ARM ARM.

Quoted 4 lines

Quoted 2 lines

- pkvm_kvm_pgtable_stage2_map() assumes that the address passed as a parameter is aligned to the size of the intended mapping

nit: pkvm_kvm_pgtable_stage2_map() -> kvm_pgtable_stage2_map()

Actually, that's pkvm_pgtable_stage2_map(). kvm_pgtable_stage2_map() itself isn't affected.

Quoted 41 lines

Quoted 41 lines

- the faulting address is only aligned for a non-page mapping

When the planets are suitably aligned (pun intended), the guest faults a page by accessing it past the bottom 4kB, and extra bits get set in the HPFAR_EL2 register. If this results in a page mapping (which is likely with large granule sizes), nothing aligns it further down, and pkvm_mapping_iter_first() finds an intersection that doesn't really exist. We assume this is a spurious fault and return -EAGAIN. And again.

This doesn't hit outside of the protected code, as the page table code always aligns the IPA down to a page boundary, hiding the issue for everyone else.

Fix it by always forcing the alignment on vma_pagesize, irrespective of the value of vma_pagesize.

Fixes 3669ddd8fa8b5 ("KVM: arm64: Add a range to pkvm_mappings")

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

---
 arch/arm64/kvm/mmu.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

arch/arm64/kvm/mmu.c +3 -4

index 8c5d259810b2f..aa587f2e28264 100644
@@ -1753,14 +1753,12 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
        }
 
        /*
-        * Both the canonical IPA and fault IPA must be hugepage-aligned to
-        * ensure we find the right PFN and lay down the mapping in the right
-        * place.
+        * Both the canonical IPA and fault IPA must be aligned to the
+        * mapping size to ensure we find the right PFN and lay down the
+        * mapping in the right place.
         */
-       if (vma_pagesize == PMD_SIZE || vma_pagesize == PUD_SIZE) {

Quoted 8 lines

arch/arm64/kvm/mmu.c static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, · line 1761

Quoted 5 lines

arch/arm64/kvm/mmu.c +2 -3

-               fault_ipa &= ~(vma_pagesize - 1);
-               ipa &= ~(vma_pagesize - 1);
-       }
+       fault_ipa &= ~(vma_pagesize - 1);
+       ipa &= ~(vma_pagesize - 1);

nit: Since we're changing this code anyway, should we use the ALIGN macros instead?

That'd be ALIGN_DOWN() then, as ALIGN() really is ALIGN_UP(), and that'd be counter-productive. Something like:

arch/arm64/kvm/mmu.c +0 -0

index aa587f2e28264..3952415c4f83b 100644

arch/arm64/kvm/mmu.c +0 -0

        gfn = ipa >> PAGE_SHIFT;
        mte_allowed = kvm_vma_mte_allowed(vma);
 

Quoted 2 lines

Reviewed-by Fuad Tabba <[email protected]>

Quoted 3 lines

and using 4, 16, and 64KB pages:

Tested-by Fuad Tabba <[email protected]>

Ah, great! I couldn't be bothered with 64kB, and only used 16kB in NV to debug quickly and then bare-metal to verify the fix.

Thanks!

        M.

--
Jazz isn't dead. It just smells funny.

Since 3669ddd8fa8b5 ("KVM: arm64: Add a range to pkvm_mappings"), pKVM tracks the memory that has been mapped into a guest in a side data structure. Crucially, it uses it to find out whether a page has already been mapped, and therefore refuses to map it twice. So far, so good.

However, this very patch completely breaks non-4kB page support, with guests being unable to boot. The most obvious symptom is that we take the same fault repeatedly, and not making forward progress. A quick investigation shows that this is because of the above rejection code.

[...]