| Message ID | 20200224025029.92-1-yuzenghui@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip/gic-v3-its: Clear Valid before writing any bits else in VPENDBASER | expand |
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. 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. Can you point me to the bit of the v4.1 spec that makes this "clear Valid before doing anything else" requirement explicit? Thanks, M.
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
Hi Zenghui, On 2020-02-25 02:06, Zenghui Yu wrote: > 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.. So that's the v4.0 version of VPENDBASER. On v4.0, you start by clearing Valid, not changing any other bit. Subsequent polling of the leads to the PendingLast bit once Dirty clears. The current code follows this principle. >> 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. v4.1 changes the way VPENDBASER works in a number of way. Clearing Valid allows a "handshake": At the point of making the vPE non-resident, to specify the expected behaviour of the redistributor once the residency has been completed. This includes requesting the doorbell or telling the GIC that we don't care to know about PendingLast. This is effectively a relaxation of the v4.0 behaviour. I believe the current state of the driver matches both specs (not using common code though). Thanks, M.
On 2020/2/26 3:45, Marc Zyngier wrote: > Hi Zenghui, > > On 2020-02-25 02:06, Zenghui Yu wrote: >> 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.. > > So that's the v4.0 version of VPENDBASER. On v4.0, you start by clearing > Valid, not changing any other bit. Subsequent polling of the leads to > the PendingLast bit once Dirty clears. The current code follows this > principle. > >>> 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. > > v4.1 changes the way VPENDBASER works in a number of way. Clearing Valid > allows > a "handshake": At the point of making the vPE non-resident, to specify the > expected behaviour of the redistributor once the residency has been > completed. > This includes requesting the doorbell or telling the GIC that we don't > care to > know about PendingLast. > > This is effectively a relaxation of the v4.0 behaviour. I believe the > current > state of the driver matches both specs (not using common code though). Yes, I agree with all of the above. Thanks for your confirmation and please ignore this patch. Thanks, Zenghui
diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h index 25fec4bde43a..effe66e1ca58 100644 --- a/arch/arm64/include/asm/arch_gicv3.h +++ b/arch/arm64/include/asm/arch_gicv3.h @@ -143,7 +143,18 @@ static inline u32 gic_read_rpr(void) #define gicr_write_vpropbaser(v, c) writeq_relaxed(v, c) #define gicr_read_vpropbaser(c) readq_relaxed(c) -#define gicr_write_vpendbaser(v, c) writeq_relaxed(v, c) +#define gicr_write_vpendbaser(v, c) do { \ + u64 tmp = readq_relaxed(c); \ + \ + /* Clear Valid before writing any bits else. */ \ + if (tmp & GICR_VPENDBASER_Valid) { \ + tmp &= ~GICR_VPENDBASER_Valid; \ + writeq_relaxed(tmp, c); \ + } \ + \ + writeq_relaxed(v, c); \ +} while (0) + #define gicr_read_vpendbaser(c) readq_relaxed(c) static inline bool gic_prio_masking_enabled(void)
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. 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. Cc: Yanlei Jia <jiayanlei@huawei.com> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> --- arch/arm64/include/asm/arch_gicv3.h | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)