KVM: arm64: Handle CMOs on Read Only memslots

It appears that when a guest traps into KVM because it is performing a CMO on a Read Only memslot, our handling of this operation is "slightly suboptimal", as we treat it as an MMIO access without a valid syndrome.

The chances that userspace is adequately equiped to deal with such an exception being slim, it would be better to handle it in the kernel.

What we need to provide is roughly as follows:

(a) if a CMO hits writeable memory, handle it as a normal memory acess
(b) if a CMO hits non-memory, skip it
(c) if a CMO hits R/O memory, that's where things become fun:
(1) if the CMO is DC IVAC, the architecture says this should result in a permission fault
(2) if the CMO is DC CIVAC, it should work similarly to (a)

We already perform (a) and (b) correctly, but (c) is a total mess. Hence we need to distinguish between IVAC (c.1) and CIVAC (c.2).

One way to do it is to treat CMOs generating a translation fault as a *read*, even when they are on a RW memslot. This allows us to further triage things:

If they come back with a permission fault, that is because this is a DC IVAC instruction:
- inside a RW memslot: no problem, treat it as a write (a)(c.2)
- inside a RO memslot: inject a data abort in the guest (c.1)

The only drawback is that DC IVAC on a yet unmapped page faults
twice: one for the initial translation fault that result in a RO mapping, and once for the permission fault. I think we can live with that.

Reported-by Jianyong Wu <[email protected]>

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

---

Notes:
I have taken the option to inject an abort in the guest when it issues a DC IVAC on a R/O memslot, but another option would be to just perform the invalidation ourselves as a DC CIAVAC.

This would have the advantage of being consistent with what we do for emulated MMIO.

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

arch/arm64/kvm/mmu.c +40 -12

index 7d2257cc5438..c7f4388bea45 100644
@@ -760,7 +760,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	struct kvm_pgtable *pgt;
 
 	fault_granule = 1UL << ARM64_HW_PGTABLE_LEVEL_SHIFT(fault_level);
-	write_fault = kvm_is_write_fault(vcpu);
+	/*
+	 * Treat translation faults on CMOs as read faults. Should
+	 * this further generate a permission fault on a R/O memslot,
+	 * it will be caught in kvm_handle_guest_abort(), with
+	 * prejudice. Permission faults on non-R/O memslot will be
+	 * gracefully handled as writes.
+	 */
+	if (fault_status == FSC_FAULT && kvm_vcpu_dabt_is_cm(vcpu))
+		write_fault = false;
+	else
+		write_fault = kvm_is_write_fault(vcpu);
 	exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu);
 	VM_BUG_ON(write_fault && exec_fault);
 

@@ -1013,19 +1023,37 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
 		}
 
 		/*
-		 * Check for a cache maintenance operation. Since we
-		 * ended-up here, we know it is outside of any memory
-		 * slot. But we can't find out if that is for a device,
-		 * or if the guest is just being stupid. The only thing
-		 * we know for sure is that this range cannot be cached.
+		 * Check for a cache maintenance operation. Three cases:
+		 *
+		 * - It is outside of any memory slot. But we can't find out
+		 *   if that is for a device, or if the guest is just being
+		 *   stupid. The only thing we know for sure is that this
+		 *   range cannot be cached.  So let's assume that the guest
+		 *   is just being cautious, and skip the instruction.
+		 *
+		 * - Otherwise, check whether this is a permission fault.
+		 *   If so, that's a DC IVAC on a R/O memslot, which is a
+		 *   pretty bad idea, and we tell the guest so.
 		 *
-		 * So let's assume that the guest is just being
-		 * cautious, and skip the instruction.
+		 * - If this wasn't a permission fault, pass it along for
+		 *   further handling (including faulting the page in if it
+		 *   was a translation fault).
 		 */
-		if (kvm_is_error_hva(hva) && kvm_vcpu_dabt_is_cm(vcpu)) {
-			kvm_incr_pc(vcpu);
-			ret = 1;
-			goto out_unlock;
+		if (kvm_vcpu_dabt_is_cm(vcpu)) {
+			if (kvm_is_error_hva(hva)) {
+				kvm_incr_pc(vcpu);
+				ret = 1;
+				goto out_unlock;
+			}
+
+			if (fault_status == FSC_PERM) {
+				/* DC IVAC on a R/O memslot */
+				kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
+				ret = 1;
+				goto out_unlock;
+			}
+
+			goto handle_access;
 		}
 
 		/*

Hi Marc,

I've been trying to get my head around what the architecture says about CMOs, so please bare with me if I misunderstood some things.

Quoted 9 lines

It appears that when a guest traps into KVM because it is performing a CMO on a Read Only memslot, our handling of this operation is "slightly suboptimal", as we treat it as an MMIO access without a valid syndrome.

The chances that userspace is adequately equiped to deal with such an exception being slim, it would be better to handle it in the kernel.

Quoted 8 lines

What we need to provide is roughly as follows:

(a) if a CMO hits writeable memory, handle it as a normal memory acess
(b) if a CMO hits non-memory, skip it
(c) if a CMO hits R/O memory, that's where things become fun:
(1) if the CMO is DC IVAC, the architecture says this should result in a permission fault
(2) if the CMO is DC CIVAC, it should work similarly to (a)

When you say it should work similarly to (a), you mean it should be handled as a normal memory access, without the "CMO hits writeable memory" part, right?

Quoted 14 lines

We already perform (a) and (b) correctly, but (c) is a total mess. Hence we need to distinguish between IVAC (c.1) and CIVAC (c.2).

One way to do it is to treat CMOs generating a translation fault as a *read*, even when they are on a RW memslot. This allows us to further triage things:

If they come back with a permission fault, that is because this is a DC IVAC instruction:
- inside a RW memslot: no problem, treat it as a write (a)(c.2)
- inside a RO memslot: inject a data abort in the guest (c.1)

Quoted 4 lines

The only drawback is that DC IVAC on a yet unmapped page faults
twice: one for the initial translation fault that result in a RO mapping, and once for the permission fault. I think we can live with that.

I'm trying to make sure I understand what the problem is.

gfn_to_pfn_prot() returnsKVM_HVA_ERR_RO_BAD if the write is to a RO memslot. KVM_HVA_ERR_RO_BAD is PAGE_OFFSET + PAGE_SIZE, which means that is_error_noslot_pfn() return true. In that case we exit to userspace with -EFAULT
for DC IVAC and DC CIVAC. But what we should be doing is this:

- For DC IVAC, inject a dabt with ISS = 0x10, meaning an external abort (that's
what kvm_inject_dabt_does()).

- For DC CIVAC, exit to userspace with -EFAULT.

Did I get that right?

Hi Alex,

Quoted 4 lines · Alexandru Elisei

Hi Marc,

I've been trying to get my head around what the architecture says about CMOs, so please bare with me if I misunderstood some things.

No worries. I've had this patch for a few weeks now, and can't make up my mind about it. It does address an actual issue though, so I couldn't just discard it... ;-)

Quoted 17 lines · Marc Zyngier

Quoted 16 lines

It appears that when a guest traps into KVM because it is performing a CMO on a Read Only memslot, our handling of this operation is "slightly suboptimal", as we treat it as an MMIO access without a valid syndrome.

The chances that userspace is adequately equiped to deal with such an exception being slim, it would be better to handle it in the kernel.

What we need to provide is roughly as follows:

(a) if a CMO hits writeable memory, handle it as a normal memory acess
(b) if a CMO hits non-memory, skip it
(c) if a CMO hits R/O memory, that's where things become fun:
  (1) if the CMO is DC IVAC, the architecture says this should result in a permission fault

Quoted 5 lines

Quoted 2 lines

  (2) if the CMO is DC CIVAC, it should work similarly to (a)

When you say it should work similarly to (a), you mean it should be handled as a normal memory access, without the "CMO hits writeable memory" part, right?

What I mean is that the cache invalidation should take place, preferably without involving KVM at all (other than populating S2 if required).

Quoted 27 lines

Quoted 14 lines

We already perform (a) and (b) correctly, but (c) is a total mess. Hence we need to distinguish between IVAC (c.1) and CIVAC (c.2).

One way to do it is to treat CMOs generating a translation fault as a *read*, even when they are on a RW memslot. This allows us to further triage things:

If they come back with a permission fault, that is because this is a DC IVAC instruction:
- inside a RW memslot: no problem, treat it as a write (a)(c.2)
- inside a RO memslot: inject a data abort in the guest (c.1)

Quoted 4 lines

The only drawback is that DC IVAC on a yet unmapped page faults
twice: one for the initial translation fault that result in a RO mapping, and once for the permission fault. I think we can live with that.

I'm trying to make sure I understand what the problem is.

gfn_to_pfn_prot() returnsKVM_HVA_ERR_RO_BAD if the write is to a RO memslot. KVM_HVA_ERR_RO_BAD is PAGE_OFFSET + PAGE_SIZE, which means that is_error_noslot_pfn() return true. In that case we exit to userspace with -EFAULT
for DC IVAC and DC CIVAC. But what we should be doing is this:

Quoted 6 lines

- For DC IVAC, inject a dabt with ISS = 0x10, meaning an external abort (that's what kvm_inject_dabt_does()).

- For DC CIVAC, exit to userspace with -EFAULT.

Did I get that right?

Not quite. What I *think* we should do is:

- DC CIVAC should just work, without going to userspace. I can't imagine a reason why we'd involve userspace for this, and we currently don't really have a good way to describe this to userspace.

- DC IVAC is more nuanced: we could either inject an exception (which is what this patch does), or perform the CMO ourselves as a DC CIVAC (consistent with the IVA->CIVA upgrade caused by having a S2 translation). This second approach is comparable to what we do when the guest issues a CMO on an emulated MMIO address (we don't inject a fault).

Thanks,

        M.

It appears that when a guest traps into KVM because it is performing a CMO on a Read Only memslot, our handling of this operation is "slightly suboptimal", as we treat it as an MMIO access without a valid syndrome.

The chances that userspace is adequately equiped to deal with such an exception being slim, it would be better to handle it in the kernel.

What we need to provide is roughly as follows:

(a) if a CMO hits writeable memory, handle it as a normal memory acess
(b) if a CMO hits non-memory, skip it
(c) if a CMO hits R/O memory, that's where things become fun:
(1) if the CMO is DC IVAC, the architecture says this should result in a permission fault
(2) if the CMO is DC CIVAC, it should work similarly to (a)

We already perform (a) and (b) correctly, but (c) is a total mess. Hence we need to distinguish between IVAC (c.1) and CIVAC (c.2).

One way to do it is to treat CMOs generating a translation fault as a *read*, even when they are on a RW memslot. This allows us to further triage things:

If they come back with a permission fault, that is because this is a DC IVAC instruction:
- inside a RW memslot: no problem, treat it as a write (a)(c.2)
- inside a RO memslot: inject a data abort in the guest (c.1)

The only drawback is that DC IVAC on a yet unmapped page faults
twice: one for the initial translation fault that result in a RO mapping, and once for the permission fault. I think we can live with that.

Reported-by Jianyong Wu <[email protected]>

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

---

Notes:
I have taken the option to inject an abort in the guest when it issues a DC IVAC on a R/O memslot, but another option would be to just perform the invalidation ourselves as a DC CIAVAC.

This would have the advantage of being consistent with what we do for emulated MMIO.

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

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

index 7d2257cc5438..c7f4388bea45 100644
@@ -760,7 +760,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	struct kvm_pgtable *pgt;
 
 	fault_granule = 1UL << ARM64_HW_PGTABLE_LEVEL_SHIFT(fault_level);

arch/arm64/kvm/mmu.c +19 -11

+	else
+		write_fault = kvm_is_write_fault(vcpu);
 	exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu);
 	VM_BUG_ON(write_fault && exec_fault);
 

@@ -1013,19 +1023,37 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
 		}
 
 		/*
-		 * Check for a cache maintenance operation. Since we
-		 * ended-up here, we know it is outside of any memory
-		 * slot. But we can't find out if that is for a device,
-		 * or if the guest is just being stupid. The only thing
-		 * we know for sure is that this range cannot be cached.
+		 * Check for a cache maintenance operation. Three cases:
+		 *
+		 * - It is outside of any memory slot. But we can't find out
+		 *   if that is for a device, or if the guest is just being
+		 *   stupid. The only thing we know for sure is that this
+		 *   range cannot be cached.  So let's assume that the guest
+		 *   is just being cautious, and skip the instruction.
+		 *
+		 * - Otherwise, check whether this is a permission fault.
+		 *   If so, that's a DC IVAC on a R/O memslot, which is a
+		 *   pretty bad idea, and we tell the guest so.
 		 *
-		 * So let's assume that the guest is just being
-		 * cautious, and skip the instruction.
+		 * - If this wasn't a permission fault, pass it along for
+		 *   further handling (including faulting the page in if it
+		 *   was a translation fault).
 		 */
-		if (kvm_is_error_hva(hva) && kvm_vcpu_dabt_is_cm(vcpu)) {
-			kvm_incr_pc(vcpu);
-			ret = 1;
-			goto out_unlock;
+		if (kvm_vcpu_dabt_is_cm(vcpu)) {
+			if (kvm_is_error_hva(hva)) {
+				kvm_incr_pc(vcpu);

Hi Marc,

Played with this for a bit to try to understand the problem better, wrote a simple MMIO device in kvmtool which maps the memory as a read-only memslot [1] and poked it with kvm-unit-tests [2].

[1] https://gitlab.arm.com/linux-arm/kvmtool-ae/-/tree/mmiodev-wip1

[2] https://gitlab.arm.com/linux-arm/kvm-unit-tests-ae/-/tree/mmiodev-wip1

Quoted 58 lines

It appears that when a guest traps into KVM because it is performing a CMO on a Read Only memslot, our handling of this operation is "slightly suboptimal", as we treat it as an MMIO access without a valid syndrome.

The chances that userspace is adequately equiped to deal with such an exception being slim, it would be better to handle it in the kernel.

What we need to provide is roughly as follows:

(a) if a CMO hits writeable memory, handle it as a normal memory acess
(b) if a CMO hits non-memory, skip it
(c) if a CMO hits R/O memory, that's where things become fun:
(1) if the CMO is DC IVAC, the architecture says this should result in a permission fault
(2) if the CMO is DC CIVAC, it should work similarly to (a)

We already perform (a) and (b) correctly, but (c) is a total mess. Hence we need to distinguish between IVAC (c.1) and CIVAC (c.2).

One way to do it is to treat CMOs generating a translation fault as a *read*, even when they are on a RW memslot. This allows us to further triage things:

If they come back with a permission fault, that is because this is a DC IVAC instruction:
- inside a RW memslot: no problem, treat it as a write (a)(c.2)
- inside a RO memslot: inject a data abort in the guest (c.1)

The only drawback is that DC IVAC on a yet unmapped page faults
twice: one for the initial translation fault that result in a RO mapping, and once for the permission fault. I think we can live with that.

Reported-by Jianyong Wu <[email protected]>

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

---

Notes:
I have taken the option to inject an abort in the guest when it issues a DC IVAC on a R/O memslot, but another option would be to just perform the invalidation ourselves as a DC CIAVAC.

This would have the advantage of being consistent with what we do for emulated MMIO.

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

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

index 7d2257cc5438..c7f4388bea45 100644
@@ -760,7 +760,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	struct kvm_pgtable *pgt;
 
 	fault_granule = 1UL << ARM64_HW_PGTABLE_LEVEL_SHIFT(fault_level);

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

Quoted 10 lines

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

-	write_fault = kvm_is_write_fault(vcpu);
+	/*
+	 * Treat translation faults on CMOs as read faults. Should
+	 * this further generate a permission fault on a R/O memslot,
+	 * it will be caught in kvm_handle_guest_abort(), with
+	 * prejudice. Permission faults on non-R/O memslot will be
+	 * gracefully handled as writes.
+	 */
+	if (fault_status == FSC_FAULT && kvm_vcpu_dabt_is_cm(vcpu))
+		write_fault = false;

This means that every DC CIVAC will map the IPA with read permissions in the stage 2 tables, regardless of the IPA being already mapped. It's harmless, but a bit unexpected.

Quoted 39 lines

arch/arm64/kvm/mmu.c +19 -11

+	else
+		write_fault = kvm_is_write_fault(vcpu);
 	exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu);
 	VM_BUG_ON(write_fault && exec_fault);
 

@@ -1013,19 +1023,37 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
 		}
 
 		/*
-		 * Check for a cache maintenance operation. Since we
-		 * ended-up here, we know it is outside of any memory
-		 * slot. But we can't find out if that is for a device,
-		 * or if the guest is just being stupid. The only thing
-		 * we know for sure is that this range cannot be cached.
+		 * Check for a cache maintenance operation. Three cases:
+		 *
+		 * - It is outside of any memory slot. But we can't find out
+		 *   if that is for a device, or if the guest is just being
+		 *   stupid. The only thing we know for sure is that this
+		 *   range cannot be cached.  So let's assume that the guest
+		 *   is just being cautious, and skip the instruction.
+		 *
+		 * - Otherwise, check whether this is a permission fault.
+		 *   If so, that's a DC IVAC on a R/O memslot, which is a
+		 *   pretty bad idea, and we tell the guest so.
 		 *
-		 * So let's assume that the guest is just being
-		 * cautious, and skip the instruction.
+		 * - If this wasn't a permission fault, pass it along for
+		 *   further handling (including faulting the page in if it
+		 *   was a translation fault).
 		 */
-		if (kvm_is_error_hva(hva) && kvm_vcpu_dabt_is_cm(vcpu)) {
-			kvm_incr_pc(vcpu);
-			ret = 1;
-			goto out_unlock;
+		if (kvm_vcpu_dabt_is_cm(vcpu)) {
+			if (kvm_is_error_hva(hva)) {
+				kvm_incr_pc(vcpu);

arch/arm64/kvm/mmu.c int kvm_handle_guest_abort(struct kvm_vcpu *vcpu) · line 1054

Quoted 10 lines

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

+				ret = 1;
+				goto out_unlock;
+			}
+
+			if (fault_status == FSC_PERM) {
+				/* DC IVAC on a R/O memslot */
+				kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
+				ret = 1;
+				goto out_unlock;
+			}

I don't like the inconsistency. We go from exiting to userspace for both DC IVAC/DC CIVAC to mapping the IPA with read permissions for DC CIVAC, but injecting a DABT for a DC IVAC. DC IVAC acts just like a DC CIVAC and requires the same permissions when executed by a guest, so I'm not sure we should be handling them differently.

Thanks,

Alex

Quoted 9 lines

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

+
+			goto handle_access;
 		}
 
 		/*

@@ -1039,6 +1067,7 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
 		goto out_unlock;
 	}
 

arch/arm64/kvm/mmu.c int kvm_handle_guest_abort(struct kvm_vcpu *vcpu) · line 1073

Quoted 4 lines

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

+handle_access:
 	/* Userspace should not be able to register out-of-bounds IPAs */
 	VM_BUG_ON(fault_ipa >= kvm_phys_size(vcpu->kvm));
 

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Quoted 9 lines · Alexandru Elisei

Hi Marc,

Played with this for a bit to try to understand the problem better, wrote a simple MMIO device in kvmtool which maps the memory as a read-only memslot [1] and poked it with kvm-unit-tests [2].

[1] https://gitlab.arm.com/linux-arm/kvmtool-ae/-/tree/mmiodev-wip1

[2] https://gitlab.arm.com/linux-arm/kvm-unit-tests-ae/-/tree/mmiodev-wip1

Looks like you forgot to add arm/mmiodev.c to your commit.

Thanks,
drew

Quoted 150 lines · Marc Zyngier

Quoted 58 lines

It appears that when a guest traps into KVM because it is performing a CMO on a Read Only memslot, our handling of this operation is "slightly suboptimal", as we treat it as an MMIO access without a valid syndrome.

The chances that userspace is adequately equiped to deal with such an exception being slim, it would be better to handle it in the kernel.

What we need to provide is roughly as follows:

(a) if a CMO hits writeable memory, handle it as a normal memory acess
(b) if a CMO hits non-memory, skip it
(c) if a CMO hits R/O memory, that's where things become fun:
(1) if the CMO is DC IVAC, the architecture says this should result in a permission fault
(2) if the CMO is DC CIVAC, it should work similarly to (a)

We already perform (a) and (b) correctly, but (c) is a total mess. Hence we need to distinguish between IVAC (c.1) and CIVAC (c.2).

One way to do it is to treat CMOs generating a translation fault as a *read*, even when they are on a RW memslot. This allows us to further triage things:

If they come back with a permission fault, that is because this is a DC IVAC instruction:
- inside a RW memslot: no problem, treat it as a write (a)(c.2)
- inside a RO memslot: inject a data abort in the guest (c.1)

The only drawback is that DC IVAC on a yet unmapped page faults
twice: one for the initial translation fault that result in a RO mapping, and once for the permission fault. I think we can live with that.

Reported-by Jianyong Wu <[email protected]>

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

---

Notes:
I have taken the option to inject an abort in the guest when it issues a DC IVAC on a R/O memslot, but another option would be to just perform the invalidation ourselves as a DC CIAVAC.

This would have the advantage of being consistent with what we do for emulated MMIO.

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

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

index 7d2257cc5438..c7f4388bea45 100644
@@ -760,7 +760,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	struct kvm_pgtable *pgt;
 
 	fault_granule = 1UL << ARM64_HW_PGTABLE_LEVEL_SHIFT(fault_level);

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

Quoted 10 lines

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

-	write_fault = kvm_is_write_fault(vcpu);
+	/*
+	 * Treat translation faults on CMOs as read faults. Should
+	 * this further generate a permission fault on a R/O memslot,
+	 * it will be caught in kvm_handle_guest_abort(), with
+	 * prejudice. Permission faults on non-R/O memslot will be
+	 * gracefully handled as writes.
+	 */
+	if (fault_status == FSC_FAULT && kvm_vcpu_dabt_is_cm(vcpu))
+		write_fault = false;

This means that every DC CIVAC will map the IPA with read permissions in the stage 2 tables, regardless of the IPA being already mapped. It's harmless, but a bit unexpected.

Quoted 39 lines

arch/arm64/kvm/mmu.c +19 -11

+	else
+		write_fault = kvm_is_write_fault(vcpu);
 	exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu);
 	VM_BUG_ON(write_fault && exec_fault);
 

@@ -1013,19 +1023,37 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
 		}
 
 		/*
-		 * Check for a cache maintenance operation. Since we
-		 * ended-up here, we know it is outside of any memory
-		 * slot. But we can't find out if that is for a device,
-		 * or if the guest is just being stupid. The only thing
-		 * we know for sure is that this range cannot be cached.
+		 * Check for a cache maintenance operation. Three cases:
+		 *
+		 * - It is outside of any memory slot. But we can't find out
+		 *   if that is for a device, or if the guest is just being
+		 *   stupid. The only thing we know for sure is that this
+		 *   range cannot be cached.  So let's assume that the guest
+		 *   is just being cautious, and skip the instruction.
+		 *
+		 * - Otherwise, check whether this is a permission fault.
+		 *   If so, that's a DC IVAC on a R/O memslot, which is a
+		 *   pretty bad idea, and we tell the guest so.
 		 *
-		 * So let's assume that the guest is just being
-		 * cautious, and skip the instruction.
+		 * - If this wasn't a permission fault, pass it along for
+		 *   further handling (including faulting the page in if it
+		 *   was a translation fault).
 		 */
-		if (kvm_is_error_hva(hva) && kvm_vcpu_dabt_is_cm(vcpu)) {
-			kvm_incr_pc(vcpu);
-			ret = 1;
-			goto out_unlock;
+		if (kvm_vcpu_dabt_is_cm(vcpu)) {
+			if (kvm_is_error_hva(hva)) {
+				kvm_incr_pc(vcpu);

arch/arm64/kvm/mmu.c int kvm_handle_guest_abort(struct kvm_vcpu *vcpu) · line 1054

Quoted 10 lines

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

+				ret = 1;
+				goto out_unlock;
+			}
+
+			if (fault_status == FSC_PERM) {
+				/* DC IVAC on a R/O memslot */
+				kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
+				ret = 1;
+				goto out_unlock;
+			}

I don't like the inconsistency. We go from exiting to userspace for both DC IVAC/DC CIVAC to mapping the IPA with read permissions for DC CIVAC, but injecting a DABT for a DC IVAC. DC IVAC acts just like a DC CIVAC and requires the same permissions when executed by a guest, so I'm not sure we should be handling them differently.

Thanks,

Alex

arch/arm64/kvm/mmu.c int kvm_handle_guest_abort(struct kvm_vcpu *vcpu) · line 1073

Quoted 13 lines

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

+
+			goto handle_access;
 		}
 
 		/*

@@ -1039,6 +1067,7 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
 		goto out_unlock;
 	}
 
+handle_access:
 	/* Userspace should not be able to register out-of-bounds IPAs */
 	VM_BUG_ON(fault_ipa >= kvm_phys_size(vcpu->kvm));
 

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm