Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Bharat Bhushan <bbhushan2@marvell.com>
To:     Mark Rutland <mark.rutland@arm.com>
CC:     "catalin.marinas@arm.com" <catalin.marinas@arm.com>,
        "will@kernel.org" <will@kernel.org>,
        "daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>,
        "maz@kernel.org" <maz@kernel.org>,
        "konrad.dybcio@somainline.org" <konrad.dybcio@somainline.org>,
        "saiprakash.ranjan@codeaurora.org" <saiprakash.ranjan@codeaurora.org>,
        "robh@kernel.org" <robh@kernel.org>,
        "marcan@marcan.st" <marcan@marcan.st>,
        "suzuki.poulose@arm.com" <suzuki.poulose@arm.com>,
        "broonie@kernel.org" <broonie@kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Linu Cherian <lcherian@marvell.com>,
        Sunil Kovvuri Goutham <sgoutham@marvell.com>
Subject: RE: [EXT] Re: [PATCH] clocksource: Add Marvell Errata-38627
 workaround
Thread-Topic: [EXT] Re: [PATCH] clocksource: Add Marvell Errata-38627
 workaround
Thread-Index: AQHXcWQ/kwOpukHdIkqoSg2KEiIGgKs0F7CAgAAR5yCABNAjgIABJ6DwgAb/o4CAE6bNMA==
Date:   Mon, 26 Jul 2021 04:29:53 +0000
Message-ID: <CO6PR18MB4465AAE7916DCFECE9D47EA2E3E89@CO6PR18MB4465.namprd18.prod.outlook.com>
References: <20210705060843.3150-1-bbhushan2@marvell.com>
 <20210705090753.GD38629@C02TD0UTHF1T.local>
 <CO6PR18MB4465687A22FE724E59D4225CE3199@CO6PR18MB4465.namprd18.prod.outlook.com>
 <20210708114157.GC24650@C02TD0UTHF1T.local>
 <CO6PR18MB4465C85A872D7CE4138EBF8FE3149@CO6PR18MB4465.namprd18.prod.outlook.com>
 <20210713161233.GB13027@C02TD0UTHF1T.local>
In-Reply-To: <20210713161233.GB13027@C02TD0UTHF1T.local>
Accept-Language: en-IN, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?7bEORaUVyxZRqQk0VrqwG+CR2pqqfY1z8feC/xeSDg6K+AOu5OjL7bZxcjvx?=
 =?us-ascii?Q?1uw7uFazYdACzTTd4E+fo0oh7EOhwAdDAdCl0TVIFeAm0PGvtStx1f5gXfVN?=
 =?us-ascii?Q?fr1U9U+GryEAbZnmulj5kLrJKYNLPa0dxgBtr8sS2iiNMUKwi8/oW6IRvSHX?=
 =?us-ascii?Q?Sqx7CPi3+Onp9ym6IuYHhPrZ1VFaVNcigmsSyCCUmEz0s1PF2AdqjCug8TQt?=
 =?us-ascii?Q?DnjHPNEtcX5Hso2oHvCp6Bah15I8ZnU0dKBh/XyQfz7sAbph6k2Hm5QwnqiV?=
 =?us-ascii?Q?IxrZt+eXxhD7BZkDfSXJDNUlSMrr7mF4mbLulWtRW9g3aFDN+LlN6+1sQ3DF?=
 =?us-ascii?Q?PuPRkpYd7lLL/fYHC/Kdb7+QV1EUIqsNe3GocMVxkjHH2da2PU8Z+WhZCOKi?=
 =?us-ascii?Q?hk/iNHN8JzDjLnIc01mTQ/ppmu2SMRk1yKkh8fbbnEsYFQiM/PKyFqKPnv7g?=
 =?us-ascii?Q?N1gkv7K144m/+fu61Jka2/Uc2ra895NVC8aU51ZHUztf1Z4mlOB3WbjGz/EG?=
 =?us-ascii?Q?ic/imQzfbp7IrPgLmFJ29zpMbi959KLHsZt6xo1DKDmO4yNSF2feMJQGCdRG?=
 =?us-ascii?Q?fCen24wUu0p3W1miLwu9wJJkr1VzyfPzKrJVCck/+5m4By2BzrxdYrxRt45W?=
 =?us-ascii?Q?TAc93ow4gv+o8txpGfLleVlgyvEvVEvtn9qtDc/eAhaTUVvna92UTbGLQuw/?=
 =?us-ascii?Q?8NfsyR5UXfCrkpzALe0X0JP43Tna0jTcL7t6cDMRplo4mu5DhmxRoECa238c?=
 =?us-ascii?Q?Pe5H5g44ns1JmLYtJmwUeYDAnSa1tH2KydnggB65He8nyGirGZdX3ItR77jP?=
 =?us-ascii?Q?oY8flAYsFrUsolaj/FTI3R+qVY4EhG+yV+3WuQNS1yE++YBZT99L0bKN/iAr?=
 =?us-ascii?Q?xLiVxwfQbjnRjtdEf59D+/7D+Slewdcq5V62e/fV6gna6/M8shAtv57/dW6W?=
 =?us-ascii?Q?d5d2qEtJXTyvKs8GBNd+WVs3jgLCOlx2D6i0Y9MiCyj6S1l0nH7Lw6OHsDQr?=
 =?us-ascii?Q?ob4fW3pNLwHrmbtWA4yfx/CBHV0xLPx3Gjwt/TuOA6r4wgcuBA+CcCt4kccD?=
 =?us-ascii?Q?ch7E+ebvXMIht2cM2WP84l4nwjNBPv1+FyrrTkoPH70VDUz4cLUaWQr2A8vq?=
 =?us-ascii?Q?kstwqvldEOa93IqopnZ+cIbgbJUVlb98FVQ1Q3IGqixE6u9ZSLjzbR73GP/v?=
 =?us-ascii?Q?Os/jmO1WhLrURwDfjItMXgYpMUAwlagg/RfPPyIOGgoRpjPvwnworW170RH4?=
 =?us-ascii?Q?7tEXzqzL5vSQp1ydyUiiylk4zBBfzpYTh89bPrKMzdzSeL87THgdyWWRvOfe?=
 =?us-ascii?Q?dABXp0OjvHw+/YwS3xzMZPTM?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CO6PR18MB4465.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 65fc025a-3065-449a-ce61-08d94fee03ae
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Jul 2021 04:29:53.0472
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: G2CNgFTsd9RND08+w1F4bzaGEUKWlbHLYIyo61LTeNxE4hwKdcIN4ors+JqYGXM8nSUfvTyEZ6KEfjUJVPOcbQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR18MB4746
X-Proofpoint-GUID: 2cILV6fKtIakugxb_2crmbrxxcd89dku
X-Proofpoint-ORIG-GUID: 2cILV6fKtIakugxb_2crmbrxxcd89dku
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.790
 definitions=2021-07-26_01:2021-07-23,2021-07-26 signatures=0
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Sorry for delayed response

Please see inline

> -----Original Message-----
> From: Mark Rutland <mark.rutland@arm.com>
> Sent: Tuesday, July 13, 2021 9:43 PM
> To: Bharat Bhushan <bbhushan2@marvell.com>
> Cc: catalin.marinas@arm.com; will@kernel.org; daniel.lezcano@linaro.org;
> maz@kernel.org; konrad.dybcio@somainline.org;
> saiprakash.ranjan@codeaurora.org; robh@kernel.org; marcan@marcan.st;
> suzuki.poulose@arm.com; broonie@kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Linu Cherian
> <lcherian@marvell.com>; Sunil Kovvuri Goutham <sgoutham@marvell.com>
> Subject: Re: [EXT] Re: [PATCH] clocksource: Add Marvell Errata-38627
> workaround
>=20
> On Tue, Jul 13, 2021 at 02:40:22AM +0000, Bharat Bhushan wrote:
> > Hi Mark,
> >
> > > -----Original Message-----
> > > From: Mark Rutland <mark.rutland@arm.com>
> > > Sent: Thursday, July 8, 2021 5:12 PM
> > > To: Bharat Bhushan <bbhushan2@marvell.com>
> > > Cc: catalin.marinas@arm.com; will@kernel.org;
> > > daniel.lezcano@linaro.org; maz@kernel.org;
> > > konrad.dybcio@somainline.org; saiprakash.ranjan@codeaurora.org;
> > > robh@kernel.org; marcan@marcan.st; suzuki.poulose@arm.com;
> > > broonie@kernel.org; linux-arm- kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; Linu Cherian <lcherian@marvell.com>
> > > Subject: Re: [EXT] Re: [PATCH] clocksource: Add Marvell Errata-38627
> > > workaround
> > >
> > > On Thu, Jul 08, 2021 at 10:47:42AM +0000, Bharat Bhushan wrote:
> > > > Hi Mark,
> > > >
> > > > Sorry for the delay, was gathering some details.
> > > > Pease see inline
> > > >
> > > > > -----Original Message-----
> > > > > From: Mark Rutland <mark.rutland@arm.com>
> > > > > Sent: Monday, July 5, 2021 2:38 PM
> > > > > To: Bharat Bhushan <bbhushan2@marvell.com>
> > > > > Cc: catalin.marinas@arm.com; will@kernel.org;
> > > > > daniel.lezcano@linaro.org; maz@kernel.org;
> > > > > konrad.dybcio@somainline.org; saiprakash.ranjan@codeaurora.org;
> > > > > robh@kernel.org; marcan@marcan.st; suzuki.poulose@arm.com;
> > > > > broonie@kernel.org; linux-arm- kernel@lists.infradead.org;
> > > > > linux-kernel@vger.kernel.org; Linu Cherian
> > > > > <lcherian@marvell.com>
> > > > > Subject: [EXT] Re: [PATCH] clocksource: Add Marvell Errata-38627
> > > > > workaround
> > > > >
> > > > > External Email
> > > > >
> > > > > ----------------------------------------------------------------
> > > > > ----
> > > > > --
> > > > > Hi Bharat,
> > > > >
> > > > > On Mon, Jul 05, 2021 at 11:38:43AM +0530, Bharat Bhushan wrote:
> > > > > > CPU pipeline have unpredicted behavior when timer interrupt
> > > > > > appears and then disappears prior to the exception happening.
> > > > > > Time interrupt appears on timer expiry and disappears when
> > > > > > timer programming or timer disable. This typically can happen
> > > > > > when a load instruction misses in the cache,  which can take
> > > > > > few hundreds of cycles, and an interrupt appears after the
> > > > > > load instruction starts executing but disappears before the loa=
d
> instruction completes.
> > > > >
> > > > > Could you elaborate on the scenario? What sort of unpredictable
> > > > > behaviour can occur? e.g:
> > > >
> > > > This is a race condition where an instruction (except store,
> > > > system, load atomic and load exclusive) becomes "nop" if interrupt
> > > > appears and disappears before taken by CPU. For example interrupt
> > > > appears after the atomic load instruction starts executing and
> > > > disappears before the atomic load instruction completes, in that
> > > > case instruction (not all) can become "nop". As interrupt
> > > > disappears before atomic instruction completes, cpu continues to
> > > > execute and while take junk from register as other dependent got "n=
op".
> > >
> > > Thanks for this; I have a number of further questions below.
> > >
> > > You said this doesn't apply to:
> > >
> > > * store
> > > * system
> > > * load atomic
> > > * load exclusive
> > >
> > > ... but your example explains this happening for an atomic load,
> > > which was in that list. Was the example bad, or was the list wrong?
> >
> > The load atomic completes successfully. It doesn't become a nop. A
> > loads atomic is significant just because it's an instruction which has
> > a long time between executing an retiring. This provides a window of
> > vulnerability when an interrupt asserts and then deasserts. This
> > stimulates the bug and causes other instructions executing in
> > parallel,  which can get nop.
>=20
> Thanks for clarifiying; this was not clear from your initial description.
>=20
> > > It's not entirely clear to me which instructions this covers. e.g.
> > > is "system" the entire system instruction class (i.e. all opcodes
> > > 0b110101010_0xxxxxxx_xxxxxxxx_xxxxxxxx), or did you mean something
> > > more specific? Does "store" include store-exlcusive?
> > >
> > > Other than that list, can this occur for *any* instruction? e.g.
> > > MOV, SHA256*, *DIV?
> >
> > There are two general classes of instructions. Those that only change
> > a gpr or PC. These are arithmetic, floating point, branch. Loads with
> > no side effects also fall into this category. These are the
> > instructions that can erroneously be nop'd. The other category are
> > instructions that can change architectural state more than a GPR.
> > These include all stores, atomic loads, exclusive loads, loads to
> > non-cacheable space,msr,mrs,eret,tlb*,sys,brk,etc, these does not get
> > "noped"
> >
> > >
> > > Does this only apply to a single instruction at a time, or can
> > > multiple instructions "become nop"?
> >
> > Can be multiple,
> >
> > >
> > > When an instruction "becomes nop", will subsequent instructions see
> > > a consistent architectural state (e.g. GPRs as they were exactly
> > > before the instruction which "becomes nop"), or can they see
> > > something else (e.g. garbage forwarded from register renaming or othe=
r
> internal microarchitectural state)?
> >
> > > > > * Does the CPU lockup?
> > > > No
> > > >
> > > > > * Does the CPU take the exception at all?
> > > > No
> > > >
> > > > > * Does the load behave erroneously?
> > > > No,
> > > >
> > > > > * Does any CPU state (e.g. GPRs, PC, PSTATE) become corrupted?
> > > >
> > > > yes, GPRs will get corrupted, will have stale value
> > >
> > > As above, is that the prior architectural value of the GPRs, or can
> > > that be some bogus microarchitectural state (e.g. from renaming or ot=
her
> forwarding paths)?
> >
> > The instructions that become a nop doesn't write the GPR and because
> > this is an OOO machine the GPR result isn't the prior architectural
> > value but whatever crap is leftover in the physical register.
>=20
> Ok, so that's a potential information leak from a different context (e.g.=
 higher EL),
> depending on what happened to be left in that physical register.
>=20
> Consider a malicious guest at EL1. What prevents it from triggering this
> deliberately, then inspecting the GPRs after taking the IRQ in order to r=
ead prior
> secrets?
>=20
> > > > > Does the problem manifest when IRQs are masked by DAIF.I, or by
> > > > > CNT*_CTL_EL0.{IMASK,ENABLE} ?
> > > >
> > > > No, there are no issue if interrupts are masked.
> > >
> > > If a write to CNTV_CTL_EL0.IMASK races with the interrupt being
> > > asserted, can that trigger the problem?
> >
> > If interrupt is enabled (DAIF) - then it will be taken, and no issue
> > But if interrupts are disabled then following sequence can see the race
> >  1)	interrupt is disabled (DAIF)
> > 2)	TVAL/ENABLE/IMASK at timer h/w programming to de-assert interrupt.
> > 	Race of Irq asserted before irq de-asserted, than this short window of
> assertion will be considered as spike from timer h/w block
> > 3)	Enable DAIF
> > 	Because of propagation delay CPU sees assertion and de-assertion
> > (spike), errata hit
> >
> > Will add "isb" around interrupt enablement in next version of patch.
>=20
> ... why?
>=20
> That doesn't seem to follow from the abvoe, so I think I'm missing a step=
.
>=20
> > > If a write to DAIF.I races with the interrupt being asserted, can
> > > that trigger the problem?
> >
> > No race with writing to DAIF.I with interrupt assertion, Writing
> > DAIF.I =3D 0 (enablement of interrupt) can race with de-assertion, whic=
h
> > can lead to hitting errata
>=20
> Ok.
>=20
> That *might* make it possible to bodge around the timer specifically, but=
 as
> below I don't think we can ensure this is safe in the presence of virtual=
ization,
> nor when considering other interrupts.
>=20
> > > From your description so far, this doesn't sound like it is specific
> > > to the timer interrupt. Is it possible for a different interrupt to t=
rigger this, e.g:
> > >
> > > * Can the same happen with another PPI, e.g. the PMU interrupt if tha=
t
> > >   gets de-asserted, or there's a race with DAIF.I?
> > >
> > > * Can the same happen with an SGI, e.g. if one CPU asserts then
> > >   de-asserts an SGI targetting another CPU, or there's a race with
> > >   DAIF.I?
> > >
> > > * Can the same happen with an SPI, e.g. if a device asserts then
> > >   de-asserts its IRQ line, or there's a race with DAIF.I?
> >
> > No issue with edge triggered, but this can happen with any level sensit=
ive
> interrupt.
>=20
> Ok.
>=20
> So that'll include at least the PMU and
>=20
> > > If not, *why* does this happen specifically for the timer interrupt?
> > >
> > > > > > Workaround of this is to ensure maximum 2us of time gap
> > > > > > between timer interrupt and timer programming which can de-asse=
rt
> timer interrupt.
> > > > >
> > > > > The code below seems to try to enforce a 2us *minimum*. Which is
> > > > > it supposed to be?
> > > >
> > > > Yes, it is minimum 2us.
> > > >
> > > > >
> > > > > Can you explain *why* this is supposed to help?
> > > > With the workaround interrupt assertion and de-assertion will be
> > > > minimum 2us
> > > apart.
> > >
> > > I understood that, but why is that deemed to be sufficient? e.g. is
> > > it somehow guaranteed that the CPU will complete the instruction that=
 would
> "become nop"
> > > in that time?
> >
> > With this delay we avoid spike, either this this will becomes an
> > actual interrupt or the spike never visible to core.
> >
> > > > > I don't see how we can guarantee this in a VM, or if the CPU
> > > > > misses on an instruction fetch.
> > > >
> > > > This errata applies to VM (virtual timer) as well, maybe there is
> > > > some gap in my understanding, how it will be different in VM.
> > > > Can you help with what issue we can have VM?
> > >
> > > A VCPU can be pre-empted by the host at *any* time, for an arbitrary
> > > length of time. So e.g. you can have a scenario such as:
> > >
> > > 1. Guest reads CNTx_TVAL, sees interrupt is 4us in the future and
> > >    decides it does not need to wait
> > > 2. Host preempts guest
> > > 3. Host does some processing for ~3.9us 4. Host returns to guest,
> > > with 0.1us left until the interrupt triggers 5. Guest reprograms
> > > CNTx_TVAL, and triggers the erratum
> >
> > Yes, when timer expire just before tval written (race condition) , so
> > there is assertion-followed by de-assertion, As interrupts are enabled
> > in host, interrupt will be visible as spike to host.
>=20
> Ok, so that's a recipe for the guest to attack the host.
>=20
> > We will apply workaround whenever entering to guest (add a delay
> > before exiting to guest in case guest timer is going to expire).
>=20
> I think this is papering over the problem.
>=20
> You've said this can happen for *any* level-triggered interrupt. AFAIK, n=
othing
> prevents a malicious guest from deliberately asserting and de-asserting a=
 level-
> triggered interrupt (e.g. by writing to the GIC distributor), and I also =
note that the
> GIC maintenance interrupt is level-triggered.
>=20
> So, as above:
>=20
> 1) A guest can deliberately cause information to be leaked to itself via
>    the corrupted GPRs. I haven't seen any rationale for why that is not
>    a problem, nor have I seen a suggested workaround.
>=20
> 2) A guest *may* be able to trigger this while the host is running. I
>    haven't seen anything that rules this out so far.
>=20
> 3) Even in the absence of virtualization, it would be necessary to
>    workaround this for *every* level-triggered interrupt, which includes
>    at the timer, PMU, and GIC maintenance interrupts, in addition to any
>    other configurable PPIs or SPIs.
>=20
> Without a fix that covers all of those, I don't think the workaround is v=
iable.

This patch covers workaround for ARM arch timer in non-virtualized cases.

While we are considering different scenarios which can trigger the issue.
After discussing with HW folks internally we have come to a conclusion that=
 there is no=20
single workaround which will fix all the scenarios. The host timer interrup=
t workaround is
different from virtualization and from other interrupt sources.

While we are working on other workarounds, we want to push timer workaround=
 first
as currently that's the one customers are encountering right now and want a=
 upstream
accepted patch soon. Other workarounds will take time to test and qualify.

Wrt drivers disabling the interrupt, except changing the driver, we don't s=
ee any common
place where we can add a workaround. Please let me your take on this.

Thanks
-Bharat

>=20
> Thanks,
> Mark.