[PATCH] ARM/arm64: KVM: fix use of WnR bit in kvm_is_write_fault()

[Ard Biesheuvel]

On 9 September 2014 11:35, Marc Zyngier <marc.zyngier at arm.com> wrote:
> Hi Ard,
>
>
> On 2014-09-08 21:29, Ard Biesheuvel wrote:
>>
>> The ISS encoding for an exception from a Data Abort has a WnR
>> bit[6] that indicates whether the Data Abort was caused by a
>> read or a write instruction. While there are several fields
>> in the encoding that are only valid if the ISV bit[24] is set,
>> WnR is not one of them, so we can read it unconditionally.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>> ---
>>
>> This fixes an issue I observed with UEFI running under QEMU/KVM using
>> NOR flash emulation and the upcoming KVM_CAP_READONLY_MEM support, where
>> NOR flash reads were mistaken for NOR flash writes, resulting in all read
>> accesses to go through the MMIO emulation layer.
>>
>>  arch/arm/include/asm/kvm_mmu.h   | 5 +----
>>  arch/arm64/include/asm/kvm_mmu.h | 5 +----
>>  2 files changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_mmu.h
>> b/arch/arm/include/asm/kvm_mmu.h
>> index 5cc0b0f5f72f..fad5648980ad 100644
>> --- a/arch/arm/include/asm/kvm_mmu.h
>> +++ b/arch/arm/include/asm/kvm_mmu.h
>> @@ -83,10 +83,7 @@ static inline bool kvm_is_write_fault(unsigned long
>> hsr)
>>         unsigned long hsr_ec = hsr >> HSR_EC_SHIFT;
>>         if (hsr_ec == HSR_EC_IABT)
>>                 return false;
>> -       else if ((hsr & HSR_ISV) && !(hsr & HSR_WNR))
>> -               return false;
>> -       else
>> -               return true;
>> +       return hsr & HSR_WNR;
>>  }
>>
>>  static inline void kvm_clean_pgd(pgd_t *pgd)
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h
>> b/arch/arm64/include/asm/kvm_mmu.h
>> index 8e138c7c53ac..09fd9e4c13d8 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -100,10 +100,7 @@ static inline bool kvm_is_write_fault(unsigned long
>> esr)
>>         if (esr_ec == ESR_EL2_EC_IABT)
>>                 return false;
>>
>> -       if ((esr & ESR_EL2_ISV) && !(esr & ESR_EL2_WNR))
>> -               return false;
>> -
>> -       return true;
>> +       return esr & ESR_EL2_WNR;
>>  }
>>
>>  static inline void kvm_clean_pgd(pgd_t *pgd) {}
>
>
> Nice catch. One thing though.
>
> This is a case where code duplication has led to this glaring bug:
> On both arm and arm64, kvm_emulate.h has code that implements this
> correctly, just that we failed to use it. Blame me.
>
> I think this should be rewritten entierely in mmu.c, with something like
> this (fully untested, of course):
>
> static bool kvm_is_write_fault(struct kvm_vcpu *vcpu)
> {
>         if (kvm_vcpu_trap_is_iabt(vcpu))
>                 return false;
>
>         return kvm_vcpu_dabt_iswrite(vcpu);
> }
>
> Care to respin it?
>

Will do.