Re: [PATCH] target/arm: added cortex-a55 CPU support for qemu-virt

[Peter Maydell]

On Thu, 10 Nov 2022 at 09:04, Timofey Kutergin <tkutergin@gmail.com> wrote:
>
>   cortex-a55 is one of newer armv8.2+ CPUs supporting native
>   Privileged Access Never (PAN) feature.

Hi; thanks for this patch. There are a few missing ID register
values below, but otherwise it looks good.

> Using this CPU
>   provides access to this feature without using fictious "max"

"fictitious"

>   CPU.
>
> Signed-off-by: Timofey Kutergin <tkutergin@gmail.com>
> ---
>  hw/arm/virt.c      |  1 +
>  target/arm/cpu64.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b871350856..fc0c9baba6 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -201,6 +201,7 @@ static const char *valid_cpus[] = {
>      ARM_CPU_TYPE_NAME("cortex-a15"),
>      ARM_CPU_TYPE_NAME("cortex-a35"),
>      ARM_CPU_TYPE_NAME("cortex-a53"),
> +    ARM_CPU_TYPE_NAME("cortex-a55"),
>      ARM_CPU_TYPE_NAME("cortex-a57"),
>      ARM_CPU_TYPE_NAME("cortex-a72"),
>      ARM_CPU_TYPE_NAME("cortex-a76"),

There's a corresponding list of supported CPUs in
docs/system/arm/virt.rst that also needs the new CPU type adding.

> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 3d74f134f5..e1ade1b2a3 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -792,6 +792,60 @@ static void aarch64_a53_initfn(Object *obj)
>      define_cortex_a72_a57_a53_cp_reginfo(cpu);
>  }
>
> +static void aarch64_a55_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    cpu->dtb_compatible = "arm,cortex-a55";
> +    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);

It's helpful for review to order the ID register values in
the same layout they are in the TRM, in the "AArch64 registers
by functional group" table, which is how we've ordered other
new CPUs we've added recently (eg see the Neoverse N1 or
the Cortex-A76).

> +    cpu->midr = 0x411fd050;

There's an r2p0 TRM out, so we might as well advertise
ourselves as r2p0:  0x412FD050

> +    cpu->revidr = 0x00000000;
> +    cpu->reset_fpsid = 0x41034070;
> +    cpu->isar.mvfr0 = 0x10110222;
> +    cpu->isar.mvfr1 = 0x13211111;
> +    cpu->isar.mvfr2 = 0x00000043;
> +    cpu->ctr = 0x84448004; /* L1Ip = VIPT */
> +    cpu->reset_sctlr = 0x00c50838;

Should be 0x30c50838 : bits [29:28] are RES1

> +    cpu->isar.id_pfr0 = 0x10000131;
> +    cpu->isar.id_pfr1 = 0x00011011;

Forgotten id_pfr2: 0x00000011

> +    cpu->isar.id_dfr0 = 0x04010088;
> +    cpu->id_afr0 = 0x00000000;
> +    cpu->isar.id_mmfr0 = 0x10201105;
> +    cpu->isar.id_mmfr1 = 0x40000000;
> +    cpu->isar.id_mmfr2 = 0x01260000;
> +    cpu->isar.id_mmfr3 = 0x02122211;

You've forgotten id_mmfr4 : 0x00021110

> +    cpu->isar.id_isar0 = 0x02101110;
> +    cpu->isar.id_isar1 = 0x13112111;
> +    cpu->isar.id_isar2 = 0x21232042;
> +    cpu->isar.id_isar3 = 0x01112131;
> +    cpu->isar.id_isar4 = 0x00011142;
> +    cpu->isar.id_isar5 = 0x01011121;
> +    cpu->isar.id_isar6 = 0x00000010;
> +    cpu->isar.id_aa64pfr0 = 0x10112222;

You've missed out id_aa64pfr1 : should be 0x0000000000000010ull

> +    cpu->isar.id_aa64dfr0 = 0x10305408;
> +    cpu->isar.id_aa64isar0 = 0x10211120;

You've missed out the DP field: this should be
0x0000100010211120ull

> +    cpu->isar.id_aa64isar1 = 0x00100001;
> +    cpu->isar.id_aa64mmfr0 = 0x00101122;
> +    cpu->isar.id_aa64mmfr1 = 0x10212122;
> +    cpu->isar.id_aa64mmfr2 = 0x00001011;
> +    cpu->isar.dbgdidr = 0x3516d000;
> +    cpu->clidr = 0x82000023;
> +    cpu->ccsidr[0] = 0x700fe01a; /* 32KB L1 dcache */
> +    cpu->ccsidr[1] = 0x200fe01a; /* 32KB L1 icache */
> +    cpu->ccsidr[2] = 0x703fe07a; /* 512KB L2 cache */

The A55 TRM says that it has an L3 cache, so I think we
also need to fill in cpu->ccsidr[4] here (NB not [3], which
is a reserved index since the L2 is not split I&D).

> +    cpu->dcz_blocksize = 4; /* 64 bytes */
> +    cpu->gic_num_lrs = 4;
> +    cpu->gic_vpribits = 5;
> +    cpu->gic_vprebits = 5;

You need to set gic_pribits also.

Missing reset_pmcr_el0: 0x41453000

The impdef sysregs are different from A53/A57/A72 etc,
so it's correct that we don't call
define_cortex_a72_a57_a53_cp_reginfo(). We can add definitions
of the impdef sysregs later if guest software runs into them
I guess...

thanks
-- PMM