Re: [PATCH 6/7] KVM: arm64: Handle stage-2 faults on stage-1 page-table walks earlier

[Marc Zyngier <maz@kernel.org>]

On Fri, 24 Jul 2020 15:35:05 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> Stage-2 faults on stage-1 page-table walks can occur on both the I-side
> and the D-side. It is IMPLEMENTATATION DEFINED whether they are reported
> as reads or writes and, in the case that they are generated by an AT
> instruction, they are reported with the CM bit set.
> 
> All of this deeply confuses the logic in kvm_handle_guest_abort();
> userspace may or may not see the fault, depending on whether it occurs
> on the data or the instruction side, and an AT instruction may be skipped
> if the translation tables are held in a read-only memslot.

Yuk, that's indeed ugly. Well spotted. I guess the saving grace is
that a S2 trap caused by an ATS1 instruction will be reported as
S1PTW+CM, while the fault caused by a CMO is reported as *either*
S1PTW *or* CM, but never both.

> 
> Move the handling of stage-2 faults on stage-1 page-table walks earlier
> so that they consistently result in either a data or an instruction abort
> being re-injected back to the guest.

The instruction abort seems to be happening as the side effect of
executing outside of a memslot, not really because of a S1PTW.  I
wonder whether these S1PTW faults should be classified as external
aborts instead (because putting your page tables outside of a memslot
seems a bit bonkers).

> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Quentin Perret <qperret@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kvm/mmu.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index adb933ecd177..9e72e7f4a2c2 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -2124,6 +2124,11 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  			goto out;
>  		}
>  
> +		if (kvm_vcpu_dabt_iss1tw(vcpu)) {
> +			ret = -ENXIO;
> +			goto out;
> +		}
> +
>  		/*
>  		 * Check for a cache maintenance operation. Since we
>  		 * ended-up here, we know it is outside of any memory
> @@ -2157,11 +2162,6 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  			goto out_unlock;
>  		}
>  
> -		if (kvm_vcpu_dabt_iss1tw(vcpu)) {
> -			ret = -ENXIO;
> -			goto out;
> -		}
> -
>  		ret = io_mem_abort(vcpu, run, fault_ipa);
>  		goto out_unlock;
>  	}
> -- 
> 2.28.0.rc0.142.g3c755180ce-goog
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm