KVM: arm64: nv: Invalidate whole L1 blocks on TLBI RIPAS2E1
PATCH 1/1 KVM: arm64: nv: Invalidate whole L1 blocks on TLBI RIPAS2E1
The shadow stage-2 can map one of the L1's stage-2 blocks with several smaller leaves. This happens when the host backs the memory with smaller pages, when the memslot is dirty logging, or when the L1 block size falls between two host block sizes.
When the L1 invalidates part of such a block, handle_ripas2e1is() only unmaps the requested range, so the shadow leaves for the rest of the block stay valid. A TLBI must remove every TLB entry for a block it touches, including the parts outside the range. The L1 does not need to issue another TLBI before reusing the rest of the block, so the L2 can still reach those pages through the stale shadow entries.
Use compute_tlb_inval_range() in s2_mmu_unmap_range() to find the L1 block size, the same as the IPAS2E1 path does, and widen the range to whole L1 blocks. The other caller, handle_vmalls12e1is(), unmaps the whole IPA space, so the widening does not change anything there.
This only affects nested virtualization: the host must be booted with kvm-arm.mode=nested and the VM must be given a virtual EL2. In such a VM it breaks the isolation that the L1 hypervisor provides. An L2 can keep reading and writing pages that the L1 has taken back and reused, either for itself or for another L2. The host, other VMs and VMs without nested virtualization are not affected, because the shadow stage-2 only maps the L1 VM's own memory.
---
This is part of our formal analysis work at Pereq. I'm not super familiar with this code. Claude wrote a self test that I used to test, but it's crap and not worth preserving.
I also considered widening the range in handle_ripas2e1is(). We would need to use the largest block size in that scenario and it seemed like it might hurt performance.
arch/arm64/kvm/sys_regs.c | 13 ++++++++++++++-- 1 file changed, 12 insertions(+), 1 deletions(-)
arch/arm64/kvm/sys_regs.c +12 -1
@@ -4116,6 +4116,17 @@ union tlbi_info {static void s2_mmu_unmap_range (struct kvm_s2_mmu *mmu,const union tlbi_info *info) { +u64 start = info->range.start; +u64 end = start + info->range.size; + +/* +* An L1 stage-2 block may be held in the shadow S2 as several smaller +* leaves, and R_TVTYQ requires the TLBI to invalidate all of them, so +* widen each end of the range to the L1 block containing it. +*/ + start =ALIGN_DOWN (start,compute_tlb_inval_range (mmu, start >>12 )); + end =ALIGN (end,compute_tlb_inval_range (mmu, (end -1 ) >>12 )); +/* * The unmap operation is allowed to drop the MMU lock and block, which * means that @mmu could be used for a different context than the one
@@ -4139,7 +4150,7 @@ static void s2_mmu_unmap_range(struct kvm_s2_mmu *mmu,* the L1 needs to put its stage-2 in a consistent state before doing * the TLBI. */ -kvm_stage2_unmap_range (mmu, info->range.start, info->range.size, true); +kvm_stage2_unmap_range (mmu, start, end - start, true); } static bool handle_vmalls12e1is(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
Signature · 1 line
2.53.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue to consider: - [Low] KVM: arm64: nv: multiple evaluation of compute_tlb_inval_range() in ALIGN*() --
Thank you for your contribution! Sashiko AI review found 1 potential issue to consider:
- [Low] KVM: arm64: nv: multiple evaluation of compute_tlb_inval_range() in ALIGN*()
--
commit a02572444c705de05e0b8656d139113ddf1c61f9
Author: Dan Carpenter <[email protected]>
KVM: arm64: nv: Invalidate whole L1 blocks on TLBI RIPAS2E1
This commit widens the range unmapped by s2_mmu_unmap_range() to cover the containing L1 stage-2 blocks using compute_tlb_inval_range().
Quoted 8 lines
arch/arm64/kvm/sys_regs.c +0
-0@@ -4116,6 +4116,17 @@ union tlbi_info {static void s2_mmu_unmap_range (struct kvm_s2_mmu *mmu,const union tlbi_info *info) {
arch/arm64/kvm/sys_regs.c union tlbi_info { · line 4128
Quoted 10 lines
arch/arm64/kvm/sys_regs.c +10
-0+u64 start = info->range.start; +u64 end = start + info->range.size; + +/* +* An L1 stage-2 block may be held in the shadow S2 as several smaller +* leaves, and R_TVTYQ requires the TLBI to invalidate all of them, so +* widen each end of the range to the L1 block containing it. +*/ + start =ALIGN_DOWN (start,compute_tlb_inval_range (mmu, start >>12 )); + end =ALIGN (end,compute_tlb_inval_range (mmu, (end -1 ) >>12 ));
[Severity: Low]
Does passing compute_tlb_inval_range() directly to ALIGN_DOWN() and ALIGN() evaluate it five times instead of twice? ALIGN_DOWN(x, a) expands a three times and ALIGN(x, a) expands a twice via __ALIGN_KERNEL() and __ALIGN_KERNEL_MASK(), so each call to s2_mmu_unmap_range() performs three redundant sets of shadow stage-2 page-table lookups under kvm->mmu_lock.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/6817d113e5f0a9cafe0ce5bc0847276b7f14aacc.1794210561.git.dan@cryspen.com?part=1
Marc Zyngier
maintainerHi Dan,
Thanks for this patch.
Quoted 26 lines · Dan Carpenter
The shadow stage-2 can map one of the L1's stage-2 blocks with several smaller leaves. This happens when the host backs the memory with smaller pages, when the memslot is dirty logging, or when the L1 block size falls between two host block sizes.
When the L1 invalidates part of such a block,
handle_ripas2e1is()only unmaps the requested range, so the shadow leaves for the rest of the block stay valid. A TLBI must remove every TLB entry for a block it touches, including the parts outside the range. The L1 does not need to issue another TLBI before reusing the rest of the block, so the L2 can still reach those pages through the stale shadow entries.Use
compute_tlb_inval_range()ins2_mmu_unmap_range()to find the L1 block size, the same as the IPAS2E1 path does, and widen the range to whole L1 blocks. The other caller,handle_vmalls12e1is(), unmaps the whole IPA space, so the widening does not change anything there.This only affects nested virtualization: the host must be booted with kvm-arm.mode=nested and the VM must be given a virtual EL2. In such a VM it breaks the isolation that the L1 hypervisor provides. An L2 can keep reading and writing pages that the L1 has taken back and reused, either for itself or for another L2. The host, other VMs and VMs without nested virtualization are not affected, because the shadow stage-2 only maps the L1 VM's own memory.
Quoted 7 lines
Fixes 5d476ca57d7d ("KVM: arm64: nv: Add handling of range-based TLBI operations")Signed-off-by Dan Carpenter <[email protected]>---
This is part of our formal analysis work at Pereq. I'm not super familiar with this code. Claude wrote a self test that I used to test, but it's crap and not worth preserving.
Is there any chance you could get Claude to decrapify the self-test? This is the sort of things that would be very valuable to have in our toolbox.
Quoted 4 lines
I also considered widening the range in
handle_ripas2e1is(). We would need to use the largest block size in that scenario and it seemed like it might hurt performance.
It would probably result in over-invalidation, and only benefit guests that play terrible games (multiple S2s tagged with the same VMID). We're better off just computing the correct range for the given MMU context as you are doing here.
Quoted 27 lines
arch/arm64/kvm/sys_regs.c | 13 ++++++++++++++-- 1 file changed, 12 insertions(+), 1 deletions(-)arch/arm64/kvm/sys_regs.c +11
-0@@ -4116,6 +4116,17 @@ union tlbi_info {static void s2_mmu_unmap_range (struct kvm_s2_mmu *mmu,const union tlbi_info *info) { +u64 start = info->range.start; +u64 end = start + info->range.size; + +/* +* An L1 stage-2 block may be held in the shadow S2 as several smaller +* leaves, and R_TVTYQ requires the TLBI to invalidate all of them, so +* widen each end of the range to the L1 block containing it. +*/ + start =ALIGN_DOWN (start,compute_tlb_inval_range (mmu, start >>12 )); + end =ALIGN (end,compute_tlb_inval_range (mmu, (end -1 ) >>12 )); +/* * The unmap operation is allowed to drop the MMU lock and block, which * means that @mmu could be used for a different context than the one
arch/arm64/kvm/sys_regs.c static void s2_mmu_unmap_range(struct kvm_s2_mmu *mmu, · line 4156
Quoted 8 lines
arch/arm64/kvm/sys_regs.c +1
-1@@ -4139,7 +4150,7 @@ static void s2_mmu_unmap_range(struct kvm_s2_mmu *mmu,* the L1 needs to put its stage-2 in a consistent state before doing * the TLBI. */ -kvm_stage2_unmap_range (mmu, info->range.start, info->range.size, true); +kvm_stage2_unmap_range (mmu, start, end - start, true); } static bool handle_vmalls12e1is(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
The only snag here is that we end-up with two extra walks for the (pretty common) TLBI VMALLS12E1* case (which by definition covers the whole IPA space). I don't think I'll lose any sleep over it, but it may come back to bite us at a later point.
In any case, I'll queue this.
Thanks,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.