Message-ID: <20210625093648.GB2782@willie-the-truck>
Date:   Fri, 25 Jun 2021 10:36:49 +0100
From:   Will Deacon <will@...nel.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-arch@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Catalin Marinas <catalin.marinas@....com>,
        Marc Zyngier <maz@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Morten Rasmussen <morten.rasmussen@....com>,
        Qais Yousef <qais.yousef@....com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Quentin Perret <qperret@...gle.com>, Tejun Heo <tj@...nel.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Valentin Schneider <valentin.schneider@....com>,
        Mark Rutland <mark.rutland@....com>, kernel-team@...roid.com
Subject: Re: [PATCH v10 13/16] arm64: Advertise CPUs capable of running
 32-bit applications in sysfs

Hi Greg,

On Fri, Jun 25, 2021 at 10:46:35AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 23, 2021 at 06:38:45PM +0100, Will Deacon wrote:
> > Since 32-bit applications will be killed if they are caught trying to
> > execute on a 64-bit-only CPU in a mismatched system, advertise the set
> > of 32-bit capable CPUs to userspace in sysfs.
> > 
> > Reviewed-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Reviewed-by: Catalin Marinas <catalin.marinas@....com>
> > Signed-off-by: Will Deacon <will@...nel.org>
> > ---
> >  .../ABI/testing/sysfs-devices-system-cpu      |  9 +++++++++
> >  arch/arm64/kernel/cpufeature.c                | 19 +++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
> > index fe13baa53c59..899377b2715a 100644
> > --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
> > +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
> > @@ -494,6 +494,15 @@ Description:	AArch64 CPU registers
> >  		'identification' directory exposes the CPU ID registers for
> >  		identifying model and revision of the CPU.
> >  
> > +What:		/sys/devices/system/cpu/aarch32_el0
> > +Date:		May 2021
> > +Contact:	Linux ARM Kernel Mailing list <linux-arm-kernel@...ts.infradead.org>
> > +Description:	Identifies the subset of CPUs in the system that can execute
> > +		AArch32 (32-bit ARM) applications. If present, the same format as
> > +		/sys/devices/system/cpu/{offline,online,possible,present} is used.
> > +		If absent, then all or none of the CPUs can execute AArch32
> > +		applications and execve() will behave accordingly.
> > +
> >  What:		/sys/devices/system/cpu/cpu#/cpu_capacity
> >  Date:		December 2016
> >  Contact:	Linux kernel mailing list <linux-kernel@...r.kernel.org>
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index 2f9fe57ead97..23eaa7f06f76 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -67,6 +67,7 @@
> >  #include <linux/crash_dump.h>
> >  #include <linux/sort.h>
> >  #include <linux/stop_machine.h>
> > +#include <linux/sysfs.h>
> >  #include <linux/types.h>
> >  #include <linux/minmax.h>
> >  #include <linux/mm.h>
> > @@ -1319,6 +1320,24 @@ const struct cpumask *system_32bit_el0_cpumask(void)
> >  	return cpu_possible_mask;
> >  }
> >  
> > +static ssize_t aarch32_el0_show(struct device *dev,
> > +				struct device_attribute *attr, char *buf)
> > +{
> > +	const struct cpumask *mask = system_32bit_el0_cpumask();
> > +
> > +	return sysfs_emit(buf, "%*pbl\n", cpumask_pr_args(mask));
> > +}
> > +static const DEVICE_ATTR_RO(aarch32_el0);
> 
> I just realized that we have a problem with this type of representation
> overflowing PAGE_SIZE on larger systems.  There is ongoing work to fix
> this up but that requires converting these to binary sysfs files, which
> is a pain to preserve the original format here.

Urgh. Do you have a link to the work trying to fix this for the other sysfs
files which are affected by this problem, please?

> Yes, for now you will be fine on these arm32 systems, but in the future
> this will have to be changed.  Because of that, should you just make
> this an individual cpu attribute (one file per cpu) and not a single
> file that lists all cpus?

Practically, I don't see this will ever be an issue for this case:

  1. 32-bit support is going to go away from the hardware, so this file
     will simply be empty in the future.

  2. The 32-bit-capable CPUs aren't dotted around randomly, but rather
     exist as contiguous ranges, so the format is reasonably compact.

> what tool is going to read this and why can't they just pick it up from
> the individual cpu files instead?

The idea is that controlling the affinity of 32-bit applications explicitly
in userspace can be done by either creating cpusets where all CPUs are
32-bit capable, or simply setting their affinity to include only
32-bit-capable CPUs. The really nice properties about the way this is
currently exposed are that it is the same as the
/sys/devices/system/cpu/{offline,online,possible,present} files and is
readibly usable by scripts. Moving the information into a per-cpu file gets
rid of both of those advantages :(

A middle ground would be to expose it as a mask (i.e. "%*pb") and use one
bit per CPU. NR_CPUS is capped to 4k on arm64 so that would be sufficient,
although then the format is weirdly different to the other masks in the same
directory.

Will

Powered by blists - more mailing lists