Hi,
> On 10 Dec 2020, at 15:45, Julien Grall <jul...@xen.org> wrote:
>
> Hi Bertrand,
>
> On 10/12/2020 15:14, Bertrand Marquis wrote:
>> Hi Julien,
>>> On 9 Dec 2020, at 23:03, Julien Grall <jul...@xen.org> wrote:
>>>
>>> Hi Bertrand,
>>>
>>> On 09/12/2020 16:30, Bertrand Marquis wrote:
>>>> Add definition and entries in cpuinfo for ID registers introduced in
>>>> newer Arm Architecture reference manual:
>>>> - ID_PFR2: processor feature register 2
>>>> - ID_DFR1: debug feature register 1
>>>> - ID_MMFR4 and ID_MMFR5: Memory model feature registers 4 and 5
>>>> - ID_ISA6: ISA Feature register 6
>>>> Add more bitfield definitions in PFR fields of cpuinfo.
>>>> Add MVFR2 register definition for aarch32.
>>>> Add mvfr values in cpuinfo.
>>>> Add some registers definition for arm64 in sysregs as some are not
>>>> always know by compilers.
>>>> Initialize the new values added in cpuinfo in identify_cpu during init.
>>>> Signed-off-by: Bertrand Marquis <bertrand.marq...@arm.com>
>>>> ---
>>>> Changes in V2:
>>>> Fix dbg32 table size and add proper initialisation of the second entry
>>>> of the table by reading ID_DFR1 register.
>>>> Changes in V3:
>>>> Fix typo in commit title
>>>> Add MVFR2 definition and handling on aarch32 and remove specific case
>>>> for mvfr field in cpuinfo (now the same on arm64 and arm32).
>>>> Add MMFR4 definition if not known by the compiler.
>>>> ---
>>>> xen/arch/arm/cpufeature.c | 18 ++++++++++
>>>> xen/include/asm-arm/arm64/sysregs.h | 28 +++++++++++++++
>>>> xen/include/asm-arm/cpregs.h | 12 +++++++
>>>> xen/include/asm-arm/cpufeature.h | 56 ++++++++++++++++++++++++-----
>>>> 4 files changed, 105 insertions(+), 9 deletions(-)
>>>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
>>>> index 44126dbf07..bc7ee5ac95 100644
>>>> --- a/xen/arch/arm/cpufeature.c
>>>> +++ b/xen/arch/arm/cpufeature.c
>>>> @@ -114,15 +114,20 @@ void identify_cpu(struct cpuinfo_arm *c)
>>>> c->mm64.bits[0] = READ_SYSREG64(ID_AA64MMFR0_EL1);
>>>> c->mm64.bits[1] = READ_SYSREG64(ID_AA64MMFR1_EL1);
>>>> + c->mm64.bits[2] = READ_SYSREG64(ID_AA64MMFR2_EL1);
>>>> c->isa64.bits[0] = READ_SYSREG64(ID_AA64ISAR0_EL1);
>>>> c->isa64.bits[1] = READ_SYSREG64(ID_AA64ISAR1_EL1);
>>>> +
>>>> + c->zfr64.bits[0] = READ_SYSREG64(ID_AA64ZFR0_EL1);
>>>> #endif
>>>> c->pfr32.bits[0] = READ_SYSREG32(ID_PFR0_EL1);
>>>> c->pfr32.bits[1] = READ_SYSREG32(ID_PFR1_EL1);
>>>> + c->pfr32.bits[2] = READ_SYSREG32(ID_PFR2_EL1);
>>>> c->dbg32.bits[0] = READ_SYSREG32(ID_DFR0_EL1);
>>>> + c->dbg32.bits[1] = READ_SYSREG32(ID_DFR1_EL1);
>>>> c->aux32.bits[0] = READ_SYSREG32(ID_AFR0_EL1);
>>>> @@ -130,6 +135,8 @@ void identify_cpu(struct cpuinfo_arm *c)
>>>> c->mm32.bits[1] = READ_SYSREG32(ID_MMFR1_EL1);
>>>> c->mm32.bits[2] = READ_SYSREG32(ID_MMFR2_EL1);
>>>> c->mm32.bits[3] = READ_SYSREG32(ID_MMFR3_EL1);
>>>> + c->mm32.bits[4] = READ_SYSREG32(ID_MMFR4_EL1);
>>>> + c->mm32.bits[5] = READ_SYSREG32(ID_MMFR5_EL1);
>>>
>>> Please don't introduce any more use of READ_SYSREG32(), they are wrong on
>>> Armv8 because system registers are always 64-bit.
>> I followed the existing implementation but ...
>>>
>>>> c->isa32.bits[0] = READ_SYSREG32(ID_ISAR0_EL1);
>>>> c->isa32.bits[1] = READ_SYSREG32(ID_ISAR1_EL1);
>>>> @@ -137,6 +144,17 @@ void identify_cpu(struct cpuinfo_arm *c)
>>>> c->isa32.bits[3] = READ_SYSREG32(ID_ISAR3_EL1);
>>>> c->isa32.bits[4] = READ_SYSREG32(ID_ISAR4_EL1);
>>>> c->isa32.bits[5] = READ_SYSREG32(ID_ISAR5_EL1);
>>>> + c->isa32.bits[6] = READ_SYSREG32(ID_ISAR6_EL1);
>>>> +
>>>> +#ifdef CONFIG_ARM_64
>>>> + c->mvfr.bits[0] = READ_SYSREG64(MVFR0_EL1);
>>>> + c->mvfr.bits[1] = READ_SYSREG64(MVFR1_EL1);
>>>> + c->mvfr.bits[2] = READ_SYSREG64(MVFR2_EL1);
>>>> +#else
>>>> + c->mvfr.bits[0] = READ_CP32(MVFR0);
>>>> + c->mvfr.bits[1] = READ_CP32(MVFR1);
>>>> + c->mvfr.bits[2] = READ_CP32(MVFR2);
>>>> +#endif
>>>
>>> READ_SYSREG() will do the job to either use READ_SYSREG64() or READ_CP32()
>>> depending on the arch used.
>> .. I can modify the ones I added and the existing ones to user READ_SYSREG
>> instead.
>> Please confirm if you want me to do that.
>
> Yes, please use READ_SYSREG() for the new ones. We can convert the others
> separately.
>
>>>
>>>> }
>>>> /*
>>>> diff --git a/xen/include/asm-arm/arm64/sysregs.h
>>>> b/xen/include/asm-arm/arm64/sysregs.h
>>>> index c60029d38f..077fd95fb7 100644
>>>> --- a/xen/include/asm-arm/arm64/sysregs.h
>>>> +++ b/xen/include/asm-arm/arm64/sysregs.h
>>>> @@ -57,6 +57,34 @@
>>>> #define ICH_AP1R2_EL2 __AP1Rx_EL2(2)
>>>> #define ICH_AP1R3_EL2 __AP1Rx_EL2(3)
>>>> +/*
>>>> + * Define ID coprocessor registers if they are not
>>>> + * already defined by the compiler.
>>>> + *
>>>> + * Values picked from linux kernel
>>>> + */
>>>> +#ifndef ID_AA64MMFR2_EL1
>>>
>>> I am a bit puzzled how this meant to work. Will the libc/compiler headers
>>> define ID_AA64MMFR2_EL1?
>> I tested this and if the compiler has a definition for the register, I am
>> not entering the ifndef.
>> So there is no header defining this but if the compiler has the definition
>> for this the ifndef will
>> not be entered.
>
> Good to hear :).
>
>>>
>>>> +#define ID_AA64MMFR2_EL1 S3_0_C0_C7_2
>>>> +#endif
>>>> +#ifndef ID_PFR2_EL1
>>>> +#define ID_PFR2_EL1 S3_0_C0_C3_4
>>>> +#endif
>>>> +#ifndef ID_MMFR4_EL1
>>>> +#define ID_MMFR4_EL1 S3_0_C0_C2_6
>>>> +#endif
>>>> +#ifndef ID_MMFR5_EL1
>>>> +#define ID_MMFR5_EL1 S3_0_C0_C3_6
>>>> +#endif
>>>> +#ifndef ID_ISAR6_EL1
>>>> +#define ID_ISAR6_EL1 S3_0_C0_C2_7
>>>> +#endif
>>>> +#ifndef ID_AA64ZFR0_EL1
>>>> +#define ID_AA64ZFR0_EL1 S3_0_C0_C4_4
>>>> +#endif
>>>> +#ifndef ID_DFR1_EL1
>>>> +#define ID_DFR1_EL1 S3_0_C0_C3_5
>>>> +#endif
>>>> +
>>>> /* Access to system registers */
>>>> #define READ_SYSREG32(name) ((uint32_t)READ_SYSREG64(name))
>>>> diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
>>>> index 8fd344146e..2690ddeb7a 100644
>>>> --- a/xen/include/asm-arm/cpregs.h
>>>> +++ b/xen/include/asm-arm/cpregs.h
>>>> @@ -63,6 +63,8 @@
>>>> #define FPSID p10,7,c0,c0,0 /* Floating-Point System ID
>>>> Register */
>>>> #define FPSCR p10,7,c1,c0,0 /* Floating-Point Status and
>>>> Control Register */
>>>> #define MVFR0 p10,7,c7,c0,0 /* Media and VFP Feature Register
>>>> 0 */
>>>> +#define MVFR1 p10,7,c6,c0,0 /* Media and VFP Feature Register
>>>> 1 */
>>>> +#define MVFR2 p10,7,c5,c0,0 /* Media and VFP Feature Register
>>>> 2 */
>>>> #define FPEXC p10,7,c8,c0,0 /* Floating-Point Exception
>>>> Control Register */
>>>> #define FPINST p10,7,c9,c0,0 /* Floating-Point Instruction
>>>> Register */
>>>> #define FPINST2 p10,7,c10,c0,0 /* Floating-point Instruction
>>>> Register 2 */
>>>> @@ -108,18 +110,23 @@
>>>> #define MPIDR p15,0,c0,c0,5 /* Multiprocessor Affinity
>>>> Register */
>>>> #define ID_PFR0 p15,0,c0,c1,0 /* Processor Feature Register 0 */
>>>> #define ID_PFR1 p15,0,c0,c1,1 /* Processor Feature Register 1 */
>>>> +#define ID_PFR2 p15,0,c0,c3,4 /* Processor Feature Register 2 */
>>>> #define ID_DFR0 p15,0,c0,c1,2 /* Debug Feature Register 0 */
>>>> +#define ID_DFR1 p15,0,c0,c3,5 /* Debug Feature Register 1 */
>>>> #define ID_AFR0 p15,0,c0,c1,3 /* Auxiliary Feature Register 0 */
>>>> #define ID_MMFR0 p15,0,c0,c1,4 /* Memory Model Feature Register
>>>> 0 */
>>>> #define ID_MMFR1 p15,0,c0,c1,5 /* Memory Model Feature Register
>>>> 1 */
>>>> #define ID_MMFR2 p15,0,c0,c1,6 /* Memory Model Feature Register
>>>> 2 */
>>>> #define ID_MMFR3 p15,0,c0,c1,7 /* Memory Model Feature Register
>>>> 3 */
>>>> +#define ID_MMFR4 p15,0,c0,c2,6 /* Memory Model Feature Register
>>>> 4 */
>>>> +#define ID_MMFR5 p15,0,c0,c3,6 /* Memory Model Feature Register
>>>> 5 */
>>>> #define ID_ISAR0 p15,0,c0,c2,0 /* ISA Feature Register 0 */
>>>> #define ID_ISAR1 p15,0,c0,c2,1 /* ISA Feature Register 1 */
>>>> #define ID_ISAR2 p15,0,c0,c2,2 /* ISA Feature Register 2 */
>>>> #define ID_ISAR3 p15,0,c0,c2,3 /* ISA Feature Register 3 */
>>>> #define ID_ISAR4 p15,0,c0,c2,4 /* ISA Feature Register 4 */
>>>> #define ID_ISAR5 p15,0,c0,c2,5 /* ISA Feature Register 5 */
>>>> +#define ID_ISAR6 p15,0,c0,c2,7 /* ISA Feature Register 6 */
>>>> #define CCSIDR p15,1,c0,c0,0 /* Cache Size ID Registers */
>>>> #define CLIDR p15,1,c0,c0,1 /* Cache Level ID Register */
>>>> #define CSSELR p15,2,c0,c0,0 /* Cache Size Selection Register
>>>> */
>>>> @@ -312,18 +319,23 @@
>>>> #define HSTR_EL2 HSTR
>>>> #define ID_AFR0_EL1 ID_AFR0
>>>> #define ID_DFR0_EL1 ID_DFR0
>>>> +#define ID_DFR1_EL1 ID_DFR1
>>>> #define ID_ISAR0_EL1 ID_ISAR0
>>>> #define ID_ISAR1_EL1 ID_ISAR1
>>>> #define ID_ISAR2_EL1 ID_ISAR2
>>>> #define ID_ISAR3_EL1 ID_ISAR3
>>>> #define ID_ISAR4_EL1 ID_ISAR4
>>>> #define ID_ISAR5_EL1 ID_ISAR5
>>>> +#define ID_ISAR6_EL1 ID_ISAR6
>>>> #define ID_MMFR0_EL1 ID_MMFR0
>>>> #define ID_MMFR1_EL1 ID_MMFR1
>>>> #define ID_MMFR2_EL1 ID_MMFR2
>>>> #define ID_MMFR3_EL1 ID_MMFR3
>>>> +#define ID_MMFR4_EL1 ID_MMFR4
>>>> +#define ID_MMFR5_EL1 ID_MMFR5
>>>> #define ID_PFR0_EL1 ID_PFR0
>>>> #define ID_PFR1_EL1 ID_PFR1
>>>> +#define ID_PFR2_EL1 ID_PFR2
>>>> #define IFSR32_EL2 IFSR
>>>> #define MDCR_EL2 HDCR
>>>> #define MIDR_EL1 MIDR
>>>> diff --git a/xen/include/asm-arm/cpufeature.h
>>>> b/xen/include/asm-arm/cpufeature.h
>>>> index c7b5052992..6cf83d775b 100644
>>>> --- a/xen/include/asm-arm/cpufeature.h
>>>> +++ b/xen/include/asm-arm/cpufeature.h
>>>> @@ -148,6 +148,7 @@ struct cpuinfo_arm {
>>>> union {
>>>> uint64_t bits[2];
>>>> struct {
>>>> + /* PFR0 */
>>>> unsigned long el0:4;
>>>> unsigned long el1:4;
>>>> unsigned long el2:4;
>>>> @@ -155,9 +156,23 @@ struct cpuinfo_arm {
>>>> unsigned long fp:4; /* Floating Point */
>>>> unsigned long simd:4; /* Advanced SIMD */
>>>> unsigned long gic:4; /* GIC support */
>>>> - unsigned long __res0:28;
>>>> + unsigned long ras:4;
>>>> + unsigned long sve:4;
>>>> + unsigned long sel2:4;
>>>> + unsigned long mpam:4;
>>>> + unsigned long amu:4;
>>>> + unsigned long dit:4;
>>>> + unsigned long __res0:4;
>>>> unsigned long csv2:4;
>>>> - unsigned long __res1:4;
>>>> + unsigned long cvs3:4;
>>>> +
>>>> + /* PFR1 */
>>>> + unsigned long bt:4;
>>>> + unsigned long ssbs:4;
>>>> + unsigned long mte:4;
>>>> + unsigned long ras_frac:4;
>>>> + unsigned long mpam_frac:4;
>>>> + unsigned long __res1:44;
>>>> };
>>>> } pfr64;
>>>> @@ -170,7 +185,7 @@ struct cpuinfo_arm {
>>>> } aux64;
>>>> union {
>>>> - uint64_t bits[2];
>>>> + uint64_t bits[3];
>>>> struct {
>>>> unsigned long pa_range:4;
>>>> unsigned long asid_bits:4;
>>>> @@ -190,6 +205,8 @@ struct cpuinfo_arm {
>>>> unsigned long pan:4;
>>>> unsigned long __res1:8;
>>>> unsigned long __res2:32;
>>>> +
>>>> + unsigned long __res3:64;
>>>> };
>>>> } mm64;
>>>> @@ -197,6 +214,10 @@ struct cpuinfo_arm {
>>>> uint64_t bits[2];
>>>> } isa64;
>>>> + struct {
>>>> + uint64_t bits[1];
>>>> + } zfr64;
>>>> +
>>>> #endif
>>>> /*
>>>> @@ -204,25 +225,38 @@ struct cpuinfo_arm {
>>>> * when running in 32-bit mode.
>>>> */
>>>> union {
>>>> - uint32_t bits[2];
>>>> + uint32_t bits[3];
>>>> struct {
>>>> + /* PFR0 */
>>>> unsigned long arm:4;
>>>> unsigned long thumb:4;
>>>> unsigned long jazelle:4;
>>>> unsigned long thumbee:4;
>>>> - unsigned long __res0:16;
>>>> + unsigned long csv2:4;
>>>> + unsigned long amu:4;
>>>> + unsigned long dit:4;
>>>> + unsigned long ras:4;
>>>> + /* PFR1 */
>>>> unsigned long progmodel:4;
>>>> unsigned long security:4;
>>>> unsigned long mprofile:4;
>>>> unsigned long virt:4;
>>>> unsigned long gentimer:4;
>>>> - unsigned long __res1:12;
>>>> + unsigned long sec_frac:4;
>>>> + unsigned long virt_frac:4;
>>>> + unsigned long gic:4;
>>>> +
>>>> + /* PFR2 */
>>>> + unsigned long csv3:4;
>>>> + unsigned long ssbs:4;
>>>> + unsigned long ras_frac:4;
>>>> + unsigned long __res2:20;
>>>> };
>>>> } pfr32;
>>>> struct {
>>>> - uint32_t bits[1];
>>>> + uint32_t bits[2];
>>>> } dbg32;
>>>> struct {
>>>> @@ -230,12 +264,16 @@ struct cpuinfo_arm {
>>>> } aux32;
>>>> struct {
>>>> - uint32_t bits[4];
>>>> + uint32_t bits[6];
>>>> } mm32;
>>>> struct {
>>>> - uint32_t bits[6];
>>>> + uint32_t bits[7];
>>>> } isa32;
>>>> +
>>>> + struct {
>>>> + uint64_t bits[3];
>>>
>>> Shouldn't this be register_t?
>> I followed the scheme of the rest of the structure which
>> is always using uint64_t or uint32_t for bits definitions.
>
> Right, but I am sure you will not be surprised if I tell you this is buggy
> ;). The historical reason is, IIRC, the original spec of Armv8.0 described
> them as 32-bit registers.
>
> The spec was updated a while ago to clarify they are 64-bit when running in
> AArch64. But a majority of them still have the top 32-bit RES0 (thankfully!).
>
>> Why should I use register_t type for this one ?
>
> Because the value is 32-bit on AArch32 and 64-bit for AArch64. I am ok to use
> 64-bit still for AArch32, but it sounds a bit of a waste of memory.
>
> What I care the most here is we use 64-bit for the new registers on AArch64.
> Otherwise, we are going to soon discover that a bit above 32 was allocated
> and not detected by Xen. I don't want to be the one doing the debugging!
>
> Admittly, this is not a new issue. However, the more offending code we had
> the more it will be difficul to get Xen to be fully compliant with the Armv8
> spec.
I agree with that so I propose to do the following:
- create a new serie to fix those:
- use READ_SYSREG in cpufeature.c
- use register_t in cpuinfo structure
- rebase my serie on top of this one and use the proper types on it
Might require more work but seem to be the right approach.
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
