[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:14:49 +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=ZwwmA0HvTWsuz+QVtXEbHROxSwKkJ99daUm8VeuBw8E=; b=lx8XMKY59xF4YeEV+wohZTP+5zz+V+6z2gh/wP9VCitcjBuDyQWeF9jwjEReQOZZu0pXhvAP52Kru/GRF/WpJiCZ02wNdiNPV0O6ELB9eL2W+BzSH5Ha9vVTqmvvYIr6M7coqCDHAkgLA54Z+iMzZrEzHrUAv1zJSdcCdTm4lXCwEpIjZ12gC3LIlyQTvWFasl2SqXtec1p3F5hwCmhYD6mKgpnPZZOa7UVOGyCu/6rOHOv0IIaxjMP6GELG2BGNRIQGmmxF4espN4B8vrEiKOfPJBl3Pms7QYbjVi017TW8Dt+1dHuNtK9TDFVz97n+PJ95bgusyu28Q2QaXfs3tA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=fiKdshi48p+4kzAo+jekP9cio4M7oT2ZWrH3cPMBBUVWoW5Q77u+rgnph96mz3HHDEV8zAoU6f+gc4MOSxWXu+VmdhXEcEw8cgYq7RSrdbWoh4y1GEKceHoaomjtYc33NlyunsLllLW0mbK9KqAe9ZsNC+/2GrmNuCOlJegBd2rc8JtikzixpNPjC//DpLZMBElEPnsOMQv++qWdnrYPxNZYo2RjZ63pb2gl7Q+E+HMRflwTnyzN4Ujo2vQtmFsvSUzLokjKPjzvNMFR8xR5AqA1vHII0HdslszMYlBEFosu5LRpyDgot9F9s4lczS+aXkWLGyUGreOKxE2jZuE+/g==
  • 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:15:09 +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/cBZB5l3jZkmpNH6hbDKrc6oiIY2AgAAasgCAAIUfgIAACBWAgAADOgCAAAFfAIAAAhyA
  • Thread-topic: [PATCH v2] xen/arm: do not read MVFR2 when is not defined

Hi,

> On 11 Jan 2021, at 19:07, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 11/01/2021 19:02, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> 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)
> 
> Hmmm.... Cavium may not the only platform where AArch32 is not supported.
> So as the values are actually UNKOWN (or UNDEF or Cavium), then there is no 
> point to read them.
> 
> Therefore the following pseudo-code should be enough:
> 
> if ( aarch32 supported )
>  read AArch32 ID registers
> 
> This will nicely solve the UNDEF on Cavium without adding more workaround in 
> the code :).

Works for me.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall




 


Rackspace

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