diff mbox

[RFC,V2,1/4] Use Aff1 with mpidr

Message ID 1430921082-16779-2-git-send-email-shlomopongratz@gmail.com
State New
Headers show

Commit Message

Shlomo Pongratz May 6, 2015, 2:04 p.m. UTC
From: Shlomo Pongratz <shlomo.pongratz@huawei.com>

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 <shlomo.pongratz@huawei.com>
---
 target-arm/helper.c | 12 ++++++++++--
 target-arm/psci.c   | 18 ++++++++++++++++--
 2 files changed, 26 insertions(+), 4 deletions(-)

Comments

Pavel Fedin May 21, 2015, 3:54 p.m. UTC | #1
Hello!

> 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

 I have found one more concern. Are you really sure about this scheme ? I am currently
experimenting with KVM, and it seems to have 16 CPUs per cluster, at least on my machine.
Actually i suggest that KVM inherits the mapping from the host. Can we do the same?
 I will take a look at kvmtool, how it determines these IDs. But hardcoded scheme is
definitely wrong.

 Cc'ed Ashok because he might also be interested.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Shlomo Pongratz May 21, 2015, 4:20 p.m. UTC | #2
On Thursday, May 21, 2015, Pavel Fedin <p.fedin@samsung.com> wrote:

>  Hello!
>
> > 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
>
>  I have found one more concern. Are you really sure about this scheme ? I
> am currently
> experimenting with KVM, and it seems to have 16 CPUs per cluster, at least
> on my machine.
> Actually i suggest that KVM inherits the mapping from the host. Can we do
> the same?
>  I will take a look at kvmtool, how it determines these IDs. But hardcoded
> scheme is
> definitely wrong.
>
>  Cc'ed Ashok because he might also be interested.
>
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
>
> Hi Pavel,

I can only quote from GIC-500 document (
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0516b/DDI0516B_gic5000_r0p0_trm.pdf)
which is currently the only GICv3 implementation) I'll recheck the GICv3
when I'll return to work on Monday.

From section 1.3 features.

The GIC-500 provides registers for managing interrupt sources, interrupt
behavior, and interrupt
routing to one or more cores. It supports:
• Multiprocessor environments with up to 128 cores.
• Up to 32 affinity-level 1 clusters.
• Up to eight cores for each cluster.

I guess your hardware uses different GIC.

Best regards,

S.P.
Pavel Fedin May 22, 2015, 6:49 a.m. UTC | #3
Hello!

> The GIC-500 provides registers for managing interrupt sources, interrupt behavior, and interrupt
> routing to one or more cores. It supports:
> • Multiprocessor environments with up to 128 cores.
> • Up to 32 affinity-level 1 clusters.
> • Up to eight cores for each cluster.

> I guess your hardware uses different GIC.

 Heh, yes, looks like that. And perhaps it's somewhat non-standard...
 I will study kvmtool and try to come up with some good solution.

 By the way, since you're referring to documentation... TRM you have mentioned contains references to "GIC architecture reference manual v3.0", which i was unable to find. On ARM resource center i see only v2 of the manual. And it looks like you have it because otherwise you would not get description of many registers. Can you point me at a correct place ?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Shlomo Pongratz May 23, 2015, 9:22 a.m. UTC | #4
On Friday, May 22, 2015, Pavel Fedin <p.fedin@samsung.com> wrote:

>  Hello!
>
> > The GIC-500 provides registers for managing interrupt sources, interrupt
> behavior, and interrupt
> > routing to one or more cores. It supports:
> > • Multiprocessor environments with up to 128 cores.
> > • Up to 32 affinity-level 1 clusters.
> > • Up to eight cores for each cluster.
>
> > I guess your hardware uses different GIC.
>
>  Heh, yes, looks like that. And perhaps it's somewhat non-standard...
>  I will study kvmtool and try to come up with some good solution.
>
>  By the way, since you're referring to documentation... TRM you have
> mentioned contains references to "GIC architecture reference manual v3.0",
> which i was unable to find. On ARM resource center i see only v2 of the
> manual. And it looks like you have it because otherwise you would not get
> description of many registers. Can you point me at a correct place ?
>
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
>
>
> Hi Pavel,

We have a copy at work which I came from ARM, I'm sure that since Samsung
manufactures ARM SoC under licence you can put your hand on one.
I can also add that most of my work was done using the GIC-500 document and
by looking for what the Linux (kernel) is doing. Only recently I was able
to put my hand on the GICv3, but It only confirmed what I already assumed.

Best regards,

S.P.
Igor Mammedov May 27, 2015, 4:12 p.m. UTC | #5
On Wed,  6 May 2015 17:04:39 +0300
shlomopongratz@gmail.com wrote:

> From: Shlomo Pongratz <shlomo.pongratz@huawei.com>
> 
> 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 <shlomo.pongratz@huawei.com>
> ---
>  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.

> +    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;
Pavel Fedin May 27, 2015, 6:03 p.m. UTC | #6
Hello!

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

 I have redone this thing from scratch:
https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg04495.html
My implementation does exactly this. It simply introduces CPU-specific MPIDR value which
CPUs can set as they wish. Also this is used by KVM because KVM has its own ideas about
how CPUs are clustered.
I posted that without RFC, because i think it's ready for application, but my message
seems to have been lost. Looks like i forgot Cc to the maintainer. My current vGICv3
work-in-progress is based on this.
 If Shlomo does not respond until friday, i think i'll post a new vGICv3 series based on
master.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Shannon Zhao May 28, 2015, 12:53 a.m. UTC | #7
On 2015/5/28 2:03, Pavel Fedin wrote:
>  Hello!
> 
>> 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.
> 
>  I have redone this thing from scratch:
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg04495.html
> My implementation does exactly this. It simply introduces CPU-specific MPIDR value which
> CPUs can set as they wish.

IIUC, your implementation also uses 8 cpus per cluster which is not
consistent with CPU spec.

> Also this is used by KVM because KVM has its own ideas about
> how CPUs are clustered.
> I posted that without RFC, because i think it's ready for application, but my message
> seems to have been lost. Looks like i forgot Cc to the maintainer. My current vGICv3
> work-in-progress is based on this.
>  If Shlomo does not respond until friday, i think i'll post a new vGICv3 series based on
> master.
> 

I guess we should give people time to do the respin or other things.

> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 
> 
>
Pavel Fedin May 28, 2015, 6:34 a.m. UTC | #8
Hi!

> IIUC, your implementation also uses 8 cpus per cluster which is not
> consistent with CPU spec.

 Yes, i have inherited the value from Shlomo's implementation. To tell the truth i don't
have time to read through all the docs. And i don't know in which PDF i should look for
this info. But, my implementation is very easy to tune. Just change the definition.
Actually it could be even made into static variable so that machines could set it before
instantiating CPUs.
 And my real HW has 16 CPUs per cluster, but i thought it's implementation-specific. Isn't
it ?

> I guess we should give people time to do the respin or other things.

 I am giving it. A week has passed. Actually one of goals of my project here at Samsung is
to upstream our changes, that's why i worry about it.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Shlomo Pongratz May 28, 2015, 7:23 a.m. UTC | #9
> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: Wednesday, 27 May, 2015 7:12 PM
> To: shlomopongratz@gmail.com
> Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; Shlomo Pongratz
> Subject: Re: [Qemu-devel] [PATCH RFC V2 1/4] Use Aff1 with mpidr
> 
> On Wed,  6 May 2015 17:04:39 +0300
> shlomopongratz@gmail.com wrote:
> 
> > From: Shlomo Pongratz <shlomo.pongratz@huawei.com>
> >
> > 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 <shlomo.pongratz@huawei.com>
> > ---
> >  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.

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;
Igor Mammedov May 28, 2015, 9:30 a.m. UTC | #10
On Thu, 28 May 2015 07:23:55 +0000
Shlomo Pongratz <shlomo.pongratz@huawei.com> wrote:

> 
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: Wednesday, 27 May, 2015 7:12 PM
> > To: shlomopongratz@gmail.com
> > Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; Shlomo Pongratz
> > Subject: Re: [Qemu-devel] [PATCH RFC V2 1/4] Use Aff1 with mpidr
> > 
> > On Wed,  6 May 2015 17:04:39 +0300
> > shlomopongratz@gmail.com wrote:
> > 
> > > From: Shlomo Pongratz <shlomo.pongratz@huawei.com>
> > >
> > > 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 <shlomo.pongratz@huawei.com>
> > > ---
> > >  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.

> 
> 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;
>
Shlomo Pongratz May 28, 2015, 12:02 p.m. UTC | #11
See below.

> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: Thursday, 28 May, 2015 12:30 PM
> To: Shlomo Pongratz
> Cc: shlomopongratz@gmail.com; qemu-devel@nongnu.org;
> peter.maydell@linaro.org
> Subject: Re: [Qemu-devel] [PATCH RFC V2 1/4] Use Aff1 with mpidr
> 
> On Thu, 28 May 2015 07:23:55 +0000
> Shlomo Pongratz <shlomo.pongratz@huawei.com> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > > Sent: Wednesday, 27 May, 2015 7:12 PM
> > > To: shlomopongratz@gmail.com
> > > Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; Shlomo
> Pongratz
> > > Subject: Re: [Qemu-devel] [PATCH RFC V2 1/4] Use Aff1 with mpidr
> > >
> > > On Wed,  6 May 2015 17:04:39 +0300
> > > shlomopongratz@gmail.com wrote:
> > >
> > > > From: Shlomo Pongratz <shlomo.pongratz@huawei.com>
> > > >
> > > > 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 <shlomo.pongratz@huawei.com>
> > > > ---
> > > >  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;
> >
Pavel Fedin May 28, 2015, 2:10 p.m. UTC | #12
Hello!

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

 Sorry for perhaps being too insisting, but have you seen my implementation? Nobody seems
to comment on it...
 It is very easy to convert into what you are suggesting. It already has CPU_PER_CORES
parameter which is currently a #define. If you turn it into a global variable, this will
be the only change you need. It would default to 8 so that old machines don't need to be
changed. But you could set it from inside virt before instantiating CPUs if you want to
change it.
 When CPUs are instantiated their IDs will be auto-generated from index (raw order number)
according to this parameter.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Shlomo Pongratz June 1, 2015, 10:59 a.m. UTC | #13
> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: Wednesday, 27 May, 2015 7:12 PM
> To: shlomopongratz@gmail.com
> Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; Shlomo Pongratz
> Subject: Re: [Qemu-devel] [PATCH RFC V2 1/4] Use Aff1 with mpidr
> 
> On Wed,  6 May 2015 17:04:39 +0300
> shlomopongratz@gmail.com wrote:
> 
> > From: Shlomo Pongratz <shlomo.pongratz@huawei.com>
> >
> > 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 <shlomo.pongratz@huawei.com>
> > ---
> >  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.

Assume we let the processor select its mpidr than for cortex-57 it will select 0-3 now how a system with GICv2 with 8 cores will work?
Note that GICv2 that doesn't affinity levels. In the GIC-500 it is written that with GICv2 backwards compatibility " This mode supports up to eight cores. The eight cores supported 
by the GIC-500 are the cores with the lowest affinity numbers". So all 8 cores must have aff0 0 to 7.
It is obvious that with GICv3 if we have 2 cortex-a57 we should have affinities 0.0.0.{0-3} and 0.0.1.{0-3} but with GICv2 we should have 0.0.0.{0-7}.
Now the only component that knows which GIC is used is the virt, it can set a property telling the GIC and the system what is the size of the lowest level affinity.
I think this number is the upper power of two of the SMP number divided by the (GICD_TYPER.CPUNumber + 1) but I'm not sure.

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;
Igor Mammedov June 2, 2015, 3:44 p.m. UTC | #14
On Thu, 28 May 2015 12:02:27 +0000
Shlomo Pongratz <shlomo.pongratz@huawei.com> wrote:

> See below.
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: Thursday, 28 May, 2015 12:30 PM
> > To: Shlomo Pongratz
> > Cc: shlomopongratz@gmail.com; qemu-devel@nongnu.org;
> > peter.maydell@linaro.org
> > Subject: Re: [Qemu-devel] [PATCH RFC V2 1/4] Use Aff1 with mpidr
> > 
> > On Thu, 28 May 2015 07:23:55 +0000
> > Shlomo Pongratz <shlomo.pongratz@huawei.com> wrote:
> > 
> > >
> > >
> > > > -----Original Message-----
> > > > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > > > Sent: Wednesday, 27 May, 2015 7:12 PM
> > > > To: shlomopongratz@gmail.com
> > > > Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; Shlomo
> > Pongratz
> > > > Subject: Re: [Qemu-devel] [PATCH RFC V2 1/4] Use Aff1 with mpidr
> > > >
> > > > On Wed,  6 May 2015 17:04:39 +0300
> > > > shlomopongratz@gmail.com wrote:
> > > >
> > > > > From: Shlomo Pongratz <shlomo.pongratz@huawei.com>
> > > > >
> > > > > 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 <shlomo.pongratz@huawei.com>
> > > > > ---
> > > > >  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.
on x86 APIC ID used to be derived for cpu_index as well like it is done in ARM for mpidr, but it has been decoupled form it, converted to property and
set by board code now which is topology aware.
Perhaps the same should be done for ARM.

> 
> 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;
> > >
> 
>
Igor Mammedov June 2, 2015, 3:51 p.m. UTC | #15
On Mon, 1 Jun 2015 10:59:35 +0000
Shlomo Pongratz <shlomo.pongratz@huawei.com> wrote:

> 
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: Wednesday, 27 May, 2015 7:12 PM
> > To: shlomopongratz@gmail.com
> > Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; Shlomo Pongratz
> > Subject: Re: [Qemu-devel] [PATCH RFC V2 1/4] Use Aff1 with mpidr
> > 
> > On Wed,  6 May 2015 17:04:39 +0300
> > shlomopongratz@gmail.com wrote:
> > 
> > > From: Shlomo Pongratz <shlomo.pongratz@huawei.com>
> > >
> > > 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 <shlomo.pongratz@huawei.com>
> > > ---
> > >  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.
> 
> Assume we let the processor select its mpidr than for cortex-57 it will select 0-3 now how a system with GICv2 with 8 cores will work?
> Note that GICv2 that doesn't affinity levels. In the GIC-500 it is written that with GICv2 backwards compatibility " This mode supports up to eight cores. The eight cores supported 
> by the GIC-500 are the cores with the lowest affinity numbers". So all 8 cores must have aff0 0 to 7.
> It is obvious that with GICv3 if we have 2 cortex-a57 we should have affinities 0.0.0.{0-3} and 0.0.1.{0-3} but with GICv2 we should have 0.0.0.{0-7}.
Where in spec it says that every CPU must have aff0 {0-7}?
I've read it as GIC MIGHT support upto 8 cores provided one implements
CPU with 8-cores, i.e. aff0 possible values are defined by used CPU type.
Neither Arch64/A53/57/72 support more than 4 cores by spec so far.

> Now the only component that knows which GIC is used is the virt, it can set a property telling the GIC and the system what is the size of the lowest level affinity.
> I think this number is the upper power of two of the SMP number divided by the (GICD_TYPER.CPUNumber + 1) but I'm not sure.
> 
> 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;
>
Pavel Fedin June 3, 2015, 10:51 a.m. UTC | #16
Hello!

> 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

 Why does it have to be a part of CPUARMState ? Isn't it a global parameter?

> on x86 APIC ID used to be derived for cpu_index as well like it is done in ARM for
mpidr, but it
> has been decoupled form it, converted to property and
> set by board code now which is topology aware.

 Can you point at the code? I'm not familiar with x86 stuff.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
diff mbox

Patch

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