[GIT PULL] arm/arm64: KVM: Fix unaligned access bug on gicv2 access

From: andre.przywara@arm•com (Andre Przywara)
To: linux-arm-kernel@lists•infradead.org
Subject: [GIT PULL] arm/arm64: KVM: Fix unaligned access bug on gicv2 access
Date: Tue, 23 Sep 2014 13:44:11 +0100	[thread overview]
Message-ID: <54216B1B.2010205@arm.com> (raw)
In-Reply-To: <54215E6D.6090308@redhat.com>

Hi,

On 23/09/14 12:50, Paolo Bonzini wrote:
> Il 23/09/2014 13:14, Christoffer Dall ha scritto:
>> On Tue, Sep 23, 2014 at 10:36:30AM +0200, Paolo Bonzini wrote:
>>> Il 23/09/2014 00:07, Will Deacon ha scritto:
>>>>>>  {
>>>>>>  	if (!(lr_desc.state & LR_STATE_MASK))
>>>>>> -		set_bit(lr, (unsigned long *)vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr);
>>>>>> +		__set_bit(lr, (unsigned long *)vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr);
>>>>>>  }
>>>> Does this work for big-endian arm64 machines? Surely the bug is due to
>>>> casting a u32 * to an unsigned long *, and not specifically related to
>>>> atomics (which is where it happened to explode)?
>>>
>> It does look like the whole thing is broken on BE systems, but fixing
>> that becomes non-trivial.  I don't think this fix is incorrect in
>> itself, but we do have a larger issue with BE.
>>
>> I took a stab at fixing this (untested for BE), which looks something
>> like the following, but I'm a bit uneasy about having to test and merge
>> this as a fix given the rush before 3.17 is released.
>>
>> Thoughts?
> 
> If big-endian is broken anyway, let's apply this only:
> 
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 35b0c12..c66dc9ed 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -168,8 +168,8 @@ struct vgic_v2_cpu_if {
>>  	u32		vgic_hcr;
>>  	u32		vgic_vmcr;
>>  	u32		vgic_misr;	/* Saved only */
>> -	u32		vgic_eisr[2];	/* Saved only */
>> -	u32		vgic_elrsr[2];	/* Saved only */
>> +	u64		vgic_eisr;	/* Saved only */
>> +	u64		vgic_elrsr;	/* Saved only */
>>  	u32		vgic_apr;
>>  	u32		vgic_lr[VGIC_V2_MAX_LRS];
>>  };

I think Marc's point on this was not to spoil 32bit code (as this is the
GIC, which is shared). In the GICv2 spec the register are declared as a
number of 32 bit registers, so there is some sense in keeping it u32.
So I came up with the following this morning:

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 35b0c12..6f884df 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -168,8 +168,14 @@ struct vgic_v2_cpu_if {
        u32             vgic_hcr;
        u32             vgic_vmcr;
        u32             vgic_misr;      /* Saved only */
-       u32             vgic_eisr[2];   /* Saved only */
-       u32             vgic_elrsr[2];  /* Saved only */
+       union {
+               u32             vgic_eisr[2];   /* Saved only */
+               unsigned long   vgic_eisr_bm[8 / sizeof(long)];
+       };
+       union {
+               u32             vgic_elrsr[2];  /* Saved only */
+               unsigned long   vgic_elrsr_bm[8 / sizeof(long)];
+       };
        u32             vgic_apr;
        u32             vgic_lr[VGIC_V2_MAX_LRS];
 };

And then use vgic_elrsr_bm in set_bit().

Admittedly a bit hacky, but fixes the alignment issue while still
retaining sane code for ARM.
If anyone knows a good fix for that "8 / sizeof(long)" kludge, I am all
ears.

Regards,
Andre.

>> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
>> index 416baed..2935405 100644
>> --- a/virt/kvm/arm/vgic-v2.c
>> +++ b/virt/kvm/arm/vgic-v2.c
>> @@ -71,35 +71,17 @@ static void vgic_v2_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
>>  				  struct vgic_lr lr_desc)
>>  {
>>  	if (!(lr_desc.state & LR_STATE_MASK))
>> -		__set_bit(lr, (unsigned long *)vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr);
>> +		vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr |= (1ULL << lr);
>>  }
>>  
>>  static u64 vgic_v2_get_elrsr(const struct kvm_vcpu *vcpu)
>>  {
>> -	u64 val;
>> -
>> -#if BITS_PER_LONG == 64
>> -	val  = vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr[1];
>> -	val <<= 32;
>> -	val |= vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr[0];
>> -#else
>> -	val = *(u64 *)vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr;
>> -#endif
>> -	return val;
>> +	return vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr;
>>  }
>>  
>>  static u64 vgic_v2_get_eisr(const struct kvm_vcpu *vcpu)
>>  {
>> -	u64 val;
>> -
>> -#if BITS_PER_LONG == 64
>> -	val  = vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr[1];
>> -	val <<= 32;
>> -	val |= vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr[0];
>> -#else
>> -	val = *(u64 *)vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr;
>> -#endif
>> -	return val;
>> +	return vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr;
>>  }
>>  
>>  static u32 vgic_v2_get_interrupt_status(const struct kvm_vcpu *vcpu)
> 
> which matches what vgic-v3 already does.
> 
> BE can be fixed in 3.18.
> 
> Paolo
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 
