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

[Jianyong Wu]

> -----Original Message-----
> From: Marc Zyngier <maz at kernel.org>
> Sent: Thursday, January 28, 2021 5:07 PM
> To: Jianyong Wu <Jianyong.Wu at arm.com>
> Cc: James Morse <James.Morse at arm.com>; will at kernel.org; Suzuki
> Poulose <Suzuki.Poulose at arm.com>; linux-arm-kernel at lists.infradead.org;
> kvmarm at lists.cs.columbia.edu; Steve Capper <Steve.Capper at arm.com>;
> Justin He <Justin.He at arm.com>; nd <nd at arm.com>
> Subject: Re: [PATCH] arm64/kvm: correct the error report in
> kvm_handle_guest_abort
> 
> On 2021-01-28 03:01, Jianyong Wu wrote:
> > Hi Marc,
> >
> >>
> >>  From 8f2a919d6f13d36445974794c76821fbb6b40f88 Mon Sep 17 00:00:00
> >> 2001
> >>  From: Marc Zyngier <maz at kernel.org>
> >> Date: Sat, 16 Jan 2021 10:53:21 +0000
> >> Subject: [PATCH] CMO on RO memslot
> >>
> >> Signed-off-by: Marc Zyngier <maz at kernel.org>
> >> ---
> >>   arch/arm64/kvm/mmu.c | 51
> +++++++++++++++++++++++++++++++++----
> >> -------
> >>   1 file changed, 39 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index
> >> 7d2257cc5438..3c176b5b0a28 100644
> >> --- a/arch/arm64/kvm/mmu.c
> >> +++ b/arch/arm64/kvm/mmu.c
> >> @@ -760,7 +760,15 @@ 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, it will be caught
> >> +	 * in kvm_handle_guest_abort(), with prejudice...
> >> +	 */
> >> +	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 +1021,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. Two 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));
> >
> > One question:
> > In general, the "DC" ops show up very early in guest. So what if the
> > guest do this before interrupt initialization? If so, the guest may
> > stuck here.
> 
> I don't understand your question. Do you mean "what if the guest does this
> without being able to handle an exception"?
> 
> If that's your question, then the answer is "don't do that".
> The architecture is clear that DC IVAC needs write permission, and will result
> in an abort being delivered if there is no writable mapping (and there can't be,
> the memslot is R/O).
> 
> DC CIVAC doesn't have that requirement, and will not generate an exception.
> 

OK, get it.
I have tested the patch above using my test case. It works well for "dc civac" and for "dc ivac" , a "Synchronous External Abort" occurs in guest as expected.

Thanks
Jianyong

> Thanks,
> 
>          M.
> --
> Jazz is not dead. It just smells funny...