| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140425183711.GA2161@lc-sj1-5012.sj.broadcom.com>
Date: Fri, 25 Apr 2014 11:37:11 -0700
From: Zi Shen Lim <zlim@...adcom.com>
To: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
CC: Mark Brown <broonie@...aro.org>,
Catalin Marinas <Catalin.Marinas@....com>,
Mark Rutland <Mark.Rutland@....com>,
Will Deacon <Will.Deacon@....com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv2 2/2] arm64: topology: add MPIDR-based detection
Hi Lorenzo,
On Fri, Apr 25, 2014 at 12:18:43PM +0100, Lorenzo Pieralisi wrote:
> On Fri, Apr 25, 2014 at 04:18:42AM +0100, Zi Shen Lim wrote:
> > Create cpu topology based on MPIDR. When hardware sets MPIDR to sane
> > values, this method will always work. Therefore it should also work well
> > as the fallback method. [1]
>
> It has to be implemented as fallback, so you have to rebase this patch
> on top of Mark's series.
Ok, I'll rebase this patch :)
>
> > When we have multiple processing elements in the system, we create
> > the cpu topology by mapping each affinity level (from lowest to highest)
> > to threads (if they exist), cores, and clusters.
> >
> > We combine data from all higher affinity levels into cluster_id
> > so we don't lose any information from MPIDR. [2]
> >
> > [1] http://www.spinics.net/lists/arm-kernel/msg317445.html
> > [2] https://lkml.org/lkml/2014/4/23/703
> >
> > Signed-off-by: Zi Shen Lim <zlim@...adcom.com>
> > ---
> > v1->v2: Addressed comments from Mark Brown.
> > - Reduce noise. Use pr_debug instead of pr_info.
> > - Don't ignore higher affinity levels.
> >
> > arch/arm64/include/asm/cputype.h | 2 ++
> > arch/arm64/kernel/topology.c | 34 ++++++++++++++++++++++++++++++++++
> > 2 files changed, 36 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h
> > index c404fb0..7639e8b 100644
> > --- a/arch/arm64/include/asm/cputype.h
> > +++ b/arch/arm64/include/asm/cputype.h
> > @@ -18,6 +18,8 @@
> >
> > #define INVALID_HWID ULONG_MAX
> >
> > +#define MPIDR_UP_BITMASK (0x1 << 30)
> > +#define MPIDR_MT_BITMASK (0x1 << 24)
> > #define MPIDR_HWID_BITMASK 0xff00ffffff
> >
> > #define MPIDR_LEVEL_BITS_SHIFT 3
> > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > index 3e06b0b..7dbf981 100644
> > --- a/arch/arm64/kernel/topology.c
> > +++ b/arch/arm64/kernel/topology.c
> > @@ -19,6 +19,8 @@
> > #include <linux/nodemask.h>
> > #include <linux/sched.h>
> >
> > +#include <asm/cputype.h>
> > +#include <asm/smp_plat.h>
> > #include <asm/topology.h>
> >
> > /*
> > @@ -71,6 +73,38 @@ static void update_siblings_masks(unsigned int cpuid)
> >
> > void store_cpu_topology(unsigned int cpuid)
> > {
> > + struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
> > + u64 mpidr;
> > +
> > + mpidr = read_cpuid_mpidr();
> > +
> > + /* Create cpu topology mapping based on MPIDR. */
> > + if (mpidr & MPIDR_UP_BITMASK) {
> > + /* Uniprocessor system */
> > + cpuid_topo->thread_id = -1;
> > + cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> > + cpuid_topo->cluster_id = -1;
> > + } else if (mpidr & MPIDR_MT_BITMASK) {
> > + /* Multiprocessor system : Multi-threads per core */
> > + cpuid_topo->thread_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> > + cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> > + cpuid_topo->cluster_id =
> > + MPIDR_AFFINITY_LEVEL(mpidr, 2) |
> > + MPIDR_AFFINITY_LEVEL(mpidr, 3) << mpidr_hash.shift_aff[3];
>
> That's probably not what you want, even though you still end up with a
> unique cluster identifier (but insanely large) if you get lucky and it
> does not overflow an int. The shift is the amount of bits the level must be
> shift _right_ to create the hash value.
Oh oops, I should read the mpidr_hash code more carefully.
My intention of using your mpidr_hash values is to achieve something better than
the naive solution of (aff2 | (aff3 << 8)).
>
> I am wondering whether it is time for me to add those as macros.
Sure :)
>
> > + } else {
> > + /* Multiprocessor system : Single-thread per core */
> > + cpuid_topo->thread_id = -1;
> > + cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> > + cpuid_topo->cluster_id =
> > + MPIDR_AFFINITY_LEVEL(mpidr, 1) |
> > + MPIDR_AFFINITY_LEVEL(mpidr, 2) << mpidr_hash.shift_aff[2] |
> > + MPIDR_AFFINITY_LEVEL(mpidr, 3) << mpidr_hash.shift_aff[3];
>
> Ditto.
>
> > + }
> > +
> > + pr_debug("CPU%u: cluster %d core %d thread %d mpidr %llx\n",
> > + cpuid, cpuid_topo->cluster_id, cpuid_topo->core_id,
> > + cpuid_topo->thread_id, mpidr);
> > +
> > update_siblings_masks(cpuid);
>
> That's why I object. With this implementation MPIDR_EL1 takes over DT,
> and we do not want that. It has to work the other way around.
>
> What you should do, in update_sibling_masks(), check if the topology has
> been reset (ie it is not set-up), and parse the MPIDR if that's the case.
Ok, point taken. MPIDR as fallback.
In the rebased patch, I'll take care of it.
>
> Thanks,
> Lorenzo
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists