diff mbox

arm64: KVM: Fix HCR_EL2 and VTCR_EL2 configuration bits

Message ID 1368099611-4738-1-git-send-email-pranavkumar@linaro.org
State New
Headers show

Commit Message

PranavkumarSawargaonkar May 9, 2013, 11:40 a.m. UTC 
This patch does following fixes:

1. Make HCR_* flags as unsigned long long constants
Reason : By default, compiler assumes numeric constants as
signed hence sign extending it when assigned to unsigned variable
such as hcr_el2 (in VCPU context). This accidently sets HCR_ID and
HCR_CD making entire guest memory non-cacheable. On real HW, this
breaks Stage2 ttbl walks and also breaks VirtIO.

2. VTCR_EL2_ORGN0_WBWA and VTCR_EL2_IRGN0_WBWA macros.

Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
Signed-off-by: Anup Patel <anup.patel@linaro.org>
---
 arch/arm64/include/asm/kvm_arm.h |   73 +++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 36 deletions(-)

Comments

Marc Zyngier May 9, 2013, 12:35 p.m. UTC | #1 
Hi Pranav,

On 09/05/13 12:40, Pranavkumar Sawargaonkar wrote:
> This patch does following fixes:
> 
> 1. Make HCR_* flags as unsigned long long constants
> Reason : By default, compiler assumes numeric constants as
> signed hence sign extending it when assigned to unsigned variable
> such as hcr_el2 (in VCPU context). This accidently sets HCR_ID and
> HCR_CD making entire guest memory non-cacheable. On real HW, this
> breaks Stage2 ttbl walks and also breaks VirtIO.

Ah. Nice one. I fixed a couple of similar bugs already, but didn't
notice that one yet, for obvious reasons...

I'll probably rewrite that patch to use the UL(x) macro instead of
_AC(), as we don't need to have an unsigned long long, and that makes
the code look much nicer (sort of).

> 2. VTCR_EL2_ORGN0_WBWA and VTCR_EL2_IRGN0_WBWA macros.

Blah. Copy paste. Fucks me all the time.

> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> ---
>  arch/arm64/include/asm/kvm_arm.h |   73 +++++++++++++++++++-------------------
>  1 file changed, 37 insertions(+), 36 deletions(-)

Thanks a lot,

	M.
PranavkumarSawargaonkar May 9, 2013, 3:51 p.m. UTC | #2 
Hi Marc,

On 9 May 2013 18:05, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Pranav,
>
> On 09/05/13 12:40, Pranavkumar Sawargaonkar wrote:
>> This patch does following fixes:
>>
>> 1. Make HCR_* flags as unsigned long long constants
>> Reason : By default, compiler assumes numeric constants as
>> signed hence sign extending it when assigned to unsigned variable
>> such as hcr_el2 (in VCPU context). This accidently sets HCR_ID and
>> HCR_CD making entire guest memory non-cacheable. On real HW, this
>> breaks Stage2 ttbl walks and also breaks VirtIO.
>
> Ah. Nice one. I fixed a couple of similar bugs already, but didn't
> notice that one yet, for obvious reasons...
>
> I'll probably rewrite that patch to use the UL(x) macro instead of
> _AC(), as we don't need to have an unsigned long long, and that makes
> the code look much nicer (sort of).
>

Sure I will resubmit with UL change.
Yeah this issue keeps wondering me and anup, unexpected VIRTIO
behavior on board for sometime on board till finally realized :)

>> 2. VTCR_EL2_ORGN0_WBWA and VTCR_EL2_IRGN0_WBWA macros.
>
> Blah. Copy paste. Fucks me all the time.
>
>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>> ---
>>  arch/arm64/include/asm/kvm_arm.h |   73 +++++++++++++++++++-------------------
>>  1 file changed, 37 insertions(+), 36 deletions(-)
>
> Thanks a lot,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>

Thanks,
Pranav
diff mbox

Patch

diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 8ced0ca..0a951db 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -18,44 +18,45 @@ 
 #ifndef __ARM64_KVM_ARM_H__
 #define __ARM64_KVM_ARM_H__
 
+#include <linux/const.h>
 #include <asm/types.h>
 
 /* Hyp Configuration Register (HCR) bits */
-#define HCR_ID		(1 << 33)
-#define HCR_CD		(1 << 32)
+#define HCR_ID		(_AC(0x1, ULL) << 33)
+#define HCR_CD		(_AC(0x1, ULL) << 32)
 #define HCR_RW_SHIFT	31
-#define HCR_RW		(1 << HCR_RW_SHIFT)
-#define HCR_TRVM	(1 << 30)
-#define HCR_HCD		(1 << 29)
-#define HCR_TDZ		(1 << 28)
-#define HCR_TGE		(1 << 27)
-#define HCR_TVM		(1 << 26)
-#define HCR_TTLB	(1 << 25)
-#define HCR_TPU		(1 << 24)
-#define HCR_TPC		(1 << 23)
-#define HCR_TSW		(1 << 22)
-#define HCR_TAC		(1 << 21)
-#define HCR_TIDCP	(1 << 20)
-#define HCR_TSC		(1 << 19)
-#define HCR_TID3	(1 << 18)
-#define HCR_TID2	(1 << 17)
-#define HCR_TID1	(1 << 16)
-#define HCR_TID0	(1 << 15)
-#define HCR_TWE		(1 << 14)
-#define HCR_TWI		(1 << 13)
-#define HCR_DC		(1 << 12)
-#define HCR_BSU		(3 << 10)
-#define HCR_BSU_IS	(1 << 10)
-#define HCR_FB		(1 << 9)
-#define HCR_VA		(1 << 8)
-#define HCR_VI		(1 << 7)
-#define HCR_VF		(1 << 6)
-#define HCR_AMO		(1 << 5)
-#define HCR_IMO		(1 << 4)
-#define HCR_FMO		(1 << 3)
-#define HCR_PTW		(1 << 2)
-#define HCR_SWIO	(1 << 1)
-#define HCR_VM		(1)
+#define HCR_RW		(_AC(0x1, ULL) << HCR_RW_SHIFT)
+#define HCR_TRVM	(_AC(0x1, ULL) << 30)
+#define HCR_HCD		(_AC(0x1, ULL) << 29)
+#define HCR_TDZ		(_AC(0x1, ULL) << 28)
+#define HCR_TGE		(_AC(0x1, ULL) << 27)
+#define HCR_TVM		(_AC(0x1, ULL) << 26)
+#define HCR_TTLB	(_AC(0x1, ULL) << 25)
+#define HCR_TPU		(_AC(0x1, ULL) << 24)
+#define HCR_TPC		(_AC(0x1, ULL) << 23)
+#define HCR_TSW		(_AC(0x1, ULL) << 22)
+#define HCR_TAC		(_AC(0x1, ULL) << 21)
+#define HCR_TIDCP	(_AC(0x1, ULL) << 20)
+#define HCR_TSC		(_AC(0x1, ULL) << 19)
+#define HCR_TID3	(_AC(0x1, ULL) << 18)
+#define HCR_TID2	(_AC(0x1, ULL) << 17)
+#define HCR_TID1	(_AC(0x1, ULL) << 16)
+#define HCR_TID0	(_AC(0x1, ULL) << 15)
+#define HCR_TWE		(_AC(0x1, ULL) << 14)
+#define HCR_TWI		(_AC(0x1, ULL) << 13)
+#define HCR_DC		(_AC(0x1, ULL) << 12)
+#define HCR_BSU		(_AC(0x3, ULL) << 10)
+#define HCR_BSU_IS	(_AC(0x1, ULL) << 10)
+#define HCR_FB		(_AC(0x1, ULL) << 9)
+#define HCR_VA		(_AC(0x1, ULL) << 8)
+#define HCR_VI		(_AC(0x1, ULL) << 7)
+#define HCR_VF		(_AC(0x1, ULL) << 6)
+#define HCR_AMO		(_AC(0x1, ULL) << 5)
+#define HCR_IMO		(_AC(0x1, ULL) << 4)
+#define HCR_FMO		(_AC(0x1, ULL) << 3)
+#define HCR_PTW		(_AC(0x1, ULL) << 2)
+#define HCR_SWIO	(_AC(0x1, ULL) << 1)
+#define HCR_VM		(_AC(0x1, ULL))
 
 /*
  * The bits we set in HCR:
@@ -111,9 +112,9 @@ 
 #define VTCR_EL2_SH0_MASK	(3 << 12)
 #define VTCR_EL2_SH0_INNER	(3 << 12)
 #define VTCR_EL2_ORGN0_MASK	(3 << 10)
-#define VTCR_EL2_ORGN0_WBWA	(3 << 10)
+#define VTCR_EL2_ORGN0_WBWA	(1 << 10)
 #define VTCR_EL2_IRGN0_MASK	(3 << 8)
-#define VTCR_EL2_IRGN0_WBWA	(3 << 8)
+#define VTCR_EL2_IRGN0_WBWA	(1 << 8)
 #define VTCR_EL2_SL0_MASK	(3 << 6)
 #define VTCR_EL2_SL0_LVL1	(1 << 6)
 #define VTCR_EL2_T0SZ_MASK	0x3f