Date:   Wed, 26 Feb 2020 09:35:26 +0800
From:   Zenghui Yu <yuzenghui@...wei.com>
To:     Marc Zyngier <maz@...nel.org>
CC:     <kvmarm@...ts.cs.columbia.edu>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <wanghaibin.wang@...wei.com>,
        Yanlei Jia <jiayanlei@...wei.com>
Subject: Re: [PATCH] irqchip/gic-v3-its: Clear Valid before writing any bits
 else in VPENDBASER

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

Powered by blists - more mailing lists