diff mbox

[v3,12/14] ARM64: KVM: vgic_elrsr and vgic_eisr need to be byteswapped in BE case

Message ID 1399997646-4716-13-git-send-email-victor.kamensky@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Victor Kamensky May 13, 2014, 4:14 p.m. UTC
On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as
one 'unsigned long *' bit fields, which has 64bit size. So we need to
swap least significant word with most significant word when code reads
those registers from h/w.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
 arch/arm64/kvm/hyp.S | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Christoffer Dall May 26, 2014, 5:35 p.m. UTC | #1
On Tue, May 13, 2014 at 09:14:04AM -0700, Victor Kamensky wrote:
> On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as
> one 'unsigned long *' bit fields, which has 64bit size. So we need to
> swap least significant word with most significant word when code reads
> those registers from h/w.
> 
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
>  arch/arm64/kvm/hyp.S | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 0620691..5035b41 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -415,10 +415,17 @@ CPU_BE(	rev	w11, w11 )
>  	str	w4, [x3, #VGIC_CPU_HCR]
>  	str	w5, [x3, #VGIC_CPU_VMCR]
>  	str	w6, [x3, #VGIC_CPU_MISR]
> +#ifndef CONFIG_CPU_BIG_ENDIAN
>  	str	w7, [x3, #VGIC_CPU_EISR]
>  	str	w8, [x3, #(VGIC_CPU_EISR + 4)]
>  	str	w9, [x3, #VGIC_CPU_ELRSR]
>  	str	w10, [x3, #(VGIC_CPU_ELRSR + 4)]
> +#else
> +	str	w7, [x3, #(VGIC_CPU_EISR + 4)]
> +	str	w8, [x3, #VGIC_CPU_EISR]
> +	str	w9, [x3, #(VGIC_CPU_ELRSR + 4)]
> +	str	w10, [x3, #VGIC_CPU_ELRSR]
> +#endif
>  	str	w11, [x3, #VGIC_CPU_APR]
>  
>  	/* Clear GICH_HCR */
> -- 
> 1.8.1.4
> 
Hmm, the data structure doesn't actually clearly define this as bitmap,
but the architecture clearly defines this as two registers.  It feels
like this critical code is getting over complicated to cater for the
fact that we can use a convenient for_each_set_bit() function when we
process a maintenance interrupt.

Marc, what are your thoughts on this?  Change vgic_process_maintenance
and clearly document the array on struct vgic_cpu, or go ahead with this
change?

-Christoffer
Marc Zyngier May 27, 2014, 7:11 p.m. UTC | #2
On 26/05/14 18:35, Christoffer Dall wrote:
> On Tue, May 13, 2014 at 09:14:04AM -0700, Victor Kamensky wrote:
>> On arm64 'u32 vgic_eisr[2];' and 'u32 vgic_elrsr[2]' are accessed as
>> one 'unsigned long *' bit fields, which has 64bit size. So we need to
>> swap least significant word with most significant word when code reads
>> those registers from h/w.
>>
>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>> ---
>>  arch/arm64/kvm/hyp.S | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>> index 0620691..5035b41 100644
>> --- a/arch/arm64/kvm/hyp.S
>> +++ b/arch/arm64/kvm/hyp.S
>> @@ -415,10 +415,17 @@ CPU_BE(	rev	w11, w11 )
>>  	str	w4, [x3, #VGIC_CPU_HCR]
>>  	str	w5, [x3, #VGIC_CPU_VMCR]
>>  	str	w6, [x3, #VGIC_CPU_MISR]
>> +#ifndef CONFIG_CPU_BIG_ENDIAN
>>  	str	w7, [x3, #VGIC_CPU_EISR]
>>  	str	w8, [x3, #(VGIC_CPU_EISR + 4)]
>>  	str	w9, [x3, #VGIC_CPU_ELRSR]
>>  	str	w10, [x3, #(VGIC_CPU_ELRSR + 4)]
>> +#else
>> +	str	w7, [x3, #(VGIC_CPU_EISR + 4)]
>> +	str	w8, [x3, #VGIC_CPU_EISR]
>> +	str	w9, [x3, #(VGIC_CPU_ELRSR + 4)]
>> +	str	w10, [x3, #VGIC_CPU_ELRSR]
>> +#endif
>>  	str	w11, [x3, #VGIC_CPU_APR]
>>  
>>  	/* Clear GICH_HCR */
>> -- 
>> 1.8.1.4
>>
> Hmm, the data structure doesn't actually clearly define this as bitmap,
> but the architecture clearly defines this as two registers.  It feels
> like this critical code is getting over complicated to cater for the
> fact that we can use a convenient for_each_set_bit() function when we
> process a maintenance interrupt.
> 
> Marc, what are your thoughts on this?  Change vgic_process_maintenance
> and clearly document the array on struct vgic_cpu, or go ahead with this
> change?

So I've already started abstracting access to ELRSR and EISR in my GICv3
patches, and we could perfectly stick the code swapping code in the
accessors, letting the world switch alone.

These changes could easily be lifter from the series, and adapted. This
would also make the "bitmap-ification" more obvious.

	M.
diff mbox

Patch

diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index 0620691..5035b41 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -415,10 +415,17 @@  CPU_BE(	rev	w11, w11 )
 	str	w4, [x3, #VGIC_CPU_HCR]
 	str	w5, [x3, #VGIC_CPU_VMCR]
 	str	w6, [x3, #VGIC_CPU_MISR]
+#ifndef CONFIG_CPU_BIG_ENDIAN
 	str	w7, [x3, #VGIC_CPU_EISR]
 	str	w8, [x3, #(VGIC_CPU_EISR + 4)]
 	str	w9, [x3, #VGIC_CPU_ELRSR]
 	str	w10, [x3, #(VGIC_CPU_ELRSR + 4)]
+#else
+	str	w7, [x3, #(VGIC_CPU_EISR + 4)]
+	str	w8, [x3, #VGIC_CPU_EISR]
+	str	w9, [x3, #(VGIC_CPU_ELRSR + 4)]
+	str	w10, [x3, #VGIC_CPU_ELRSR]
+#endif
 	str	w11, [x3, #VGIC_CPU_APR]
 
 	/* Clear GICH_HCR */