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

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

[Stefano Stabellini]

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.

Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 xen/arch/arm/cpufeature.c           | 2 ++
 xen/include/asm-arm/arm32/sysregs.h | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
index 1f6a85aafe..698bfa0201 100644
--- a/xen/arch/arm/cpufeature.c
+++ b/xen/arch/arm/cpufeature.c
@@ -150,7 +150,9 @@ void identify_cpu(struct cpuinfo_arm *c)
 
         c->mvfr.bits[0] = READ_SYSREG(MVFR0_EL1);
         c->mvfr.bits[1] = READ_SYSREG(MVFR1_EL1);
+#ifndef MVFR2_MAYBE_UNDEFINED
         c->mvfr.bits[2] = READ_SYSREG(MVFR2_EL1);
+#endif
 }
 
 /*
diff --git a/xen/include/asm-arm/arm32/sysregs.h b/xen/include/asm-arm/arm32/sysregs.h
index 25cdcbfa4e..6841d5de43 100644
--- a/xen/include/asm-arm/arm32/sysregs.h
+++ b/xen/include/asm-arm/arm32/sysregs.h
@@ -62,6 +62,9 @@
 #define READ_SYSREG(R...)       READ_SYSREG32(R)
 #define WRITE_SYSREG(V, R...)   WRITE_SYSREG32(V, R)
 
+/* MVFR2 is not defined on ARMv7 */
+#define MVFR2_MAYBE_UNDEFINED
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __ASM_ARM_ARM32_SYSREGS_H */
-- 
2.17.1

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

[Julien Grall]

Hi Stefano,

On 08/01/2021 19: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.
> 
> Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

Acked-by: Julien Grall <jgrall@amazon.com>

With one question below:

> ---
>   xen/arch/arm/cpufeature.c           | 2 ++
>   xen/include/asm-arm/arm32/sysregs.h | 3 +++
>   2 files changed, 5 insertions(+)
> 
> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> index 1f6a85aafe..698bfa0201 100644
> --- a/xen/arch/arm/cpufeature.c
> +++ b/xen/arch/arm/cpufeature.c
> @@ -150,7 +150,9 @@ void identify_cpu(struct cpuinfo_arm *c)
>   
>           c->mvfr.bits[0] = READ_SYSREG(MVFR0_EL1);
>           c->mvfr.bits[1] = READ_SYSREG(MVFR1_EL1);
> +#ifndef MVFR2_MAYBE_UNDEFINED
>           c->mvfr.bits[2] = READ_SYSREG(MVFR2_EL1);
> +#endif

Is there any guarantee that c->mvfr.bits[2] will be zeroed by default?

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

On Fri, 8 Jan 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 08/01/2021 19: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.
> > 
> > Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> With one question below:

Thanks!


> >   xen/arch/arm/cpufeature.c           | 2 ++
> >   xen/include/asm-arm/arm32/sysregs.h | 3 +++
> >   2 files changed, 5 insertions(+)
> > 
> > diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> > index 1f6a85aafe..698bfa0201 100644
> > --- a/xen/arch/arm/cpufeature.c
> > +++ b/xen/arch/arm/cpufeature.c
> > @@ -150,7 +150,9 @@ void identify_cpu(struct cpuinfo_arm *c)
> >             c->mvfr.bits[0] = READ_SYSREG(MVFR0_EL1);
> >           c->mvfr.bits[1] = READ_SYSREG(MVFR1_EL1);
> > +#ifndef MVFR2_MAYBE_UNDEFINED
> >           c->mvfr.bits[2] = READ_SYSREG(MVFR2_EL1);
> > +#endif
> 
> Is there any guarantee that c->mvfr.bits[2] will be zeroed by default?

It is coming from one of the following:

- xen/arch/arm/setup.c: struct cpuinfo_arm __read_mostly boot_cpu_data;
- xen/arch/arm/smpboot.c: struct cpuinfo_arm cpu_data[NR_CPUS];

Both are global variables so they should be both zeroed.

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

[Jan Beulich]

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 ...

> Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")

... this, faulting (according to osstest logs) early during boot on

000000000025D314	mrs	x1, id_pfr2_el1

afaict.

Jan

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

[Julien Grall]

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?

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.

Cheers,

[1] 
http://logs.test-lab.xenproject.org/osstest/logs/158293/test-arm64-arm64-xl-xsm/info.html

> 
> Jan
> 

[1]


-- 
Julien Grall

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

[Bertrand Marquis]

Hi Julien,

Sorry for the delay but I was on holiday until today.

> On 11 Jan 2021, at 10:25, Julien Grall <julien@xen.org> 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.
Now if some platforms are generating an UNDEF we need to understand in what cases and behave the same way for the guest.

Do i understand it right that on Cavium which has no aarch32 support the access is generating an UNDEF ?

Cheers
Bertrand

> 
> Cheers,
> 
> [1] http://logs.test-lab.xenproject.org/osstest/logs/158293/test-arm64-arm64-xl-xsm/info.html
> 
>> Jan
> 
> [1]
> 
> 
> -- 
> Julien Grall

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

[Julien Grall]

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@xen.org> 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.

> 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.

Cheers,

-- 
Julien Grall

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

[Bertrand Marquis]

Hi Julien,

> On 11 Jan 2021, at 18:50, Julien Grall <julien@xen.org> 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@xen.org> 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

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

[Julien Grall]

On 11/01/2021 19:02, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

> 
>> On 11 Jan 2021, at 18:50, Julien Grall <julien@xen.org> 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@xen.org> 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 :).

Cheers,

-- 
Julien Grall

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

[Bertrand Marquis]

Hi,

> On 11 Jan 2021, at 19:07, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 11/01/2021 19:02, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 11 Jan 2021, at 18:50, Julien Grall <julien@xen.org> 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@xen.org> 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