Re: [PATCH v3 5/7] xen/arm: Add handler for cp15 ID registers

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Stefano,

> On 9 Dec 2020, at 19:54, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Wed, 9 Dec 2020, Bertrand Marquis wrote:
>> Add support for emulation of cp15 based ID registers (on arm32 or when
>> running a 32bit guest on arm64).
>> The handlers are returning the values stored in the guest_cpuinfo
>> structure for known registers and RAZ for all reserved registers.
>> In the current status the MVFR registers are no supported.
>> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>> Changes in V2: Rebase
>> Changes in V3:
>>  Add case definition for reserved registers
>>  Add handling of reserved registers as RAZ.
>>  Fix code style in GENERATE_TID3_INFO declaration
>> 
>> ---
>> xen/arch/arm/vcpreg.c        | 39 ++++++++++++++++++++++++++++++++++++
>> xen/include/asm-arm/cpregs.h | 25 +++++++++++++++++++++++
>> 2 files changed, 64 insertions(+)
>> 
>> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
>> index cdc91cdf5b..d371a1c38c 100644
>> --- a/xen/arch/arm/vcpreg.c
>> +++ b/xen/arch/arm/vcpreg.c
>> @@ -155,6 +155,14 @@ TVM_REG32(CONTEXTIDR, CONTEXTIDR_EL1)
>>         break;                                                      \
>>     }
>> 
>> +/* Macro to generate easily case for ID co-processor emulation */
>> +#define GENERATE_TID3_INFO(reg, field, offset)                      \
>> +    case HSR_CPREG32(reg):                                          \
>> +    {                                                               \
>> +        return handle_ro_read_val(regs, regidx, cp32.read, hsr,     \
>> +                          1, guest_cpuinfo.field.bits[offset]);     \
>> +    }
>> +
>> void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
>> {
>>     const struct hsr_cp32 cp32 = hsr.cp32;
>> @@ -286,6 +294,37 @@ void do_cp15_32(struct cpu_user_regs *regs, const union 
>> hsr hsr)
>>          */
>>         return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
>> 
>> +    /*
>> +     * HCR_EL2.TID3
>> +     *
>> +     * This is trapping most Identification registers used by a guest
>> +     * to identify the processor features
>> +     */
>> +    GENERATE_TID3_INFO(ID_PFR0, pfr32, 0)
>> +    GENERATE_TID3_INFO(ID_PFR1, pfr32, 1)
>> +    GENERATE_TID3_INFO(ID_PFR2, pfr32, 2)
>> +    GENERATE_TID3_INFO(ID_DFR0, dbg32, 0)
>> +    GENERATE_TID3_INFO(ID_DFR1, dbg32, 1)
>> +    GENERATE_TID3_INFO(ID_AFR0, aux32, 0)
>> +    GENERATE_TID3_INFO(ID_MMFR0, mm32, 0)
>> +    GENERATE_TID3_INFO(ID_MMFR1, mm32, 1)
>> +    GENERATE_TID3_INFO(ID_MMFR2, mm32, 2)
>> +    GENERATE_TID3_INFO(ID_MMFR3, mm32, 3)
>> +    GENERATE_TID3_INFO(ID_MMFR4, mm32, 4)
>> +    GENERATE_TID3_INFO(ID_MMFR5, mm32, 5)
>> +    GENERATE_TID3_INFO(ID_ISAR0, isa32, 0)
>> +    GENERATE_TID3_INFO(ID_ISAR1, isa32, 1)
>> +    GENERATE_TID3_INFO(ID_ISAR2, isa32, 2)
>> +    GENERATE_TID3_INFO(ID_ISAR3, isa32, 3)
>> +    GENERATE_TID3_INFO(ID_ISAR4, isa32, 4)
>> +    GENERATE_TID3_INFO(ID_ISAR5, isa32, 5)
>> +    GENERATE_TID3_INFO(ID_ISAR6, isa32, 6)
>> +    /* MVFR registers are in cp10 no cp15 */
>> +
>> +    HSR_CPREG32_TID3_RESERVED_CASE:
>> +        /* Handle all reserved registers as RAZ */
>> +        return handle_ro_raz(regs, regidx, cp32.read, hsr, 1);
> 
> Same question as for the aarch64 case: do we need to do write-ignore
> for the reserved registers?

Arm architecture reference manual is listing all those registers as RO including
the reserved ones (cf table D12-2). This said I have no objection to make them
write ignore but from my understanding this would not reflect the hardware
behaviour.

> 
> 
>>     /*
>>      * HCR_EL2.TIDCP
>>      *
>> diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
>> index 2690ddeb7a..5cb1ad5cbe 100644
>> --- a/xen/include/asm-arm/cpregs.h
>> +++ b/xen/include/asm-arm/cpregs.h
>> @@ -133,6 +133,31 @@
>> #define VPIDR           p15,4,c0,c0,0   /* Virtualization Processor ID 
>> Register */
>> #define VMPIDR          p15,4,c0,c0,5   /* Virtualization Multiprocessor ID 
>> Register */
>> 
>> +/*
>> + * Those cases are catching all Reserved registers trapped by TID3 which
>> + * currently have no assignment.
>> + * HCR.TID3 is trapping all registers in the group 3:
>> + * coproc == p15, opc1 == 0, CRn == c0, CRm == {c2-c7}, opc2 == {0-7}.
>> + */
>> +#define HSR_CPREG32_TID3_CASES(REG)     case HSR_CPREG32(p15,0,c0,REG,0): \
>> +                                        case HSR_CPREG32(p15,0,c0,REG,1): \
>> +                                        case HSR_CPREG32(p15,0,c0,REG,2): \
>> +                                        case HSR_CPREG32(p15,0,c0,REG,3): \
>> +                                        case HSR_CPREG32(p15,0,c0,REG,4): \
>> +                                        case HSR_CPREG32(p15,0,c0,REG,5): \
>> +                                        case HSR_CPREG32(p15,0,c0,REG,6): \
>> +                                        case HSR_CPREG32(p15,0,c0,REG,7)
>> +
>> +#define HSR_CPREG32_TID3_RESERVED_CASE  case HSR_CPREG32(p15,0,c0,c3,0): \
>> +                                        case HSR_CPREG32(p15,0,c0,c3,1): \
>> +                                        case HSR_CPREG32(p15,0,c0,c3,2): \
>> +                                        case HSR_CPREG32(p15,0,c0,c3,3): \
>> +                                        case HSR_CPREG32(p15,0,c0,c3,7): \
>> +                                        HSR_CPREG32_TID3_CASES(c4): \
>> +                                        HSR_CPREG32_TID3_CASES(c5): \
>> +                                        HSR_CPREG32_TID3_CASES(c6): \
>> +                                        HSR_CPREG32_TID3_CASES(c7)
> 
> The following are missing, is it a problem?
> 
> p15,0,c0,c0,2
> p15,0,c0,c0,3
> p15,0,c0,c0,4
> p15,0,c0,c0,6
> p15,0,c0,c0,7

HCR.TID3 documentation is saying that access to "coproc == p15, opc1 == 0, 
CRn == c0, CRm == {c2-c7}, opc2 == {0-7}” are trapped so CRm = c0 is not 
handled here.

Cheers
Bertradn