Re: [PATCH v2 01/10] ppc/pseries: Add Power11 cpu type

[Cédric Le Goater]

On 4/26/24 19:05, Aditya Gupta wrote:
> Hello Cédric,
>
> Thanks for your reviews.
>
> On Fri, Apr 26, 2024 at 04:27:04PM +0200, Cédric Le Goater wrote:
> > Hello Aditya
> >
> > On 4/26/24 13:00, Aditya Gupta wrote:
> > > Add base support for "--cpu power11" in QEMU.
> > >
> > > Power11 core is same as Power10, hence reuse functions defined for
> > > Power10.
> >
> > Power11 uses the same ISA it seems. What's the value then ?
>
> Yes, it uses the same ISA. But I added this option so we can have a
> Power11 PVR in QEMU, which should be identified as Power11 in skiboot
> and linux, hence defined Power11 cpu type, even though code here is
> almost same as Power10.
>
> > > Cc: Cédric Le Goater <clg@kaod.org>
> > > Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > Cc: David Gibson <david@gibson.dropbear.id.au>
> > > Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > > Cc: Mahesh J Salgaonkar <mahesh@linux.ibm.com>
> > > Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
> > > Cc: Nicholas Piggin <npiggin@gmail.com>
> > > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> > > ---
> > >    docs/system/ppc/pseries.rst |  6 +--
> > >    hw/ppc/spapr_cpu_core.c     |  1 +
> >
> >
> > I would separate the CPU target code adding support for a new POWER
> > Processor from the machine code (pseries).
>
> Sure, I will split it in v3.
>
> > >    target/ppc/compat.c         |  7 +++
> > >    target/ppc/cpu-models.c     |  2 +
> > >    target/ppc/cpu-models.h     |  2 +
> > >    target/ppc/cpu_init.c       | 99 +++++++++++++++++++++++++++++++++++++
> > >    6 files changed, 114 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/docs/system/ppc/pseries.rst b/docs/system/ppc/pseries.rst
> > > index a876d897b6e4..3277564b34c2 100644
> > > --- a/docs/system/ppc/pseries.rst
> > > +++ b/docs/system/ppc/pseries.rst
> > > @@ -15,9 +15,9 @@ Supported devices
> > >    =================
> > >     * Multi processor support for many Power processors generations: POWER7,
> > > -   POWER7+, POWER8, POWER8NVL, POWER9, and Power10. Support for POWER5+ exists,
> > > -   but its state is unknown.
> > > - * Interrupt Controller, XICS (POWER8) and XIVE (POWER9 and Power10)
> > > +   POWER7+, POWER8, POWER8NVL, POWER9, Power10 and Power11. Support for POWER5+
> > > +   exists, but its state is unknown.
> >
> > The POWER5+ pseries machine seems functionnal with SLOF
> > (Sep 18 2023 18:57:48) and Linux 6.6.3 under TCG. May be worth
> > to mention (for AIX users) in another patch.
> >
> > > + * Interrupt Controller, XICS (POWER8) and XIVE (POWER9, Power10, Power11)
> > >     * vPHB PCIe Host bridge.
> > >     * vscsi and vnet devices, compatible with the same devices available on a
> > >       PowerVM hypervisor with VIOS managing LPARs.
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index e7c9edd033c8..c6e85c031178 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -401,6 +401,7 @@ static const TypeInfo spapr_cpu_core_type_infos[] = {
> > >        DEFINE_SPAPR_CPU_CORE_TYPE("power9_v2.0"),
> > >        DEFINE_SPAPR_CPU_CORE_TYPE("power9_v2.2"),
> > >        DEFINE_SPAPR_CPU_CORE_TYPE("power10_v2.0"),
> > > +    DEFINE_SPAPR_CPU_CORE_TYPE("power11"),
> > >    #ifdef CONFIG_KVM
> > >        DEFINE_SPAPR_CPU_CORE_TYPE("host"),
> > >    #endif
> > > diff --git a/target/ppc/compat.c b/target/ppc/compat.c
> > > index ebef2cccecf3..12dd8ae290ca 100644
> > > --- a/target/ppc/compat.c
> > > +++ b/target/ppc/compat.c
> > > @@ -100,6 +100,13 @@ static const CompatInfo compat_table[] = {
> > >            .pcr_level = PCR_COMPAT_3_10,
> > >            .max_vthreads = 8,
> > >        },
> > > +    { /* POWER11, ISA3.10 */
> > > +        .name = "power11",
> > > +        .pvr = CPU_POWERPC_LOGICAL_3_10_PLUS,
> > > +        .pcr = PCR_COMPAT_3_10,
> > > +        .pcr_level = PCR_COMPAT_3_10,
> > > +        .max_vthreads = 8,
> > > +    },
> > >    };
> > >    static const CompatInfo *compat_by_pvr(uint32_t pvr)
> > > diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
> > > index f2301b43f78b..1870e69b63df 100644
> > > --- a/target/ppc/cpu-models.c
> > > +++ b/target/ppc/cpu-models.c
> > > @@ -734,6 +734,8 @@
> > >                    "POWER9 v2.2")
> > >        POWERPC_DEF("power10_v2.0",  CPU_POWERPC_POWER10_DD20,           POWER10,
> > >                    "POWER10 v2.0")
> > > +    POWERPC_DEF("power11",  CPU_POWERPC_POWER11,           POWER11,
> > > +                "POWER11")
> > >    #endif /* defined (TARGET_PPC64) */
> > >    /***************************************************************************/
> > > diff --git a/target/ppc/cpu-models.h b/target/ppc/cpu-models.h
> > > index 0229ef3a9a5c..a1b540c3aa9e 100644
> > > --- a/target/ppc/cpu-models.h
> > > +++ b/target/ppc/cpu-models.h
> > > @@ -354,6 +354,7 @@ enum {
> > >        CPU_POWERPC_POWER10_BASE       = 0x00800000,
> > >        CPU_POWERPC_POWER10_DD1        = 0x00801100,
> > >        CPU_POWERPC_POWER10_DD20       = 0x00801200,
> > > +    CPU_POWERPC_POWER11            = 0x00821200,
> >
> > is that a DD2.2 PVR ? If so, It should be mentionned in the definition.
>
> Yes, I have kept the last 2 bytes same as P10 DD2. I will mention it
> above the line I have added it, in v3.

Skiboot reports :

[    0.121234172,6] P11 DD1.00 detected


C.