Re: [PATCH 2/2] target/arm: Implement Neoverse N2 CPU model

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 17 Oct 2023 at 14:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > Implement a model of the Neoverse N2 CPU. This is an Armv9.0-A
>> > processor very similar to the Cortex-A710. The differences are:
>> >  * no FEAT_EVT
>> >  * FEAT_DGH (data gathering hint)
>> >  * FEAT_NV (not yet implemented in QEMU)
>> >  * Statistical Profiling Extension (not implemented in QEMU)
>> >  * 48 bit physical address range, not 40
>> >  * CTR_EL0.DIC = 1 (no explicit icache cleaning needed)
>> >  * PMCR_EL0.N = 6 (always 6 PMU counters, not 20)
>> >
>> > Because it has 48-bit physical address support, we can use
>> > this CPU in the sbsa-ref board as well as the virt board.
>> >
>> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> > ---
>> >  docs/system/arm/virt.rst |   1 +
>> >  hw/arm/sbsa-ref.c        |   1 +
>> >  hw/arm/virt.c            |   1 +
>> >  target/arm/tcg/cpu64.c   | 103 +++++++++++++++++++++++++++++++++++++++
>> >  4 files changed, 106 insertions(+)
>> >
>> > diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
>> > index e1697ac8f48..7c4c80180c6 100644
>> > --- a/docs/system/arm/virt.rst
>> > +++ b/docs/system/arm/virt.rst
>> > @@ -63,6 +63,7 @@ Supported guest CPU types:
>> >  - ``host`` (with KVM only)
>> >  - ``neoverse-n1`` (64-bit)
>> >  - ``neoverse-v1`` (64-bit)
>> > +- ``neoverse-n2`` (64-bit)
>> >  - ``max`` (same as ``host`` for KVM; best possible emulation with TCG)
>> >
>> >  Note that the default is ``cortex-a15``, so for an AArch64 guest you must
>> > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
>> > index bc89eb48062..4db287287e1 100644
>> > --- a/hw/arm/sbsa-ref.c
>> > +++ b/hw/arm/sbsa-ref.c
>> > @@ -154,6 +154,7 @@ static const char * const valid_cpus[] = {
>> >      ARM_CPU_TYPE_NAME("cortex-a72"),
>> >      ARM_CPU_TYPE_NAME("neoverse-n1"),
>> >      ARM_CPU_TYPE_NAME("neoverse-v1"),
>> > +    ARM_CPU_TYPE_NAME("neoverse-n2"),
>> >      ARM_CPU_TYPE_NAME("max"),
>> >  };
>> >
>> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> > index 8ad78b23c24..42253462735 100644
>> > --- a/hw/arm/virt.c
>> > +++ b/hw/arm/virt.c
>> > @@ -215,6 +215,7 @@ static const char *valid_cpus[] = {
>> >      ARM_CPU_TYPE_NAME("a64fx"),
>> >      ARM_CPU_TYPE_NAME("neoverse-n1"),
>> >      ARM_CPU_TYPE_NAME("neoverse-v1"),
>> > +    ARM_CPU_TYPE_NAME("neoverse-n2"),
>> >  #endif
>> >      ARM_CPU_TYPE_NAME("cortex-a53"),
>> >      ARM_CPU_TYPE_NAME("cortex-a57"),
>> > diff --git a/target/arm/tcg/cpu64.c b/target/arm/tcg/cpu64.c
>> > index ea43cf3c1ee..370cc82f0ef 100644
>> > --- a/target/arm/tcg/cpu64.c
>> > +++ b/target/arm/tcg/cpu64.c
>> > @@ -963,6 +963,108 @@ static void aarch64_a710_initfn(Object *obj)
>> >      aarch64_add_sve_properties(obj);
>> >  }
>> >
>> > +/* Extra IMPDEF regs in the N2 beyond those in the A710 */
>> > +static const ARMCPRegInfo neoverse_n2_cp_reginfo[] = {
>> > +    { .name = "CPURNDBR_EL3", .state = ARM_CP_STATE_AA64,
>> > +      .opc0 = 3, .opc1 = 6, .crn = 15, .crm = 3, .opc2 = 0,
>> > +      .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> > +    { .name = "CPURNDPEID_EL3", .state = ARM_CP_STATE_AA64,
>> > +      .opc0 = 3, .opc1 = 6, .crn = 15, .crm = 3, .opc2 = 1,
>> > +      .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> > +};
>> > +
>> > +static void aarch64_neoverse_n2_initfn(Object *obj)
>> > +{
>> > +    ARMCPU *cpu = ARM_CPU(obj);
>> > +
>> > +    cpu->dtb_compatible = "arm,neoverse-n2";
>> > +    set_feature(&cpu->env, ARM_FEATURE_V8);
>> > +    set_feature(&cpu->env, ARM_FEATURE_NEON);
>> > +    set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
>> > +    set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>> > +    set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>> > +    set_feature(&cpu->env, ARM_FEATURE_EL2);
>> > +    set_feature(&cpu->env, ARM_FEATURE_EL3);
>> > +    set_feature(&cpu->env, ARM_FEATURE_PMU);
>> > +
>> > +    /* Ordered by Section B.5: AArch64 ID registers */
>> > +    cpu->midr          = 0x410FD493; /* r0p3 */
>> > +    cpu->revidr        = 0;
>> > +    cpu->isar.id_pfr0  = 0x21110131;
>> > +    cpu->isar.id_pfr1  = 0x00010000; /* GIC filled in later */
>> > +    cpu->isar.id_dfr0  = 0x16011099;
>> > +    cpu->id_afr0       = 0;
>> > +    cpu->isar.id_mmfr0 = 0x10201105;
>> > +    cpu->isar.id_mmfr1 = 0x40000000;
>> > +    cpu->isar.id_mmfr2 = 0x01260000;
>> > +    cpu->isar.id_mmfr3 = 0x02122211;
>> > +    cpu->isar.id_isar0 = 0x02101110;
>> > +    cpu->isar.id_isar1 = 0x13112111;
>> > +    cpu->isar.id_isar2 = 0x21232042;
>> > +    cpu->isar.id_isar3 = 0x01112131;
>> > +    cpu->isar.id_isar4 = 0x00010142;
>> > +    cpu->isar.id_isar5 = 0x11011121; /* with Crypto */
>> > +    cpu->isar.id_mmfr4 = 0x01021110;
>> > +    cpu->isar.id_isar6 = 0x01111111;
>> > +    cpu->isar.mvfr0    = 0x10110222;
>> > +    cpu->isar.mvfr1    = 0x13211111;
>> > +    cpu->isar.mvfr2    = 0x00000043;
>> > +    cpu->isar.id_pfr2  = 0x00000011;
>> > +    cpu->isar.id_aa64pfr0  = 0x1201111120111112ull; /* GIC filled in later */
>> > +    cpu->isar.id_aa64pfr1  = 0x0000000000000221ull;
>> > +    cpu->isar.id_aa64zfr0  = 0x0000110100110021ull; /* with Crypto */
>> > +    cpu->isar.id_aa64dfr0  = 0x000011f210305619ull;
>> > +    cpu->isar.id_aa64dfr1  = 0;
>> > +    cpu->id_aa64afr0       = 0;
>> > +    cpu->id_aa64afr1       = 0;
>> > +    cpu->isar.id_aa64isar0 = 0x0221111110212120ull; /* with Crypto */
>> > +    cpu->isar.id_aa64isar1 = 0x0011111101211052ull;
>> > +    cpu->isar.id_aa64mmfr0 = 0x0000022200101125ull;
>> > +    cpu->isar.id_aa64mmfr1 = 0x0000000010212122ull;
>> > +    cpu->isar.id_aa64mmfr2 = 0x1221011112101011ull;
>> > +    cpu->clidr             = 0x0000001482000023ull;
>> > +    cpu->gm_blocksize      = 4;
>> > +    cpu->ctr               = 0x00000004b444c004ull;
>> > +    cpu->dcz_blocksize     = 4;
>> > +    /* TODO FEAT_MPAM: mpamidr_el1 = 0x0000_0001_001e_01ff */
>>
>> Jonathan mentioned he was hacking about with MPAM so if there is a stub
>> implementation of the fields it would be nice to add them.
>
> I'm just following the existing A710 code here. There isn't
> a stub implementation of the fields, which is why there's
> just a TODO comment documenting the ID register value.
>
>> > +
>> > +    /* Section B.7.2: PMCR_EL0 */
>> > +    cpu->isar.reset_pmcr_el0 = 0x3000;  /* with 6 counters */
>> > +
>> > +    /* Section B.8.9: ICH_VTR_EL2 */
>> > +    cpu->gic_num_lrs = 4;
>> > +    cpu->gic_vpribits = 5;
>> > +    cpu->gic_vprebits = 5;
>> > +    cpu->gic_pribits = 5;
>> > +
>> > +    /* Section 14: Scalable Vector Extensions support */
>> > +    cpu->sve_vq.supported = 1 << 0;  /* 128bit */
>> > +
>> > +    /*
>> > +     * The Neoverse N2 TRM does not list CCSIDR values.  The layout of
>> > +     * the caches are in text in Table 7-1, Table 8-1, and Table 9-1.
>> > +     *
>> > +     * L1: 4-way set associative 64-byte line size, total 64K.
>> > +     * L2: 8-way set associative 64 byte line size, total either 512K or 1024K.
>> > +     */
>> > +    cpu->ccsidr[0] = make_ccsidr64(4, 64, 64 * KiB);   /* L1 dcache */
>> > +    cpu->ccsidr[1] = cpu->ccsidr[0];                   /* L1 icache */
>> > +    cpu->ccsidr[2] = make_ccsidr64(8, 64, 512 * KiB);  /* L2 cache */
>> > +
>> > +    /* FIXME: Not documented -- copied from neoverse-v1 */
>> > +    cpu->reset_sctlr = 0x30c50838;
>>
>> Hmm reset_sctlr might need to be bigger because being used for sctlr_el3
>> is a 64 bit entity which has a number of fields:
>
> In theory, yes. In practice, only if we get to a point where we
> want to set some bit in the top 32 bits, which we don't yet.
>
>>
>>   - DSSBS,bit[44] to have some sort of impdef value
>>   - the others all look like architecturally UNKNOWN values
>>
>> Otherwise I think this is:
>>
>>   - 3 - RES1 29:28
>>   - 0 - RES0 + I assume LE EE bit
>>   - c - RES1 23, EIS 22 (architecturally UNKNOWN on reset)
>>   - 5 - RES1 18, 16
>>   - 0 - RES0 15:14, EnDB 13 (architecturally UNKNOWN on reset), I 12 RES0
>>   - 8 - EOS 11 (architecturally UNKNOWN on reset)
>>   - 3 - RES1 5:4
>>   - 8 - SA 3 (architecturally UNKNOWN on reset)
>>
>> It's a little confusing because the language describing these
>> architecturally unknown states refers to resetting to EL3 (I assume a
>> decent firmware would make sure everything is set correctly). Which
>> makes me wonder if reset_sctlr is really "sane values for a direct
>> kernel boot"?
>
> It's the hardware reset value, at least nominally speaking.
> We use the value for our SCTLR_EL1 if we aren't emulating EL2 or EL3,
> which means we diverge a little from the hardware which probably doesn't
> support an EL1-only config.
>
> To the extent that we need to set values here for kernel boot, we would
> do that in hw/arm/boot.c. But mostly we don't, because the kernel sets
> up the SCTLR register for the EL it is entered in.
>
> This used to be a bit better defined on older CPUs when previous
> versions of the architecture weren't so loose about values being
> UNKNOWN on reset. It's unfortunate that we don't have the documented
> value for whatever the IMPDEF reset value is for this CPU. In
> practice, because it's IMPDEF and UNKNOWN it doesn't matter too much --
> guest software will set it as needed. The value used here is "safe"
> because it's one we already use for other CPUs.

Fair enough:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro