[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2] xen/arm: do not read MVFR2 when is not defined


  • To: Julien Grall <julien@xxxxxxx>
  • From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • Date: Mon, 11 Jan 2021 19:02:22 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=7xzt0osNr+WBVnFNzSOloMmM4TStk3mixaVvrOwFjpY=; b=UKRuN36k1kdVclOUzB7g0Hh+84hnuEcaH/eqvYQSziGeaPyctb8dch9ohZCZTelx339KZTW+MvkzlYS1GOrqqh7eXm4pw/5OBaD9cpE9W/ybyiIyZEAAcWbwkF3V9xqTAM4IRfk1e4PV84lSXIBYDmyMTx9uLGZWLOhVZnefDwGNMiAcmXtqUQYUJSqbbgDWCuQ6OSw2gKdoM5jRBNdn/dqgvfp9r3lEpouwAgikjIW3lBJwZ9OaqSSWJHUOZE/9qR1Qi6bQWtCm1QM4JlY+S2Y6LMaeIRB8yJAcq+b6h9hiwT560mr10UrmlZBYQlP0hy0qel+RahhT4cT3uAx97Q==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Ki8XZ8Rv0nNjwZx1zBrkFp33S/172ZzX9Cl0UkI4Ot8yqa+kZzp+ZAj/vKpbsYyoO5T3XRMvjVosFtvatIWGR+PKQuGveiIcMMeRbh3z8CeouSU5prAPvFLjY5kCTHGQcokxPtjd7MtbSgavkIf43QOnEDToicjUco7VMnXHLO+hiA9dwPppZzbiFlwW6QnbpV5IKOl0XRLvNSMEFeZVkIjwzxFm5k5gaPsOjChERwq3eK97oCOLAoKi8+KRfV86wcxRISPptW9wYipCNzzXy5RHrBxzHvo/PIQNO7RphLqg7d+fpMP/hYMAH4Cfl9cWLE74yREG/N04hUoXPHExKw==
  • Authentication-results-original: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=arm.com;
  • Cc: Jan Beulich <jbeulich@xxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, "Volodymyr_Babchuk@xxxxxxxx" <Volodymyr_Babchuk@xxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andre Przywara <Andre.Przywara@xxxxxxx>
  • Delivery-date: Mon, 11 Jan 2021 19:02:41 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHW5fO/cBZB5l3jZkmpNH6hbDKrc6oiIY2AgAAasgCAAIUfgIAACBWAgAADOgA=
  • Thread-topic: [PATCH v2] xen/arm: do not read MVFR2 when is not defined

Hi Julien,

> On 11 Jan 2021, at 18:50, Julien Grall <julien@xxxxxxx> wrote:
> 
> On 11/01/2021 18:21, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>> Sorry for the delay but I was on holiday until today.
> 
> Welcome back! No worries.
> 
>>> On 11 Jan 2021, at 10:25, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> Hi Jan,
>>> 
>>> On 11/01/2021 08:49, Jan Beulich wrote:
>>>> On 08.01.2021 20:22, Stefano Stabellini wrote:
>>>>> MVFR2 is not available on ARMv7. It is available on ARMv8 aarch32 and
>>>>> aarch64. If Xen reads MVFR2 on ARMv7 it could crash.
>>>>> 
>>>>> Avoid the issue by doing the following:
>>>>> 
>>>>> - define MVFR2_MAYBE_UNDEFINED on arm32
>>>>> - if MVFR2_MAYBE_UNDEFINED, do not attempt to read MVFR2 in Xen
>>>>> - keep the 3rd register_t in struct cpuinfo_arm.mvfr on arm32 so that a
>>>>>   guest read to the register returns '0' instead of crashing the guest.
>>>>> 
>>>>> '0' is an appropriate value to return to the guest because it is defined
>>>>> as "no support for miscellaneous features".
>>>>> 
>>>>> Aarch64 Xen is not affected by this patch.
>>>> But it looks to also be affected by ...
>>> 
>>> AFAICT, the smoke test passed on Laxton0 (AMD Seattle) [1] over the 
>>> week-end.
>>> 
>>>>> Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
>>>> ... this, faulting (according to osstest logs) early during boot on
>>> 
>>> The xen-unstable flight [2] ran on Rochester0 (Cavium Thunder-X). So this 
>>> has something to do with the platform.
>>> 
>>> The main difference is AMD Seattle supports AArch32 while Cavium Thunder-X 
>>> doesn't.
>>> 
>>>> 000000000025D314   mrs     x1, id_pfr2_el1
>>> This register contains information for the AArch32 state.
>>> 
>>> AFAICT, the Arm Arm back to at least ARM DDI 0487A.j (published in 2016) 
>>> described the encoding as Read-Only. So I am not sure why we receive an 
>>> UNDEF here, the more it looks like ID_PFR{0, 1}_EL1 were correctly accessed.
>>> 
>>> Andre, Bertrand, do you have any clue?
>> I will double check this but my understanding when I checked this was that 
>> it would be possible to read with an unknown value but should not generate 
>> an UNDEF.
>>> 
>>> However, most of the AArch32 ID registers are UNKNOWN on platform not 
>>> implementing AArch32. So we may want to conditionally skip the access to 
>>> AArch32 state.
>> We could skip aarch32 registers on platforms not supporting aarch32 but we 
>> will still have to provide values to a guest trying to access them so might 
>> be better to return what is returned by the hardware.
> 
> Per the Arm Arm, the value of the registers may changed at any time. IOW, two 
> read of the sytem registers may return different values.
> 
> IIRC, the original intent of the series was to provide sanitized value of the 
> ID registers. So I think it would be unwise to let the guest using the values.
> 
> Instead, I would suggest to implement them as RAZ.

Works for me.

> 
>> Now if some platforms are generating an UNDEF we need to understand in what 
>> cases and behave the same way for the guest.
> 
> I am not entirely sure what you mean by platforms here.
> 
> If you mean any platform conforming with the Arm Arm, then I agree with your 
> statement.
> 
> However, if you refer to platform that may not follow the Arm Arm, then I 
> disagree. We should try to expose a sane interface to the guest whenever it 
> is possible.
> 
> In this case, I would bet the hardware would not even allow us to trap the 
> ID_PFR2. Although, I haven't tried it.
> 
>> Do i understand it right that on Cavium which has no aarch32 support the 
>> access is generating an UNDEF ?
> 
> Yes. The UNDEF will happen when trying to read ID_PFR2_EL1. Interestingly, it 
> doesn't happen when reading ID_PFR{0, 1}_EL1. So this smells like a silicon 
> bug.

Sounds like the ifdef ARM64 should be something like if (!cavium)

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.