Re: [PATCH 5/5] target/arm: Implement cortex-a710

[Richard Henderson]

On 8/10/23 08:49, Peter Maydell wrote:
> On Thu, 10 Aug 2023 at 03:36, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> > The cortex-a710 is a first generation ARMv9.0-A processor.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   docs/system/arm/virt.rst |   1 +
> >   hw/arm/virt.c            |   1 +
> >   target/arm/tcg/cpu64.c   | 167 +++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 169 insertions(+)
> >
> > diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
> > index 51cdac6841..e1697ac8f4 100644
> > --- a/docs/system/arm/virt.rst
> > +++ b/docs/system/arm/virt.rst
> > @@ -58,6 +58,7 @@ Supported guest CPU types:
> >   - ``cortex-a57`` (64-bit)
> >   - ``cortex-a72`` (64-bit)
> >   - ``cortex-a76`` (64-bit)
> > +- ``cortex-a710`` (64-bit)
> >   - ``a64fx`` (64-bit)
> >   - ``host`` (with KVM only)
> >   - ``neoverse-n1`` (64-bit)
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 7d9dbc2663..d1522c305d 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -211,6 +211,7 @@ static const char *valid_cpus[] = {
> >       ARM_CPU_TYPE_NAME("cortex-a55"),
> >       ARM_CPU_TYPE_NAME("cortex-a72"),
> >       ARM_CPU_TYPE_NAME("cortex-a76"),
> > +    ARM_CPU_TYPE_NAME("cortex-a710"),
> >       ARM_CPU_TYPE_NAME("a64fx"),
> >       ARM_CPU_TYPE_NAME("neoverse-n1"),
> >       ARM_CPU_TYPE_NAME("neoverse-v1"),
>
> Will sbsa-ref want this core ?

It only has 40 PA bits, and I think sbsa-ref requires 48.

> > +static void define_cortex_a710_cp_reginfo(ARMCPU *cpu)
> > +{
> > +    /*
> > +     * The Cortex A710 has all of the Neoverse V1's IMPDEF
> > +     * registers and a few more of its own.
> > +     */
> > +    define_arm_cp_regs(cpu, neoverse_n1_cp_reginfo);
> > +    define_arm_cp_regs(cpu, neoverse_v1_cp_reginfo);
> > +    define_arm_cp_regs(cpu, cortex_a710_cp_reginfo);
>
> The TRM doesn't document the existence of these regs
> from the n1 reginfo:
>
>      { .name = "ERXPFGCDN_EL1", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 2, .opc2 = 2,
>        .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>      { .name = "ERXPFGCTL_EL1", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 2, .opc2 = 1,
>        .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>      { .name = "ERXPFGF_EL1", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 2, .opc2 = 0,
>        .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>
> This one in the v1 reginfo:
>
>      { .name = "CPUPPMCR3_EL3", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 6, .crn = 15, .crm = 2, .opc2 = 6,
>        .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>
> exists but has been renamed CPUPPMCR6_EL3, which means it's
> a duplicate of an entry in your new array. Meanwhile the
> A710's actual CPUPPMCR3_EL3 at 3, 0, c15, c2, 4 isn't in
> your new array.
>
> (I thought we had an assert to detect duplicate regdefs,
> so I'm surprised this didn't fall over.)

It did fall over.  Pre-send-email testing mistake, which I found immediately after (of 
course).

> > +    cpu->revidr = cpu->midr;         /* mirror midr: "no significance" */
>
> The bit about "no significance" is just the boilerplate text from
> the architecture manual. I think we should continue our usual
> practice of setting the revidr to 0.

Ok.

> > +    cpu->isar.id_dfr0  = 0x06011099; /* w/o FEAT_TRF */
>
> You don't have to suppress FEAT_TRF manually, we do
> it in cpu.c.

Ok.

> > +    cpu->isar.id_isar5 = 0x11011121;
>
> For isar5 we could say /* with Crypto */

Ok.

> > +    cpu->isar.id_mmfr4 = 0x21021110;
>
> I don't think we implement HPDS == 2 (that's FEAT_HPDS2).
> I guess we should push it down to HPDS 1 only in cpu.c
> for now. (Or implement it, it's probably simple.)

Feh.  I thought I'd double-checked all of the features.
I'll have a look at implementing that.

> > +    cpu->ctr               = 0x00000004b444c004ull; /* with DIC set */
>
> Why set DIC? The h/w doesn't.

Heh.  From the comment in neoverse-v1, I thought you had force enabled it there.  But it 
must simply be a h/w option?

> > +    cpu->ccsidr[0] = 0x000000ff0000001aull; /* 64KB L1 dcache */
> > +    cpu->ccsidr[1] = 0x000000ff0000001aull; /* 64KB L1 icache */
> > +    cpu->ccsidr[2] = 0x000003ff0000003aull; /* 512KB L2 cache */
>
> I was too lazy to do this for neoverse-v1, so I don't insist
> on it here, but if we're going to find ourselves calculating
> new-format ccsidr values by hand for each new CPU, I wonder if we
> should define a macro that takes numsets, assoc, linesize,
> subtracts 1 where relevant, and shifts them into the right bit
> fields? (Shame the preprocessor can't do a log2() operation ;-))

I'll create something for this.
It doesn't need to be in the preprocessor.  :-)

Thanks for the careful review.


r~