From mboxrd@z Thu Jan 1 00:00:00 1970 From: andre.przywara@arm.com (Andre Przywara) Date: Tue, 23 Sep 2014 13:44:11 +0100 Subject: [GIT PULL] arm/arm64: KVM: Fix unaligned access bug on gicv2 access In-Reply-To: <54215E6D.6090308@redhat.com> References: <1411420350-368-1-git-send-email-christoffer.dall@linaro.org> <1411420350-368-2-git-send-email-christoffer.dall@linaro.org> <20140922220710.GA24499@arm.com> <5421310E.9020902@redhat.com> <20140923111412.GG12187@cbox> <54215E6D.6090308@redhat.com> Message-ID: <54216B1B.2010205@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 >