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

[Marc Zyngier]

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.

Thanks,

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