Messages in this thread | | | Subject | Re: [PATCH] irqchip/gic-v3-its: Clear Valid before writing any bits else in VPENDBASER | From | Zenghui Yu <> | Date | Tue, 25 Feb 2020 10:06:26 +0800 |
| |
Hi Marc,
On 2020/2/25 7:47, Marc Zyngier wrote: > Hi Zenghui, > > On 2020-02-24 02:50, Zenghui Yu wrote: >> The Valid bit must be cleared before changing anything else when writing >> GICR_VPENDBASER to avoid the UNPREDICTABLE behavior. This is exactly what >> we've done on 32bit arm, but not on arm64. > > I'm not quite sure how you decide that Valid must be cleared before > changing > anything else. The reason why we do it on 32bit is that we cannot update > the full 64bit register at once, so we start by clearing Valid so that > we can update the rest. arm64 doesn't require that.
The problem came out from discussions with our GIC engineers and what we talked about at that time was IHI 0069E 9.11.36 - the description of the Valid field:
"Writing a new value to any bit of GICR_VPENDBASER, other than GICR_VPENDBASER.Valid, when GICR_VPENDBASER.Valid==1 is UNPREDICTABLE."
It looks like we should first clear the Valid and then write something else. We might have some mis-understanding about this statement..
> > For the rest of discussion, let's ignore GICv4.1 32bit support (I'm > pretty sure nobody cares about that). > >> This works fine on GICv4 where we only clear Valid for a vPE deschedule. >> With the introduction of GICv4.1, we might also need to talk something >> else >> (e.g., PendingLast, Doorbell) to the redistributor when clearing the >> Valid. >> Let's port the 32bit gicr_write_vpendbaser() to arm64 so that hardware >> can >> do the right thing after descheduling the vPE. > > The spec says that: > > "For a write that writes GICR_VPENDBASER.Valid from 1 to 0, if > GICR_VPENDBASER.PendingLast is written as 1 then > GICR_VPENDBASER.PendingLast > takes an UNKNOWN value and GICR_VPENDBASER.Doorbell is treated as being 0." > > and > > "When GICR_VPENDBASER.Valid is written from 1 to 0, if there are > outstanding > enabled pending interrupts GICR_VPENDBASER.Doorbell is treated as 0." > > which indicate that PendingLast/Doorbell have to be written at the same > time > as we clear Valid.
Yes. I obviously missed these two points when writing this patch.
> Can you point me to the bit of the v4.1 spec that makes > this "clear Valid before doing anything else" requirement explicit?
No, nothing in v4.1 spec supports me :-( The above has been forwarded to Hisilicon and I will confirm these with them. It would be easy for hardware to handle the PendingLast/DB when clearing Valid, I think.
Thank you, Zenghui
| |