Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Igor Mammedov]

On Sun, 31 May 2015 11:03:53 +0000
Shlomo Pongratz <address@hidden> wrote:

> Hi,
> 
> See below.
> 
> > -----Original Message-----
> > From: Pavel Fedin [mailto:address@hidden
> > Sent: Friday, 29 May, 2015 9:45 AM
> > To: Shlomo Pongratz; address@hidden
> > Cc: address@hidden; 'Ashok Kumar'
> > Subject: RE: [PATCH] Use Aff1 with mpidr
> > 
> >  Hi!
> > 
> > > I see that you added mpidr to ARMCpu and initialized it in virt.c then
> > > you use it in
> > mpidr_read.
> > > Thus you must fix all other virtual machines in hw/arm not just virt.c
> > > as it is not
> > initialized for
> > > them.
> > 
> >  The only change in virt.c is:
> > --- cut ---
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c index a7f9a10..a1186c5 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -317,7 +317,11 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo
> > *vbi)
> >                                          "enable-method", "psci");
> >          }
> > 
> > -        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", cpu);
> > +        /*
> > +         * If cpus node's #address-cells property is set to 1
> > +         * The reg cell bits [23:0] must be set to bits [23:0] of 
> > MPIDR_EL1.
> > +         */
> > +        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg",
> > + armcpu->mpidr);
> >          g_free(nodename);
> >      }
> >  }
> > --- cut ---
> >  So that it takes MPIDR instead of just CPU index. Theoretically - yes, may 
> > be
> > other machines should also be changed, but:
> > 1. They are 32-bit, so MPIDR == index for them, because there are no more
> > than 8 CPUs.
> > 2. Those machines AFAIK do not compose device tree by themselves. They
> > use pre-made ones instead, coming for example with kernel.
> >  Only virt currently can be a 64-bit machine and cares about more than 8
> > CPUs.
> >  As to MPIDR initialization, it is done in arm_cpu_initfn(), therefore all 
> > ARM
> > CPUs get this automatically. There's no need to modify code for every
> > machine.
> 
> I think we should take Igor's comment into account. The CPUS_PER_CLUSTER 
> should not be a constant, and maybe should be initialized in arm_cpu_initfn 
> and aarch64_{a53|a57}_initfn, as psci need to know it.

I has been suggesting something like following to allow every CPU type override
affinity definitions:

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index ed5a644..f309699 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -181,6 +181,7 @@ typedef struct AArch64CPUClass {
     /*< private >*/
     ARMCPUClass parent_class;
     /*< public >*/
+    void (*get_affinity)(ARMCPU *cpu, uint8_t *aff1, uint8_t *aff2, uint8_t 
*aff3)
 } AArch64CPUClass;
 
 static inline ARMCPU *arm_env_get_cpu(CPUARMState *env)
diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index 270bc2f..bf19efe 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -235,7 +235,9 @@ static void aarch64_cpu_set_pc(CPUState *cs, vaddr value)
 static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
 {
     CPUClass *cc = CPU_CLASS(oc);
+    AArch64CPUClass *acc = ARMCPUClass(oc);
 
+    acc->get_affinity = coretex_a[57|53|..]_get_affinity;
 #if !defined(CONFIG_USER_ONLY)
     cc->do_interrupt = aarch64_cpu_do_interrupt;
 #endif