On Wed, Nov 11, 2020 at 07:10:44PM +0000, Catalin Marinas wrote:
> On Mon, Nov 09, 2020 at 09:30:18PM +0000, Will Deacon wrote:
> > +static bool has_32bit_el0(const struct arm64_cpu_capabilities *entry, int scope)
> > +{
> > + if (!has_cpuid_feature(entry, scope))
> > + return allow_mismatched_32bit_el0;
>
> I still don't like overriding the cpufeature mechanism in this way. What about
> something like below? It still doesn't fit perfectly but at least the
> capability represents what was detected in the system. We then decide in
> system_supports_32bit_el0() whether to allow asymmetry. There is an
> extra trick to park a non-AArch32 capable CPU in has_32bit_el0() if it
> comes up late and the feature has already been advertised with
> !allow_mismatched_32bit_el0.
I deliberately allow late onlining of 64-bit-only cores and I don't think
this is something we should forbid (although it's not clear from your patch
when allow_mismatched_32bit_el0 gets set). Furthermore, killing CPUs from
the matches callback feels _very_ dodgy to me, as it's invoked indirectly
by things such as this_cpu_has_cap().
> I find it clearer, though I probably stared at it more than at your
> patch ;).
Yeah, swings and roundabouts...
I think we're quibbling on implementation details a bit here whereas we
should probably be focussing on what to do about execve() and CPU hotplug.
Your patch doesn't apply on top of my series or replace this one, so there's
not an awful lot I can do with it. I'm about to post a v3 with a tentative
solution for execve(), so please could you demonstrate your idea on top of
that so I can see how it fits together?
I'd like to move on from the "I don't like this" (none of us do) discussion
and figure out the functional aspects, if possible. We can always paint it
a different colour later on, but we don't even have a full solution yet.
Thanks,
Will
