xen/arch/arm/cpufeature.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-)
Don't read aarch32 system registers at boot time when the aarch32 state
is not available. They are UNKNOWN, so it is not useful to read them.
Moreover, on Cavium ThunderX reading ID_PFR2_EL1 causes a Xen crash.
Instead, only read them when aarch32 is available.
Leave the corresponding fields in struct cpuinfo_arm so that they
are read-as-zero from a guest.
Since we are editing identify_cpu, also fix the indentation: 4 spaces
instead of 8.
Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
Link: https://marc.info/?l=xen-devel&m=161035501118086
Link: http://logs.test-lab.xenproject.org/osstest/logs/158293/test-arm64-arm64-xl-xsm/info.html
Suggested-by: Julien Grall <julien@xen.org>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
xen/arch/arm/cpufeature.c | 35 +++++++++++++++++++++--------------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
index 698bfa0201..b1c82ade49 100644
--- a/xen/arch/arm/cpufeature.c
+++ b/xen/arch/arm/cpufeature.c
@@ -101,29 +101,35 @@ int enable_nonboot_cpu_caps(const struct arm_cpu_capabilities *caps)
void identify_cpu(struct cpuinfo_arm *c)
{
- c->midr.bits = READ_SYSREG(MIDR_EL1);
- c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
+ bool aarch32 = true;
+
+ c->midr.bits = READ_SYSREG(MIDR_EL1);
+ c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
#ifdef CONFIG_ARM_64
- c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
- c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
+ c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
+ c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
+
+ c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
+ c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
- c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
- c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
+ c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
+ c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
- c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
- c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
+ c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
+ c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
+ c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
- c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
- c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
- c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
+ c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
+ c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
- c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
- c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
+ c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
- c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
+ aarch32 = c->pfr64.el1 == 2;
#endif
+ if ( aarch32 )
+ {
c->pfr32.bits[0] = READ_SYSREG(ID_PFR0_EL1);
c->pfr32.bits[1] = READ_SYSREG(ID_PFR1_EL1);
c->pfr32.bits[2] = READ_SYSREG(ID_PFR2_EL1);
@@ -153,6 +159,7 @@ void identify_cpu(struct cpuinfo_arm *c)
#ifndef MVFR2_MAYBE_UNDEFINED
c->mvfr.bits[2] = READ_SYSREG(MVFR2_EL1);
#endif
+ }
}
/*
--
2.17.1
Hi Stefano,
On 12/01/2021 00:16, Stefano Stabellini wrote:
> Don't read aarch32 system registers at boot time when the aarch32 state
> is not available. They are UNKNOWN, so it is not useful to read them.
> Moreover, on Cavium ThunderX reading ID_PFR2_EL1 causes a Xen crash.
> Instead, only read them when aarch32 is available.
AArch32 may be supported in EL0 but not in EL1. So I think you want to
clarify in the commit message/title which EL you are referring to.
>
> Leave the corresponding fields in struct cpuinfo_arm so that they
> are read-as-zero from a guest.
>
> Since we are editing identify_cpu, also fix the indentation: 4 spaces
> instead of 8.
I was going to ask to split that in a separate patch. But then, I
noticed that it avoids to change the indentation of the if body twice.
So I am ok with that.
>
> Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
> Link: https://marc.info/?l=xen-devel&m=161035501118086
NIT: I would suggest to use lore.kernel.org just because the link
contains the message-id. So if the website goes down, it is still
possible to track the original discussion.
> Link: http://logs.test-lab.xenproject.org/osstest/logs/158293/test-arm64-arm64-xl-xsm/info.html
IIRC we only keep the logs around for a couple of weeks. So this is
going to be break quickly. Therefore, I would suggest to remove this link.
> Suggested-by: Julien Grall <julien@xen.org>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> xen/arch/arm/cpufeature.c | 35 +++++++++++++++++++++--------------
> 1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> index 698bfa0201..b1c82ade49 100644
> --- a/xen/arch/arm/cpufeature.c
> +++ b/xen/arch/arm/cpufeature.c
> @@ -101,29 +101,35 @@ int enable_nonboot_cpu_caps(const struct arm_cpu_capabilities *caps)
>
> void identify_cpu(struct cpuinfo_arm *c)
> {
> - c->midr.bits = READ_SYSREG(MIDR_EL1);
> - c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
> + bool aarch32 = true;
> +
> + c->midr.bits = READ_SYSREG(MIDR_EL1);
> + c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>
> #ifdef CONFIG_ARM_64
> - c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
> - c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
> + c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
> + c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
> +
> + c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
> + c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>
> - c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
> - c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
> + c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
> + c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>
> - c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
> - c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
> + c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
> + c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
> + c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
>
> - c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
> - c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
> - c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
> + c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
> + c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>
> - c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
> - c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
> + c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>
> - c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
> + aarch32 = c->pfr64.el1 == 2;
This is checking that AArch32 is available in EL1. However, it may not
be the case yet it would be available in EL0.
As a consequence, 32-bit userspace wouldn't work properly after this patch.
The Arm Arm mandates that if AArch32 is available at EL(n), then it must
be available at EL(n - 1).
So we should check that AArch32 is available at EL0 because this would
cover the case where AArch32 is enabled at EL1.
Furthermore, I would also like to avoid hardcoding value in the code as
it is less readable. We already define cpu_has_el0_32 which use the boot
CPU feature. Maybe we want to expand the macro or split it?
> #endif
>
> + if ( aarch32 )
I read this check as "If AArch32 is not available at any EL". But you
are checking whether it is available at a given level. So I would
suggest to suffix with the EL for clarification.
In this case, I think you will want to call it aarch32_el0.
> + {
> c->pfr32.bits[0] = READ_SYSREG(ID_PFR0_EL1);
> c->pfr32.bits[1] = READ_SYSREG(ID_PFR1_EL1);
> c->pfr32.bits[2] = READ_SYSREG(ID_PFR2_EL1);
> @@ -153,6 +159,7 @@ void identify_cpu(struct cpuinfo_arm *c)
> #ifndef MVFR2_MAYBE_UNDEFINED
> c->mvfr.bits[2] = READ_SYSREG(MVFR2_EL1);
> #endif
> + }
> }
>
> /*
>
Cheers,
--
Julien Grall
Hello Julien,
> On 12 Jan 2021, at 11:00 am, Julien Grall <julien@xen.org> wrote:
>
> Hi Stefano,
>
> On 12/01/2021 00:16, Stefano Stabellini wrote:
>> Don't read aarch32 system registers at boot time when the aarch32 state
>> is not available. They are UNKNOWN, so it is not useful to read them.
>> Moreover, on Cavium ThunderX reading ID_PFR2_EL1 causes a Xen crash.
>> Instead, only read them when aarch32 is available.
> AArch32 may be supported in EL0 but not in EL1. So I think you want to clarify in the commit message/title which EL you are referring to.
>
>> Leave the corresponding fields in struct cpuinfo_arm so that they
>> are read-as-zero from a guest.
>> Since we are editing identify_cpu, also fix the indentation: 4 spaces
>> instead of 8.
>
> I was going to ask to split that in a separate patch. But then, I noticed that it avoids to change the indentation of the if body twice. So I am ok with that.
>
>> Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
>> Link: https://marc.info/?l=xen-devel&m=161035501118086
>
> NIT: I would suggest to use lore.kernel.org just because the link contains the message-id. So if the website goes down, it is still possible to track the original discussion.
>
>> Link: http://logs.test-lab.xenproject.org/osstest/logs/158293/test-arm64-arm64-xl-xsm/info.html
>
> IIRC we only keep the logs around for a couple of weeks. So this is going to be break quickly. Therefore, I would suggest to remove this link.
>
>> Suggested-by: Julien Grall <julien@xen.org>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>> ---
>> xen/arch/arm/cpufeature.c | 35 +++++++++++++++++++++--------------
>> 1 file changed, 21 insertions(+), 14 deletions(-)
>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
>> index 698bfa0201..b1c82ade49 100644
>> --- a/xen/arch/arm/cpufeature.c
>> +++ b/xen/arch/arm/cpufeature.c
>> @@ -101,29 +101,35 @@ int enable_nonboot_cpu_caps(const struct arm_cpu_capabilities *caps)
>> void identify_cpu(struct cpuinfo_arm *c)
>> {
>> - c->midr.bits = READ_SYSREG(MIDR_EL1);
>> - c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>> + bool aarch32 = true;
>> +
>> + c->midr.bits = READ_SYSREG(MIDR_EL1);
>> + c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>> #ifdef CONFIG_ARM_64
>> - c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
>> - c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
>> + c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
>> + c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
>> +
>> + c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
>> + c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>> - c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
>> - c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>> + c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
>> + c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>> - c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
>> - c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>> + c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
>> + c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
>> + c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
>> - c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
>> - c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
>> - c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
>> + c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
>> + c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>> - c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
>> - c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>> + c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>> - c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>> + aarch32 = c->pfr64.el1 == 2;
>
> This is checking that AArch32 is available in EL1. However, it may not be the case yet it would be available in EL0.
As per my understanding please correct me if I am wrong, if AArch32 is allowed at an EL, it must be allowed all lower Exception levels.
For example, if EL3 allows AArch32, then it must be allowed at all lower Exception levels.That means if we are checking the EL1 for AArch32 EL0 should also support AArch32.
I think "aarch32 = c->pfr64.el1 == 2” is correct to check.
Regards,
Rahul
> As a consequence, 32-bit userspace wouldn't work properly after this patch.
>
> The Arm Arm mandates that if AArch32 is available at EL(n), then it must be available at EL(n - 1).
>
> So we should check that AArch32 is available at EL0 because this would
> cover the case where AArch32 is enabled at EL1.
>
> Furthermore, I would also like to avoid hardcoding value in the code as it is less readable. We already define cpu_has_el0_32 which use the boot CPU feature. Maybe we want to expand the macro or split it?
>
>> #endif
>> + if ( aarch32 )
> I read this check as "If AArch32 is not available at any EL". But you are checking whether it is available at a given level. So I would suggest to suffix with the EL for clarification.
>
> In this case, I think you will want to call it aarch32_el0.
>
>> + {
>> c->pfr32.bits[0] = READ_SYSREG(ID_PFR0_EL1);
>> c->pfr32.bits[1] = READ_SYSREG(ID_PFR1_EL1);
>> c->pfr32.bits[2] = READ_SYSREG(ID_PFR2_EL1);
>> @@ -153,6 +159,7 @@ void identify_cpu(struct cpuinfo_arm *c)
>> #ifndef MVFR2_MAYBE_UNDEFINED
>> c->mvfr.bits[2] = READ_SYSREG(MVFR2_EL1);
>> #endif
>> + }
>> }
>> /*
>
> Cheers,
>
> --
> Julien Grall
On 12/01/2021 13:28, Rahul Singh wrote:
> Hello Julien,
Hi Rahul,
>
>> On 12 Jan 2021, at 11:00 am, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Stefano,
>>
>> On 12/01/2021 00:16, Stefano Stabellini wrote:
>>> Don't read aarch32 system registers at boot time when the aarch32 state
>>> is not available. They are UNKNOWN, so it is not useful to read them.
>>> Moreover, on Cavium ThunderX reading ID_PFR2_EL1 causes a Xen crash.
>>> Instead, only read them when aarch32 is available.
>> AArch32 may be supported in EL0 but not in EL1. So I think you want to clarify in the commit message/title which EL you are referring to.
>>
>>> Leave the corresponding fields in struct cpuinfo_arm so that they
>>> are read-as-zero from a guest.
>>> Since we are editing identify_cpu, also fix the indentation: 4 spaces
>>> instead of 8.
>>
>> I was going to ask to split that in a separate patch. But then, I noticed that it avoids to change the indentation of the if body twice. So I am ok with that.
>>
>>> Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
>>> Link: https://marc.info/?l=xen-devel&m=161035501118086
>>
>> NIT: I would suggest to use lore.kernel.org just because the link contains the message-id. So if the website goes down, it is still possible to track the original discussion.
>>
>>> Link: http://logs.test-lab.xenproject.org/osstest/logs/158293/test-arm64-arm64-xl-xsm/info.html
>>
>> IIRC we only keep the logs around for a couple of weeks. So this is going to be break quickly. Therefore, I would suggest to remove this link.
>>
>>> Suggested-by: Julien Grall <julien@xen.org>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>> ---
>>> xen/arch/arm/cpufeature.c | 35 +++++++++++++++++++++--------------
>>> 1 file changed, 21 insertions(+), 14 deletions(-)
>>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
>>> index 698bfa0201..b1c82ade49 100644
>>> --- a/xen/arch/arm/cpufeature.c
>>> +++ b/xen/arch/arm/cpufeature.c
>>> @@ -101,29 +101,35 @@ int enable_nonboot_cpu_caps(const struct arm_cpu_capabilities *caps)
>>> void identify_cpu(struct cpuinfo_arm *c)
>>> {
>>> - c->midr.bits = READ_SYSREG(MIDR_EL1);
>>> - c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>>> + bool aarch32 = true;
>>> +
>>> + c->midr.bits = READ_SYSREG(MIDR_EL1);
>>> + c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>>> #ifdef CONFIG_ARM_64
>>> - c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
>>> - c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
>>> + c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
>>> + c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
>>> +
>>> + c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
>>> + c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>>> - c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
>>> - c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>>> + c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
>>> + c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>>> - c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
>>> - c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>>> + c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
>>> + c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
>>> + c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
>>> - c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
>>> - c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
>>> - c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
>>> + c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
>>> + c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>>> - c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
>>> - c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>>> + c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>>> - c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>>> + aarch32 = c->pfr64.el1 == 2;
>>
>> This is checking that AArch32 is available in EL1. However, it may not be the case yet it would be available in EL0.
>
> As per my understanding please correct me if I am wrong, if AArch32 is allowed at an EL, it must be allowed all lower Exception levels.
This statement is correct.
>
> For example, if EL3 allows AArch32, then it must be allowed at all lower Exception levels.That means if we are checking the EL1 for AArch32 EL0 should also support AArch32.
> I think "aarch32 = c->pfr64.el1 == 2” is correct to check.
I agree that if EL1 supports AArch32, then it means EL0 will not support.
However, if EL1 doesn't support AArch32, then it doesn't imply that EL0
will not support AArch32.
Therefore, the check suggested would not be correct because it would
prevent 32-bit userspace running on HW (such as IIRC Cortex-A76) that
only support AArch32 in EL0.
Cheers,
--
Julien Grall
Hi,
> On 12 Jan 2021, at 14:27, Julien Grall <julien@xen.org> wrote:
>
>
>
> On 12/01/2021 13:28, Rahul Singh wrote:
>> Hello Julien,
>
> Hi Rahul,
>
>>> On 12 Jan 2021, at 11:00 am, Julien Grall <julien@xen.org> wrote:
>>>
>>> Hi Stefano,
>>>
>>> On 12/01/2021 00:16, Stefano Stabellini wrote:
>>>> Don't read aarch32 system registers at boot time when the aarch32 state
>>>> is not available. They are UNKNOWN, so it is not useful to read them.
>>>> Moreover, on Cavium ThunderX reading ID_PFR2_EL1 causes a Xen crash.
>>>> Instead, only read them when aarch32 is available.
>>> AArch32 may be supported in EL0 but not in EL1. So I think you want to clarify in the commit message/title which EL you are referring to.
>>>
>>>> Leave the corresponding fields in struct cpuinfo_arm so that they
>>>> are read-as-zero from a guest.
>>>> Since we are editing identify_cpu, also fix the indentation: 4 spaces
>>>> instead of 8.
>>>
>>> I was going to ask to split that in a separate patch. But then, I noticed that it avoids to change the indentation of the if body twice. So I am ok with that.
>>>
>>>> Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
>>>> Link: https://marc.info/?l=xen-devel&m=161035501118086
>>>
>>> NIT: I would suggest to use lore.kernel.org just because the link contains the message-id. So if the website goes down, it is still possible to track the original discussion.
>>>
>>>> Link: http://logs.test-lab.xenproject.org/osstest/logs/158293/test-arm64-arm64-xl-xsm/info.html
>>>
>>> IIRC we only keep the logs around for a couple of weeks. So this is going to be break quickly. Therefore, I would suggest to remove this link.
>>>
>>>> Suggested-by: Julien Grall <julien@xen.org>
>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>>> ---
>>>> xen/arch/arm/cpufeature.c | 35 +++++++++++++++++++++--------------
>>>> 1 file changed, 21 insertions(+), 14 deletions(-)
>>>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
>>>> index 698bfa0201..b1c82ade49 100644
>>>> --- a/xen/arch/arm/cpufeature.c
>>>> +++ b/xen/arch/arm/cpufeature.c
>>>> @@ -101,29 +101,35 @@ int enable_nonboot_cpu_caps(const struct arm_cpu_capabilities *caps)
>>>> void identify_cpu(struct cpuinfo_arm *c)
>>>> {
>>>> - c->midr.bits = READ_SYSREG(MIDR_EL1);
>>>> - c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>>>> + bool aarch32 = true;
>>>> +
>>>> + c->midr.bits = READ_SYSREG(MIDR_EL1);
>>>> + c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>>>> #ifdef CONFIG_ARM_64
>>>> - c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
>>>> - c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
>>>> + c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
>>>> + c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
>>>> +
>>>> + c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
>>>> + c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>>>> - c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
>>>> - c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>>>> + c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
>>>> + c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>>>> - c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
>>>> - c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>>>> + c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
>>>> + c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
>>>> + c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
>>>> - c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
>>>> - c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
>>>> - c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
>>>> + c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
>>>> + c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>>>> - c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
>>>> - c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>>>> + c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>>>> - c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>>>> + aarch32 = c->pfr64.el1 == 2;
>>>
>>> This is checking that AArch32 is available in EL1. However, it may not be the case yet it would be available in EL0.
>> As per my understanding please correct me if I am wrong, if AArch32 is allowed at an EL, it must be allowed all lower Exception levels.
>
> This statement is correct.
>
>> For example, if EL3 allows AArch32, then it must be allowed at all lower Exception levels.That means if we are checking the EL1 for AArch32 EL0 should also support AArch32.
>> I think "aarch32 = c->pfr64.el1 == 2” is correct to check.
>
> I agree that if EL1 supports AArch32, then it means EL0 will not support.
>
> However, if EL1 doesn't support AArch32, then it doesn't imply that EL0 will not support AArch32.
>
> Therefore, the check suggested would not be correct because it would prevent 32-bit userspace running on HW (such as IIRC Cortex-A76) that only support AArch32 in EL0.
I agree with this, we should check aarch32 at EL0 as if it is not supported there it is not supported at all and the the registers should not be needed.
If it is supported in EL0, wether or not it is supported at lower level, we need to read those registers.
So we should check pfr64.el0 instead.
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
On Tue, 12 Jan 2021, Julien Grall wrote: > > + aarch32 = c->pfr64.el1 == 2; > > This is checking that AArch32 is available in EL1. However, it may not be the > case yet it would be available in EL0. > > As a consequence, 32-bit userspace wouldn't work properly after this patch. > > The Arm Arm mandates that if AArch32 is available at EL(n), then it must be > available at EL(n - 1). > > So we should check that AArch32 is available at EL0 because this would > cover the case where AArch32 is enabled at EL1. OK > Furthermore, I would also like to avoid hardcoding value in the code as it is > less readable. We already define cpu_has_el0_32 which use the boot CPU > feature. Maybe we want to expand the macro or split it? I agree Technically, cpu_has_el0_32 works as is, because it is called after boot_cpu_data has been updated. So we could just use it. What do you think? > > #endif > > + if ( aarch32 ) > I read this check as "If AArch32 is not available at any EL". But you are > checking whether it is available at a given level. So I would suggest to > suffix with the EL for clarification. > > In this case, I think you will want to call it aarch32_el0. OK
On Tue, 12 Jan 2021 at 19:09, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Tue, 12 Jan 2021, Julien Grall wrote: > > > + aarch32 = c->pfr64.el1 == 2; > > > > This is checking that AArch32 is available in EL1. However, it may not be the > > case yet it would be available in EL0. > > > > As a consequence, 32-bit userspace wouldn't work properly after this patch. > > > > The Arm Arm mandates that if AArch32 is available at EL(n), then it must be > > available at EL(n - 1). > > > > So we should check that AArch32 is available at EL0 because this would > > cover the case where AArch32 is enabled at EL1. > > OK > > > > Furthermore, I would also like to avoid hardcoding value in the code as it is > > less readable. We already define cpu_has_el0_32 which use the boot CPU > > feature. Maybe we want to expand the macro or split it? > > I agree > > Technically, cpu_has_el0_32 works as is, because it is called after > boot_cpu_data has been updated. So we could just use it. What do you > think? I thought about that when I wrote the first e-mail. However, this would not work properly for heterogeneous platforms. There is still a risk to read garbage (or UNDEF) if the boot CPU supports AArch32 but the others. Cheers,
On Tue, 12 Jan 2021, Julien Grall wrote:
> On Tue, 12 Jan 2021 at 19:09, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >
> > On Tue, 12 Jan 2021, Julien Grall wrote:
> > > > + aarch32 = c->pfr64.el1 == 2;
> > >
> > > This is checking that AArch32 is available in EL1. However, it may not be the
> > > case yet it would be available in EL0.
> > >
> > > As a consequence, 32-bit userspace wouldn't work properly after this patch.
> > >
> > > The Arm Arm mandates that if AArch32 is available at EL(n), then it must be
> > > available at EL(n - 1).
> > >
> > > So we should check that AArch32 is available at EL0 because this would
> > > cover the case where AArch32 is enabled at EL1.
> >
> > OK
> >
> >
> > > Furthermore, I would also like to avoid hardcoding value in the code as it is
> > > less readable. We already define cpu_has_el0_32 which use the boot CPU
> > > feature. Maybe we want to expand the macro or split it?
> >
> > I agree
> >
> > Technically, cpu_has_el0_32 works as is, because it is called after
> > boot_cpu_data has been updated. So we could just use it. What do you
> > think?
>
> I thought about that when I wrote the first e-mail. However, this
> would not work properly for heterogeneous platforms.
> There is still a risk to read garbage (or UNDEF) if the boot CPU
> supports AArch32 but the others.
Yeah, but that is not the kind of thing that can be actually different
in a heterogeneous platform, as far as I am aware?
In any case, a check that takes that into consideraion would be:
aarch32_el0 = cpu_has_el0_32 &&
(boot_cpu_feature64(el0) == cpu_feature64(c, el0));
If you have something better in mind please feel free to suggest it and
I'll add it to the patch. Otherwise, I'll send it with this later today
with a note that if you want to make a change on commit you have my
blessing :-)
On Tue, 12 Jan 2021 at 21:05, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Tue, 12 Jan 2021, Julien Grall wrote: > > On Tue, 12 Jan 2021 at 19:09, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > > > On Tue, 12 Jan 2021, Julien Grall wrote: > > > > > + aarch32 = c->pfr64.el1 == 2; > > > > > > > > This is checking that AArch32 is available in EL1. However, it may not be the > > > > case yet it would be available in EL0. > > > > > > > > As a consequence, 32-bit userspace wouldn't work properly after this patch. > > > > > > > > The Arm Arm mandates that if AArch32 is available at EL(n), then it must be > > > > available at EL(n - 1). > > > > > > > > So we should check that AArch32 is available at EL0 because this would > > > > cover the case where AArch32 is enabled at EL1. > > > > > > OK > > > > > > > > > > Furthermore, I would also like to avoid hardcoding value in the code as it is > > > > less readable. We already define cpu_has_el0_32 which use the boot CPU > > > > feature. Maybe we want to expand the macro or split it? > > > > > > I agree > > > > > > Technically, cpu_has_el0_32 works as is, because it is called after > > > boot_cpu_data has been updated. So we could just use it. What do you > > > think? > > > > I thought about that when I wrote the first e-mail. However, this > > would not work properly for heterogeneous platforms. > > There is still a risk to read garbage (or UNDEF) if the boot CPU > > supports AArch32 but the others. > > Yeah, but that is not the kind of thing that can be actually different > in a heterogeneous platform, as far as I am aware? Arm CPU vendors can do a lot of interesting mix. For Samsung released in the past a big.LITTLE with a mix of Armv8.0 and Armv8.2 cores (see [1]). It turns out to be a massive blunder because they hack Linux to avoid detecting a minimum feature set. So the userspace app was trying to use LSE atomics on Armv8.0 (yes). I wouldn't be surprised to see a phone with a mix of 64-bit only processor and one with 32-bit EL0 to run legacy apps. > > In any case, a check that takes that into consideraion would be: > > aarch32_el0 = cpu_has_el0_32 && > (boot_cpu_feature64(el0) == cpu_feature64(c, el0)); Why do you want to check the boot CPU feature? The per-cpu cpuinfo should really contain a raw copy of the ID registers of that CPU. Anything else will make our life very difficult when we try to look for a common set of features or want to expose big.LITTLE to a guest (this request comes back time to time). > > If you have something better in mind please feel free to suggest it and > I'll add it to the patch. Otherwise, I'll send it with this later today > with a note that if you want to make a change on commit you have my > blessing :-) Let me start by saying that I think the existing cpu_has_* are misnamed because the name suggest it would check the features of the current CPU but they only check the boot CPU. But I am not going to suggest a renaming for now. The header cpufeature.h likely needs an overhaul. Instead I would suggest the following: Pseudo-code: #define cpu_feature64_has_el0(c) cpu_feature64(c, el0) == 2 And the the code would use: aarch32_el0 = cpu_feature64_has_el0(c); [1] https://github.com/golang/go/issues/28431
Hi Stefano,
> On 12 Jan 2021, at 00:16, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> Don't read aarch32 system registers at boot time when the aarch32 state
> is not available. They are UNKNOWN, so it is not useful to read them.
> Moreover, on Cavium ThunderX reading ID_PFR2_EL1 causes a Xen crash.
> Instead, only read them when aarch32 is available.
>
> Leave the corresponding fields in struct cpuinfo_arm so that they
> are read-as-zero from a guest.
I agree with the fact that all users of identify_cpu are currently using spaces
which are 0 but to make the core a bit more robust we could do a memset(0)
of the structure at the beginning of the function.
>
> Since we are editing identify_cpu, also fix the indentation: 4 spaces
> instead of 8.
>
> Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
> Link: https://marc.info/?l=xen-devel&m=161035501118086
> Link: http://logs.test-lab.xenproject.org/osstest/logs/158293/test-arm64-arm64-xl-xsm/info.html
> Suggested-by: Julien Grall <julien@xen.org>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> xen/arch/arm/cpufeature.c | 35 +++++++++++++++++++++--------------
> 1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> index 698bfa0201..b1c82ade49 100644
> --- a/xen/arch/arm/cpufeature.c
> +++ b/xen/arch/arm/cpufeature.c
> @@ -101,29 +101,35 @@ int enable_nonboot_cpu_caps(const struct arm_cpu_capabilities *caps)
>
> void identify_cpu(struct cpuinfo_arm *c)
> {
> - c->midr.bits = READ_SYSREG(MIDR_EL1);
> - c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
> + bool aarch32 = true;
> +
> + c->midr.bits = READ_SYSREG(MIDR_EL1);
> + c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>
> #ifdef CONFIG_ARM_64
> - c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
> - c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
> + c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
> + c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
> +
> + c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
> + c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>
> - c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
> - c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
> + c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
> + c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>
> - c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
> - c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
> + c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
> + c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
> + c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
>
> - c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
> - c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
> - c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
> + c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
> + c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>
> - c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
> - c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
> + c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>
> - c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
> + aarch32 = c->pfr64.el1 == 2;
I would put some () around the test.
> #endif
>
> + if ( aarch32 )
> + {
> c->pfr32.bits[0] = READ_SYSREG(ID_PFR0_EL1);
> c->pfr32.bits[1] = READ_SYSREG(ID_PFR1_EL1);
> c->pfr32.bits[2] = READ_SYSREG(ID_PFR2_EL1);
> @@ -153,6 +159,7 @@ void identify_cpu(struct cpuinfo_arm *c)
> #ifndef MVFR2_MAYBE_UNDEFINED
> c->mvfr.bits[2] = READ_SYSREG(MVFR2_EL1);
> #endif
If we test for aarch32, the ifndef here should not be needed anymore.
Is your previous patch really still needed if we go with the aarch32 path ?
Cheers
Bertrand
> + }
> }
>
> /*
> --
> 2.17.1
>
On 12/01/2021 10:50, Bertrand Marquis wrote:
> Hi Stefano,
>
>> On 12 Jan 2021, at 00:16, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>
>> Don't read aarch32 system registers at boot time when the aarch32 state
>> is not available. They are UNKNOWN, so it is not useful to read them.
>> Moreover, on Cavium ThunderX reading ID_PFR2_EL1 causes a Xen crash.
>> Instead, only read them when aarch32 is available.
>>
>> Leave the corresponding fields in struct cpuinfo_arm so that they
>> are read-as-zero from a guest.
>
> I agree with the fact that all users of identify_cpu are currently using spaces
> which are 0 but to make the core a bit more robust we could do a memset(0)
> of the structure at the beginning of the function.
>
>>
>> Since we are editing identify_cpu, also fix the indentation: 4 spaces
>> instead of 8.
>>
>> Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
>> Link: https://marc.info/?l=xen-devel&m=161035501118086
>> Link: http://logs.test-lab.xenproject.org/osstest/logs/158293/test-arm64-arm64-xl-xsm/info.html
>> Suggested-by: Julien Grall <julien@xen.org>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>> ---
>> xen/arch/arm/cpufeature.c | 35 +++++++++++++++++++++--------------
>> 1 file changed, 21 insertions(+), 14 deletions(-)
>>
>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
>> index 698bfa0201..b1c82ade49 100644
>> --- a/xen/arch/arm/cpufeature.c
>> +++ b/xen/arch/arm/cpufeature.c
>> @@ -101,29 +101,35 @@ int enable_nonboot_cpu_caps(const struct arm_cpu_capabilities *caps)
>>
>> void identify_cpu(struct cpuinfo_arm *c)
>> {
>> - c->midr.bits = READ_SYSREG(MIDR_EL1);
>> - c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>> + bool aarch32 = true;
>> +
>> + c->midr.bits = READ_SYSREG(MIDR_EL1);
>> + c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>>
>> #ifdef CONFIG_ARM_64
>> - c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
>> - c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
>> + c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
>> + c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
>> +
>> + c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
>> + c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>>
>> - c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
>> - c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>> + c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
>> + c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>>
>> - c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
>> - c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>> + c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
>> + c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
>> + c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
>>
>> - c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
>> - c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
>> - c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
>> + c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
>> + c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>>
>> - c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
>> - c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>> + c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>>
>> - c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>> + aarch32 = c->pfr64.el1 == 2;
>
> I would put some () around the test.
>
>> #endif
>>
>> + if ( aarch32 )
>> + {
>> c->pfr32.bits[0] = READ_SYSREG(ID_PFR0_EL1);
>> c->pfr32.bits[1] = READ_SYSREG(ID_PFR1_EL1);
>> c->pfr32.bits[2] = READ_SYSREG(ID_PFR2_EL1);
>> @@ -153,6 +159,7 @@ void identify_cpu(struct cpuinfo_arm *c)
>> #ifndef MVFR2_MAYBE_UNDEFINED
>> c->mvfr.bits[2] = READ_SYSREG(MVFR2_EL1);
>> #endif
>
> If we test for aarch32, the ifndef here should not be needed anymore.
> > Is your previous patch really still needed if we go with the aarch32
path ?
There were two undefs discovered:
1) On Armv7 when accessing MVFR2_EL1
2) On Cavium Thunder-X (Armv8) when accessing ID_PFR2_EL1
If you remove the #ifdef, then you will re-introduce the UNDEF on Armv7.
Cheers,
--
Julien Grall
Hi Julien,
> On 12 Jan 2021, at 12:46, Julien Grall <julien@xen.org> wrote:
>
>
>
> On 12/01/2021 10:50, Bertrand Marquis wrote:
>> Hi Stefano,
>>> On 12 Jan 2021, at 00:16, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>
>>> Don't read aarch32 system registers at boot time when the aarch32 state
>>> is not available. They are UNKNOWN, so it is not useful to read them.
>>> Moreover, on Cavium ThunderX reading ID_PFR2_EL1 causes a Xen crash.
>>> Instead, only read them when aarch32 is available.
>>>
>>> Leave the corresponding fields in struct cpuinfo_arm so that they
>>> are read-as-zero from a guest.
>> I agree with the fact that all users of identify_cpu are currently using spaces
>> which are 0 but to make the core a bit more robust we could do a memset(0)
>> of the structure at the beginning of the function.
>>>
>>> Since we are editing identify_cpu, also fix the indentation: 4 spaces
>>> instead of 8.
>>>
>>> Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
>>> Link: https://marc.info/?l=xen-devel&m=161035501118086
>>> Link: http://logs.test-lab.xenproject.org/osstest/logs/158293/test-arm64-arm64-xl-xsm/info.html
>>> Suggested-by: Julien Grall <julien@xen.org>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>> ---
>>> xen/arch/arm/cpufeature.c | 35 +++++++++++++++++++++--------------
>>> 1 file changed, 21 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
>>> index 698bfa0201..b1c82ade49 100644
>>> --- a/xen/arch/arm/cpufeature.c
>>> +++ b/xen/arch/arm/cpufeature.c
>>> @@ -101,29 +101,35 @@ int enable_nonboot_cpu_caps(const struct arm_cpu_capabilities *caps)
>>>
>>> void identify_cpu(struct cpuinfo_arm *c)
>>> {
>>> - c->midr.bits = READ_SYSREG(MIDR_EL1);
>>> - c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>>> + bool aarch32 = true;
>>> +
>>> + c->midr.bits = READ_SYSREG(MIDR_EL1);
>>> + c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>>>
>>> #ifdef CONFIG_ARM_64
>>> - c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
>>> - c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
>>> + c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
>>> + c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
>>> +
>>> + c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
>>> + c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>>>
>>> - c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
>>> - c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>>> + c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
>>> + c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>>>
>>> - c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
>>> - c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>>> + c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
>>> + c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
>>> + c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
>>>
>>> - c->mm64.bits[0] = READ_SYSREG(ID_AA64MMFR0_EL1);
>>> - c->mm64.bits[1] = READ_SYSREG(ID_AA64MMFR1_EL1);
>>> - c->mm64.bits[2] = READ_SYSREG(ID_AA64MMFR2_EL1);
>>> + c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
>>> + c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>>>
>>> - c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
>>> - c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>>> + c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>>>
>>> - c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>>> + aarch32 = c->pfr64.el1 == 2;
>> I would put some () around the test.
>>> #endif
>>>
>>> + if ( aarch32 )
>>> + {
>>> c->pfr32.bits[0] = READ_SYSREG(ID_PFR0_EL1);
>>> c->pfr32.bits[1] = READ_SYSREG(ID_PFR1_EL1);
>>> c->pfr32.bits[2] = READ_SYSREG(ID_PFR2_EL1);
>>> @@ -153,6 +159,7 @@ void identify_cpu(struct cpuinfo_arm *c)
>>> #ifndef MVFR2_MAYBE_UNDEFINED
>>> c->mvfr.bits[2] = READ_SYSREG(MVFR2_EL1);
>>> #endif
>> If we test for aarch32, the ifndef here should not be needed anymore.
>> > Is your previous patch really still needed if we go with the aarch32
> path ?
>
> There were two undefs discovered:
> 1) On Armv7 when accessing MVFR2_EL1
> 2) On Cavium Thunder-X (Armv8) when accessing ID_PFR2_EL1
>
> If you remove the #ifdef, then you will re-introduce the UNDEF on Armv7.
Right sorry missed that one.
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
© 2016 - 2024 Red Hat, Inc.