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

[Marc Zyngier <maz@kernel.org>]

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...