What the hell?
On Mon, 2 Mar 2020 at 12:09, Marc Zyngier <[email protected]> wrote: > Hi Zenghui, > > On 2020-03-02 08:18, Zenghui Yu wrote: > > On 2020/3/2 3:00, Marc Zyngier wrote: > >> On 2020-02-28 19:37, Marc Zyngier wrote: > >>> On 2020-02-20 03:11, Zenghui Yu wrote: > >> > >>>> Do we really need to grab the vpe_lock for those which are belong to > >>>> the same irqchip with its_vpe_set_affinity()? The IRQ core code > >>>> should > >>>> already ensure the mutual exclusion among them, wrong? > >>> > >>> I've been trying to think about that, but jet-lag keeps getting in > >>> the way. > >>> I empirically think that you are right, but I need to go and check > >>> the various > >>> code paths to be sure. Hopefully I'll have a bit more brain space > >>> next week. > >> > >> So I slept on it and came back to my senses. The only case we actually > >> need > >> to deal with is when an affinity change impacts *another* interrupt. > >> > >> There is only two instances of this issue: > >> > >> - vLPIs have their *physical* affinity impacted by the affinity of the > >> vPE. Their virtual affinity is of course unchanged, but the > >> physical > >> one becomes important with direct invalidation. Taking a per-VPE > >> lock > >> in such context should address the issue. > >> > >> - vSGIs have the exact same issue, plus the matter of requiring some > >> *extra* one when reading the pending state, which requires a RMW > >> on two different registers. This requires an extra per-RD lock. > > > > Agreed with both! > > > >> > >> My original patch was stupidly complex, and the irq_desc lock is > >> perfectly enough to deal with anything that only affects the interrupt > >> state itself. > >> > >> GICv4 + direct invalidation for vLPIs breaks this by bypassing the > >> serialization initially provided by the ITS, as the RD is completely > >> out of band. The per-vPE lock brings back this serialization. > >> > >> I've updated the branch, which seems to run OK on D05. I still need > >> to run the usual tests on the FVP model though. > > > > I have pulled the latest branch and it looks good to me, except for > > one remaining concern: > > > > GICR_INV{LPI, ALL}R + GICR_SYNCR can also be accessed concurrently > > by multiple direct invalidation, should we also use the per-RD lock > > to ensure mutual exclusion? It looks not so harmful though, as this > > will only increase one's polling time against the Busy bit (in my > > view). > > > > But I point it out again for confirmation. > > I was about to say that it doesn't really matter because it is only a > performance optimisation (and we're noty quite there yet), until I > spotted > this great nugget in the spec: > > <quote> > Writing GICR_INVLPIR or GICR_INVALLR when GICR_SYNCR.Busy==1 is > CONSTRAINED > UNPREDICTABLE: > - The write is IGNORED . > - The invalidate specified by the write is performed. > </quote> > > So we really need some form of mutual exclusion on a per-RD basis to > ensure > that no two invalidations occur at the same time, ensuring that Busy > clears > between the two. > > Thanks for the heads up, > > M. > -- > Jazz is not dead. It just smells funny... > _______________________________________________ > kvmarm mailing list > [email protected] > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm >
_______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
