[PATCH] irqchip/gic-v3-its: Clear Valid before writing any bits else in VPENDBASER

[PATCH] irqchip/gic-v3-its: Clear Valid before writing any bits else in VPENDBASER

[Zenghui Yu]

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(-)

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)
-- 
2.19.1

Re: [PATCH] irqchip/gic-v3-its: Clear Valid before writing any bits else in VPENDBASER

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] irqchip/gic-v3-its: Clear Valid before writing any bits else in VPENDBASER

[Zenghui Yu]

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

Re: [PATCH] irqchip/gic-v3-its: Clear Valid before writing any bits else in VPENDBASER

[Marc Zyngier]

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.

-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] irqchip/gic-v3-its: Clear Valid before writing any bits else in VPENDBASER

[Zenghui Yu]

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