Message ID | 54216B1B.2010205@arm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
On Tue, Sep 23, 2014 at 01:44:11PM +0100, Andre Przywara wrote: > 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. > I honestly thing this obfuscates what's going on more than it helps. I think in general complicating your data structure because of the way you consume it is the wrong way to go, unless it significantly simplifies a complicated set of manipulators. Another thing is that this fix does not address the fact that you're still returning a u64 from vgic_get_elrsr() and related functions, which will break with the use of for_each_set_bit() in the callers when the host is a 32-bit BE system. You'd have to change the accessor functions to return an (unsigned long *) as well with your change above and 64-bit BE systems would have to switch the order of the words when accessing your vgic_elrsr_bm field. I tried this, and it doesn't look nice. Therefore, I think we should really just merge the one-line fix or the patch I sent before. Paolo seems fine with it either way. If anyone feels like reviewing my patch and giving it a quick test on a BE system with a version of QEMU with the pl011 level-triggered patch, real soon, like today'ish, then we can use that, but otherwise let's go with the one-liner. -Christoffer
Il 23/09/2014 15:52, Christoffer Dall ha scritto: > Therefore, I think we should really just merge the one-line fix or the > patch I sent before. Paolo seems fine with it either way. Yes, it's on its way to Linus. Paolo
On 23 September 2014 14:52, Christoffer Dall <christoffer.dall@linaro.org> wrote: > If anyone feels like reviewing my patch and giving it a quick test on a > BE system with a version of QEMU with the pl011 level-triggered patch, FWIW, any old version of QEMU running the vexpress-a15 model will also use level-triggered interrupts for pl011, because the upstream DTB which we use for that board has always correctly marked the pl011 and all the other motherboard devices as being level-triggered. I'm still not 100% convinced we shouldn't mark the virtio-mmio devices as level-triggered, incidentally. I *think* that (a) the spec pretty heavily implies that the lines behave as level triggered but (b) the specific text in the spec about required guest code to avoid races (s.2.4.2 of the 0.9.5 spec) means that even if the interrupt controller treats them as edge triggered it's OK. -- PMM
On Tue, Sep 23, 2014 at 4:01 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 23 September 2014 14:52, Christoffer Dall > <christoffer.dall@linaro.org> wrote: >> If anyone feels like reviewing my patch and giving it a quick test on a >> BE system with a version of QEMU with the pl011 level-triggered patch, > > FWIW, any old version of QEMU running the vexpress-a15 model > will also use level-triggered interrupts for pl011, because > the upstream DTB which we use for that board has always > correctly marked the pl011 and all the other motherboard > devices as being level-triggered. > > I'm still not 100% convinced we shouldn't mark the > virtio-mmio devices as level-triggered, incidentally. > I *think* that (a) the spec pretty heavily implies that > the lines behave as level triggered but (b) the > specific text in the spec about required guest code > to avoid races (s.2.4.2 of the 0.9.5 spec) means that > even if the interrupt controller treats them as edge > triggered it's OK. > I think we should really sit down and figure out the right thing to do during KVM Forum if we can allocate a slot for that. Marc seems to also have some input he would like to share on this subject. For the record, I'm fine with changing the virtio-mmio devices, but it's probably worth quickly measuring the performance impact first. Thanks, -Christoffer
On Tue, Sep 23, 2014 at 03:52:49PM +0200, Paolo Bonzini wrote: > Il 23/09/2014 15:52, Christoffer Dall ha scritto: > > Therefore, I think we should really just merge the one-line fix or the > > patch I sent before. Paolo seems fine with it either way. > > Yes, it's on its way to Linus. > Thanks Paolo! -Christoffer
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]; };