diff mbox

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

Message ID 1410208167-32532-1-git-send-email-ard.biesheuvel@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Ard Biesheuvel Sept. 8, 2014, 8:29 p.m. UTC
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@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(-)

Comments

Laszlo Ersek Sept. 8, 2014, 8:52 p.m. UTC | #1
On 09/08/14 22: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@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) {}
> 

No clue about the 32-bit case, but the 64-bit change checks out against
the ARM ARM. The ISV (ISS[24]) is documented to determine the validity
of bits ISS[23:14], but WnR is ISS[6], hence it appears independent indeed.

The pre-patch code only considered a clear WnR meaningful /
consequential only if the ISV was set -- more precisely, it only even
looked at the WnR then, due to the short-circuit nature of && --, and it
defaulted to "write fault". Synchronous data aborts due to funky
register writeback instructions don't set the ISV, hence the code used
to turn its back on the clear WnR. (Apologies for explaining it to
myself publicly.) We now ignore the ISV and key off the WnR only.

You're awesome, Ard. (And now you can drop a few patches from your
linaro-topic-virt-post-v7-roundup branch! :))

Acked-by: Laszlo Ersek <lersek@redhat.com>

Cheers
Laszlo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier Sept. 9, 2014, 9:35 a.m. UTC | #2
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@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?

Thanks,

         M.
Ard Biesheuvel Sept. 9, 2014, 10:01 a.m. UTC | #3
On 9 September 2014 11:35, Marc Zyngier <marc.zyngier@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@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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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) {}