Re: [PATCH v4 04/12] KVM: arm64: Add ARM64_HAS_LPA2 CPU capability

From: Marc Zyngier <maz@kernel.org>
To: Ryan Roberts <ryan.roberts@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	James Morse <james.morse@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	Anshuman Khandual <anshuman.khandual@arm.com>,
	linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev
Subject: Re: [PATCH v4 04/12] KVM: arm64: Add ARM64_HAS_LPA2 CPU capability
Date: Mon, 23 Oct 2023 10:34:58 +0100	[thread overview]
Message-ID: <867cnd65rx.wl-maz@kernel.org> (raw)
In-Reply-To: <e2dd92a1-a8bb-4421-a814-2d663acc7f11@arm.com>

On Fri, 20 Oct 2023 16:03:37 +0100,
Ryan Roberts <ryan.roberts@arm.com> wrote:
> 
> On 20/10/2023 09:16, Marc Zyngier wrote:
> > On Mon, 09 Oct 2023 19:50:00 +0100,
> > Ryan Roberts <ryan.roberts@arm.com> wrote:
> >>
> >> +static bool has_lpa2(const struct arm64_cpu_capabilities *entry, int scope)
> >> +{
> >> +	u64 mmfr0;
> >> +	bool ret;
> >> +
> >> +	mmfr0 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1);
> >> +	ret = has_lpa2_at_stage1(mmfr0);
> >> +
> >> +	if (kvm_get_mode() != KVM_MODE_NONE)
> >> +		ret = ret && has_lpa2_at_stage2(mmfr0);
> > 
> > Isn't it too late to go back on the decision to use LPA2 at S1 if you
> > realise that S2 doesn't support it?
> 
> The KVM mode dependent part was a change that Oliver asked for. I guess you are
> talking about kernel S1? I don't think it's too late here to decide whether the
> (nvhe) hyp s1 should use LPA2. But I guess your point is that kernel s1 would
> have had to decide much earlier in boot and will have had to take LPA2 support
> in both S1 and S2 into account, and would not have the KVM mode info available
> to it at that point?

That's roughly my point. When we reach this point on a VHE system,
we're pretty far along and I'm not sure we can turn back. In all
honesty, if a system doesn't support LPA2 at S2, it is in a pretty bad
shape and we shouldn't bother supporting it. Or at least not with KVM.

Just because the architecture allows braindead configurations doesn't
mean we have to go out of our way to support them. In this case, I'd
be absolutely fine with disabling KVM altogether.

> > Why isn't this patch the first or second in the series? You could use
> > it to drive the LPA2 decision in the patch #2, avoiding the ugly lpa2
> > flag...
> 
> I still only think this works if we put my patch and Ard's patch in atomically?
> Or at least force has_lpa2() to always return false until both are in, then flip
> the switch atomically.

Whichever works for you. My only ask is to try to minimise the churn
here.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.