Il 23/09/2014 14:44, Andre Przywara ha scritto:
> 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.
u32 vgic_eisr[2] __aligned(BITS_PER_LONG/8);
u32 vgic_elrsr[2] __aligned(BITS_PER_LONG/8);
Still wouldn't fix big-endian, however, and it's not necessary if we go
for set_bit as in Christoffer's original patch.
Paolo
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
