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

[Jianyong Wu]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz at kernel.org>
> Sent: Friday, January 15, 2021 7:21 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-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 at 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.
Ok

> 
> > +			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;
> 
Thanks Marc, it's OK for me and I will do the test for it.

Thanks
Jianyong

> --
> Jazz is not dead. It just smells funny...