Re: [Qemu-devel] [PATCH RFC V2 1/4] Use Aff1 with mpidr

[Shlomo Pongratz]

See below.

> -----Original Message-----
> From: Igor Mammedov [mailto:address@hidden
> Sent: Thursday, 28 May, 2015 12:30 PM
> To: Shlomo Pongratz
> Cc: address@hidden; address@hidden;
> address@hidden
> Subject: Re: [Qemu-devel] [PATCH RFC V2 1/4] Use Aff1 with mpidr
> 
> On Thu, 28 May 2015 07:23:55 +0000
> Shlomo Pongratz <address@hidden> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: Igor Mammedov [mailto:address@hidden
> > > Sent: Wednesday, 27 May, 2015 7:12 PM
> > > To: address@hidden
> > > Cc: address@hidden; address@hidden; Shlomo
> Pongratz
> > > Subject: Re: [Qemu-devel] [PATCH RFC V2 1/4] Use Aff1 with mpidr
> > >
> > > On Wed,  6 May 2015 17:04:39 +0300
> > > address@hidden wrote:
> > >
> > > > From: Shlomo Pongratz <address@hidden>
> > > >
> > > > In order to support up to 128 cores with GIC-500 (GICv3
> > > > implementation)
> > > > affinity1 must be used. GIC-500 support up to 32 clusters with up
> > > > to
> > > > 8 cores in a cluster. So for example, if one wishes to have 16
> > > > cores, the options are: 2 clusters of 8 cores each, 4 clusters
> > > > with 4 cores each Currently only the first option is supported.
> > > > In order to have more flexible scheme the virt machine must pass
> > > > the desired scheme.
> > > >
> > > > Signed-off-by: Shlomo Pongratz <address@hidden>
> > > > ---
> > > >  target-arm/helper.c | 12 ++++++++++--
> > > >  target-arm/psci.c   | 18 ++++++++++++++++--
> > > >  2 files changed, 26 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/target-arm/helper.c b/target-arm/helper.c index
> > > > f8f8d76..f555c0b 100644
> > > > --- a/target-arm/helper.c
> > > > +++ b/target-arm/helper.c
> > > > @@ -2035,11 +2035,19 @@ static const ARMCPRegInfo
> > > > strongarm_cp_reginfo[] = {  static uint64_t mpidr_read(CPUARMState
> > > > *env, const ARMCPRegInfo *ri)  {
> > > >      CPUState *cs = CPU(arm_env_get_cpu(env));
> > > > -    uint32_t mpidr = cs->cpu_index;
> > > > -    /* We don't support setting cluster ID ([8..11]) (known as Aff1
> > > > +    uint32_t mpidr, aff0, aff1;
> > > > +    uint32_t cpuid = cs->cpu_index;
> > > > +    /* We don't support setting cluster ID ([16..23]) (known as
> > > > + Aff2
> > > >       * in later ARM ARM versions), or any of the higher affinity level
> fields,
> > > >       * so these bits always RAZ.
> > > >       */
> > > > +    /* Currently GIC-500 code supports 64 cores in 16 clusters
> > > > + with 8 cores
> > > each
> > > > +     * Future code will remove this limitation.
> > > > +     * This code is valid for GIC-400 too.
> > > > +     */
> > > > +    aff0 = cpuid % 8;
> > > Even though GIC-500 spec says that it supports 8 cores I haven't
> > > found in it how it encodes aff0 but encoding is specified by
> > > respective CPU specs. I've checked a53, a57, a72 specs and they limit aff0
> to max 0x3 value.
> > > I think encoding should be CPU type specific i.e. not defined by
> > > what GIC can support and once we add CPU type with 8 cores, it would
> > > provide it's own version of mpidr_read since it would be defined by spec
> how to encode aff0.
> > >
> >
> > Hi,
> >
> > I wonder how this will work with GICv2 with 8 cores.
> > I agree that with GICv3 and 2 cortex-57 the affinities should be
> > 0.0.0.{0..3} and 0.0.1.{0..3} but with GICv2 or GICv3 in backwards
> compatibility we should have 0.0.0.{0..7}. Now only the virt knows which GIC
> is been used and psci is ignorant.
> > I assume the virt can set a property to the GIC and the QEMU saying how
> many cores are in the lowest affinity level.
> > That should be 0..7 with GICv2 according to the SMP value and processor
> dependent for GICV3.
> I think aff0 format is CPU dependent regardless of which GIC is used since
> mpidr is CPU owned register.
> 

O.K. so let's see.
In virt.c::fdt_add_cpu_node we set aff1 to be cpu_number / #cores-in-soc and 
aff0 to be cpu_numer % #cores-is-soc. This is fairly.
In  helpr.c:mpidr_read we calculate aff0 & aff1 from cs->cpu_index in the same 
way but AFAIK the input parameter i.e. CPUARMState doesn't has the 
#cores-in-soc nor any other useful information. We can add this field and 
initialize it in virt.c but then we need to touch all other machines. Or we can 
say that if this number is zero than there is no aff1 and aff0 is 
cs->cpu_index. Please note that this is not according to spec but this is the 
current implementation (before the GICv3 patch) so according to the purists the 
current implementation with GICv2 should be limited to 4 cortex-a57 cores!
In psci.c we need to break the aff0 & aff1 that the kernel is giving us back to 
linear number for this we again need the #cores-is-soc, but again all we have 
is ARMCPU and CPUARMState.

Or we can say that we let aff0 be 8 even for cortex-a57.

So the question is; what is the best way to go?

Best regards,

S.P.

> >
> > Best regards,
> >
> > S.P.
> >
> >
> > > > +    aff1 = cpuid / 8;
> > > > +    mpidr = (aff1 << 8) | aff0;
> > > >      if (arm_feature(env, ARM_FEATURE_V7MP)) {
> > > >          mpidr |= (1U << 31);
> > > >          /* Cores which are uniprocessor (non-coherent) diff --git
> > > > a/target-arm/psci.c b/target-arm/psci.c index d8fafab..64fbe61
> > > > 100644
> > > > --- a/target-arm/psci.c
> > > > +++ b/target-arm/psci.c
> > > > @@ -86,6 +86,7 @@ void arm_handle_psci_call(ARMCPU *cpu)
> > > >      CPUARMState *env = &cpu->env;
> > > >      uint64_t param[4];
> > > >      uint64_t context_id, mpidr;
> > > > +    uint32_t core, Aff1, Aff0;
> > > >      target_ulong entry;
> > > >      int32_t ret = 0;
> > > >      int i;
> > > > @@ -121,7 +122,16 @@ void arm_handle_psci_call(ARMCPU *cpu)
> > > >
> > > >          switch (param[2]) {
> > > >          case 0:
> > > > -            target_cpu_state = qemu_get_cpu(mpidr & 0xff);
> > > > +            /* MPIDR_EL1 [RES0:affinity-3:RES1:U:RES0:MT:affinity-
> 1:affinity-0]
> > > > +             * GIC 500 code currently supports 32 clusters with 8 
> > > > cores each
> > > > +             * but no more than 128 cores. Future version will have 
> > > > flexible
> > > > +             * affinity selection
> > > > +             * GIC 400 supports 8 cores so 0x7 for Aff0 is O.K. too
> > > > +             */
> > > > +            Aff1 = (mpidr & 0xff00) >> (8 - 3); /* Shift by 8 multiply 
> > > > by 8 */
> > > > +            Aff0 = mpidr & 0x7;
> > > > +            core = Aff1 + Aff0;
> > > > +            target_cpu_state = qemu_get_cpu(core);
> > > >              if (!target_cpu_state) {
> > > >                  ret = QEMU_PSCI_RET_INVALID_PARAMS;
> > > >                  break;
> > > > @@ -153,7 +163,11 @@ void arm_handle_psci_call(ARMCPU *cpu)
> > > >          context_id = param[3];
> > > >
> > > >          /* change to the cpu we are powering up */
> > > > -        target_cpu_state = qemu_get_cpu(mpidr & 0xff);
> > > > +        /* Currently supports 64 cores in 16 clusters with 8 cores 
> > > > each */
> > > > +        Aff1 = (mpidr & 0xff00) >> (8 - 3); /* Shift by 8 multiply by 
> > > > 8 */
> > > > +        Aff0 = mpidr & 0x7;
> > > > +        core = Aff1 + Aff0;
> > > > +        target_cpu_state = qemu_get_cpu(core);
> > > >          if (!target_cpu_state) {
> > > >              ret = QEMU_PSCI_RET_INVALID_PARAMS;
> > > >              break;
> >