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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6ce5c751-6d17-b9ee-4054-edad7de075bf@huawei.com>
Date:   Tue, 25 Feb 2020 10:06: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

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