On Wed, Oct 28, 2020 at 06:56:21PM +0000, Catalin Marinas wrote:
> On Wed, Oct 28, 2020 at 12:40:49PM +0000, Will Deacon wrote:
> > On Wed, Oct 28, 2020 at 11:49:46AM +0000, Catalin Marinas wrote:
> > > On Wed, Oct 28, 2020 at 11:23:43AM +0000, Will Deacon wrote:
> > > > On Wed, Oct 28, 2020 at 11:22:06AM +0000, Catalin Marinas wrote:
> > > > > On Wed, Oct 28, 2020 at 11:17:13AM +0000, Will Deacon wrote:
> > > > > > On Wed, Oct 28, 2020 at 11:12:04AM +0000, Catalin Marinas wrote:
> > > Anyway, I see your reasoning behind the late CPUs but I don't
> > > particularly like abusing the cpufeature support to pretend a
> > > SYSTEM_FEATURE is available before knowing any CPU has it (maybe we do
> > > it in other cases, I haven't checked).
> >
> > Hmm, but that's exactly what this cmdline option is about. We pretend that
> > the system has 32-bit EL0 when normally we would say that we don't.
>
> So that's more about force-enabling 32-bit irrespective of whether any
> CPU supports it (not just in the mismatched/asymmetric case). Of course,
> if the aarch32_el0 mask is empty, the apps would get SIGKILL'ed.
Yes, but given that we can't generally rule out a 32-bit CPU coming online
late, I don't think we should pretend that we're in a position to detect
a 64-bit-only system.
> > > Could we not instead add a new feature for asymmetric support that's
> > > defined as ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE? This would be allowed
> > > for late CPUs and we keep the system_supports_32bit_el0() unchanged.
> >
> > I really don't think this gains us anything.
>
> It saves us having to explain to someone passing this option on a TX2
> why personality(PER_LINUX32) and even execve() appear to work (well,
> until SIGKILL). The lscpu tool, for example, uses personality() to
> display whether the CPUs support 32-bit.
It really doesn't save us having to explain what this option does: it will
need to be documented regardless. If you pass this option on TX2 then, yes,
you get access to the PER_LINUX32 personality because that's what this
option does. If you don't like that, don't pass the option! It's like
passing "nosmp" and being surprised that you only have one CPU online.
> Also with PER_LINUX32, /proc/cpuinfo shows the 32-bit HWCAPs. We have
> compat_elf_hwcap pre-populated with some stuff which is entirely untrue
> if AArch32 is missing.
>
> Thinking about the COMPAT_HWCAPs, do we actually populate them properly
> on an asymmetric system if the boot CPU is not AArch32-capable? In my
> original patch I had to defer populating boot_cpu_data with AArch32
> information until a capable CPU was found. If not,
> update_32bit_cpu_features() will set most 32-bit features to 0.
I think there are two interesting aspects to the COMPAT_HWCAPS:
1. Both my patches and the patches from Qais seem to get this wrong, so
the set of reported 32-bit caps is too small. I'll look at fixing this
for v2.
2. The 32-bit hwcaps are exposed by the PER_LINUX32 personality, and so
have to be fixed at boot *even if the boot CPUs are all 64-bit-only*.
This means that if the first 32-bit-capable core is onlined late, then
it will only get the base capabilities, but I think that's fine and
consistent with our overall handling of hwcaps (which cannot appear
dynamically to userspace).
> > The current users of system_supports_32bit_el0() are:
> >
> > - The ELF loader
> > - CPU feature sanitisation code
> > - Personality syscall
>
> There three need a relaxed system_supports_32bit_el0(), so we could
> change it to check a new relaxed feature.
>
> > - KVM
>
> Here I think we need the stronger guarantee, no 32-bit allowed in
> guests (the original symmetric feature check).
Right, and I handle this in these patches. But the point is really that
the majority of the users of system_supports_32bit_el0() are actually
interested in the asymmetric case and it doesn't make sense to call that out
separately. If it's the naming you object to, we could rename
system_supports_32bit_el0() to something else? Adding a new cap just adds
complexity that we don't need.
> > and, afaict, all of these would need to check the new feature if we added
> > it. I think it would also mean that at least one 32-bit capable CPU would
> > have to boot early in order for the new feature to be advertised, which
> > feels like an artificial restriction to me, particularly as you could just
> > offline it immediately.
>
> How strong requirement is to allow late CPUs here? I think we'd miss the
> COMPAT_HWCAPs as we no longer populate them once user-space started,
> they are actually setup via smp_cpus_done() -> setup_cpu_features().
I think the requirement is to be as consistent as possible and not introduce
more new behaviours than we really have to. Allowing late onlining of cores
but fixing the (compat) hwcaps during boot is fine.
Will
