Re: [PATCH v2 2/2] arm64: Don't use KPTI where we have E0PD

From: Mark Brown <broonie@kernel•org>
To: Catalin Marinas <catalin.marinas@arm•com>
Cc: Will Deacon <will@kernel•org>,
	linux-arm-kernel@lists•infradead.org,
	Suzuki K Poulose <suzuki.poulose@arm•com>
Subject: Re: [PATCH v2 2/2] arm64: Don't use KPTI where we have E0PD
Date: Fri, 16 Aug 2019 13:10:05 +0100	[thread overview]
Message-ID: <20190816121005.GB4039@sirena.co.uk> (raw)
In-Reply-To: <20190816102424.GA28874@arrakis.emea.arm.com>

[-- Attachment #1.1: Type: text/plain, Size: 3404 bytes --]

On Fri, Aug 16, 2019 at 11:24:24AM +0100, Catalin Marinas wrote:
> On Thu, Aug 15, 2019 at 05:35:42PM +0100, Will Deacon wrote:

> > > +	if (IS_ENABLED(CONFIG_ARM64_E0PD)) {
> > > +		ftr = read_sysreg_s(SYS_ID_AA64MMFR2_EL1);
> > > +		if ((ftr >> ID_AA64MMFR2_E0PD_SHIFT) & 0xf)
> > > +			return false;
> > > +	}

> What I don't particularly like here is that on big.LITTLE this hunk may
> have a different behaviour depending on which CPU you run it on. In
> general, such CPUID access should only be done in a non-preemptible
> context.

> We probably get away with this during early boot (before CPU caps have
> been set up) when arm64_kernel_unmapped_at_el0() is false since we only
> have a single CPU running. Later on at run-time, we either have
> arm64_kernel_unmapped_at_el0() true, meaning that some CPU is missing
> E0PD with kaslr_offset() > 0, or the kernel is mapped at EL0 with all
> CPUs having E0PD. But I find it hard to reason about.

Yes, all this stuff is unfortunately hard to reason about since there's
several environment changes during boot which have a material effect and
also multiple different things that might trigger KPTI.  IIRC my thinking
here was that if we turned on KPTI we're turning it on for all CPUs so 
by the time we could be prempted we'd be returning true from the earlier
check for arm64_kernel_unmapped_at_el0() but it's possible I missed some
case there.  I was trying to avoid disturbing the existing code too much
unless I had a strong reason to on the basis that I might be missing
something about the way it was done.

> Could we move the above hunk in this block:

> 	} else if (!static_branch_likely(&arm64_const_caps_ready)) {
> 		...
> 	}

> and reshuffle the rest of the function to only rely on
> arm64_kernel_unmapped_at_el0() when the caps are ready (at run-time)?

I've added the check, will look at the reshuffle.

> > Thinking about this further, I think we can simply move all of the
> > 'kaslr_offset() > 0' checks used by the kpti code (i.e. in
> > arm64_kernel_unmapped_at_el0(), kpti_install_ng_mappings() and
> > unmap_kernel_at_el0()) into a helper function which does the check for
> > E0PD as well. Perhaps 'kaslr_requires_kpti()' ?

> I agree, this needs some refactoring as we have this decision in three
> separate places.

> Trying to put my thoughts together. At run-time, with capabilities fully
> enabled, we want:

>   arm64_kernel_use_ng_mappings() == arm64_kernel_unmapped_at_el0()

>   KPTI is equivalent to arm64_kernel_unmapped_at_el0()

Yes, this bit is simple - once we're up and running everything is clear.

> I think kaslr_requires_kpti() should access the raw CPUID registers (for
> E0PD, TX1 bug) and be called only by unmap_kernel_at_el0() and
> arm64_kernel_use_ng_mappings(), the latter if !arm64_const_caps_ready.
> The boot CPU should store kaslr_requires_kpti() value somewhere and
> kpti_install_ng_mappings() should check this variable before deciding to
> skip the page table rewrite.

We definitely need some variable I think, and I think you're right that
making the decision on the boot CPU would simplify things a lot.  The
systems with very large memories that are most affected by the cost of
moving from global to non-global mappings are most likely symmetric
anyway so only looking at the boot CPU should be fine for that.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel