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

[Peter Maydell]

On Thu, 10 Aug 2023 at 18:05, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> 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.

Yes, it does want 48 (we ran into that with some other core).

> >> +    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.

I think we (meaning Linaro) kind of noted a lot of features
as architecturally optional and then didn't think through
that we might need them anyway for specific implementations.
(I got surprised by FEAT_NV that way for the Neoverse-V1.)

> >> +    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?

Yes, the Neoverse-V1 TRM documents a config option of
"instruction cache hardware coherency" (which sets DIC),
and that the IDC pin "reflects the inverse value of the
BROADCASTCACHEMAINTPOU pin". So I opted for the config
choices that happen to be faster for QEMU.

thanks
-- PMM