Re: [PATCH] arm64/kvm: correct the error report in kvm_handle_guest_abort

[Marc Zyngier <maz@kernel.org>]

On 2021-01-15 09:30, Jianyong Wu wrote:
> Currently, error report when cache maintenance at read-only memory 
> range,
> like rom, is not clear enough and even not correct. As the specific 
> error
> is definitely known by kvm, it is obliged to give it out.
> 
> Fox example, in a qemu/kvm VM, if the guest do dc at the pflash range 
> from
> 0 to 128M, error is reported by kvm as "Data abort outside memslots 
> with
> no valid syndrome info" which is not quite correct.
> 
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
>  arch/arm64/kvm/mmu.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 7d2257cc5438..de66b7e38a5b 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1022,9 +1022,15 @@ int kvm_handle_guest_abort(struct kvm_vcpu 
> *vcpu)
>  		 * So let's assume that the guest is just being
>  		 * cautious, and skip the instruction.
>  		 */
> -		if (kvm_is_error_hva(hva) && kvm_vcpu_dabt_is_cm(vcpu)) {
> -			kvm_incr_pc(vcpu);
> -			ret = 1;
> +		if (kvm_vcpu_dabt_is_cm(vcpu)) {
> +			if (kvm_is_error_hva(hva)) {
> +				kvm_incr_pc(vcpu);
> +				ret = 1;
> +				goto out_unlock;
> +			}
> +
> +			kvm_err("Do cache maintenance in the read-only memory range\n");

We don't scream on the console for guests bugs.

> +			ret = -EFAULT;
>  			goto out_unlock;
>  		}

And what is userspace going to do with that? To be honest, I'd rather
not report it in any case:

- either it isn't mapped, and there is no cache to clean/invalidate
- or it is mapped read-only:
   - if it is a "DC IVAC", the guest should get the fault as per
     the ARM ARM. But I don't think we can identify the particular CMO
     at this stage, so actually performing an invalidation is the least
     bad thing to do.

How about this (untested)?

         M.

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 7d2257cc5438..0f497faad131 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1013,16 +1013,27 @@ 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. Two cases:
  		 *
-		 * So let's assume that the guest is just being
-		 * cautious, and skip the instruction.
+		 * - 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, clean/invalidate the whole memslot. We
+		 *   should special-case DC IVAC and inject a
+		 *   permission fault, but we can't really identify it
+		 *   in this context.
  		 */
-		if (kvm_is_error_hva(hva) && kvm_vcpu_dabt_is_cm(vcpu)) {
+		if (kvm_vcpu_dabt_is_cm(vcpu)) {
+			if (!kvm_is_error_hva(hva)) {
+				spin_lock(&vcpu->kvm->mmu_lock);
+				stage2_flush_memslot(vcpu->kvm, memslot);
+				spin_unlock(&vcpu->kvm->mmu_lock);
+			}
+
  			kvm_incr_pc(vcpu);
  			ret = 1;
  			goto out_unlock;

-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm