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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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