From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ard Biesheuvel Subject: Re: [PATCH] ARM/arm64: KVM: fix use of WnR bit in kvm_is_write_fault() Date: Tue, 9 Sep 2014 12:01:43 +0200 Message-ID: References: <1410208167-32532-1-git-send-email-ard.biesheuvel@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: "linux-arm-kernel@lists.infradead.org" , Christoffer Dall , Laszlo Ersek , kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org To: Marc Zyngier Return-path: Received: from mail-lb0-f171.google.com ([209.85.217.171]:62609 "EHLO mail-lb0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751914AbaIIKBp (ORCPT ); Tue, 9 Sep 2014 06:01:45 -0400 Received: by mail-lb0-f171.google.com with SMTP id 10so9306522lbg.2 for ; Tue, 09 Sep 2014 03:01:43 -0700 (PDT) In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On 9 September 2014 11:35, Marc Zyngier 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 >> --- >> >> 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: ard.biesheuvel@linaro.org (Ard Biesheuvel) Date: Tue, 9 Sep 2014 12:01:43 +0200 Subject: [PATCH] ARM/arm64: KVM: fix use of WnR bit in kvm_is_write_fault() In-Reply-To: References: <1410208167-32532-1-git-send-email-ard.biesheuvel@linaro.org> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 9 September 2014 11:35, Marc Zyngier 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 >> --- >> >> 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.