What  the hell?


On Mon, 2 Mar 2020 at 12:09, Marc Zyngier <maz@kernel.org> 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
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>