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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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