Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 21 Jan 2023 12:02:03 +0000
Message-ID: <86k01gm6ys.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Oliver Upton <oliver.upton@linux.dev>,
        Akihiko Odaki <akihiko.odaki@daynix.com>
Cc:     Mark Brown <broonie@kernel.org>, linux-kernel@vger.kernel.org,
        kvmarm@lists.linux.dev, kvmarm@lists.cs.columbia.edu,
        linux-arm-kernel@lists.infradead.org,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        James Morse <james.morse@arm.com>,
        Will Deacon <will@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        asahi@lists.linux.dev, Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Sven Peter <sven@svenpeter.dev>,
        Hector Martin <marcan@marcan.st>
Subject: Re: [PATCH v7 7/7] KVM: arm64: Normalize cache configuration
In-Reply-To: <Y8meCFkrVXurXlTk@google.com>
References: <20230112023852.42012-1-akihiko.odaki@daynix.com>
        <20230112023852.42012-8-akihiko.odaki@daynix.com>
        <Y8meCFkrVXurXlTk@google.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Thu, 19 Jan 2023 19:46:16 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hi Akihiko,
> 
> On Thu, Jan 12, 2023 at 11:38:52AM +0900, Akihiko Odaki wrote:
> > Before this change, the cache configuration of the physical CPU was
> > exposed to vcpus. This is problematic because the cache configuration a
> > vcpu sees varies when it migrates between vcpus with different cache
> > configurations.
> > 
> > Fabricate cache configuration from the sanitized value, which holds the
> > CTR_EL0 value the userspace sees regardless of which physical CPU it
> > resides on.
> > 
> > CLIDR_EL1 and CCSIDR_EL1 are now writable from the userspace so that
> > the VMM can restore the values saved with the old kernel.
> > 
> > Suggested-by: Marc Zyngier <maz@kernel.org>
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> I needed to squash in the patch below to get all of this working.
> Writing back the value read for a given cache level was failing, which I
> caught with the get-reg-list selftest.
> 
> Pushed the result here if you want to have a look:
> 
>   https://github.com/oupton/linux/tree/kvm-arm64/virtual-cache-geometry
> 
> --
> Thanks,
> Oliver
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 459e6d358dab..b6228f7d1d8d 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -148,17 +148,19 @@ static u32 get_ccsidr(struct kvm_vcpu *vcpu, u32 csselr)
>  
>  static int set_ccsidr(struct kvm_vcpu *vcpu, u32 csselr, u32 val)
>  {
> -	u8 line_size = FIELD_GET(CCSIDR_EL1_LineSize, val);
> +	u8 line_size = SYS_FIELD_GET(CCSIDR_EL1, LineSize, val);
> +	u32 cur = get_ccsidr(vcpu, csselr);
> +	u8 min_line_size = SYS_FIELD_GET(CCSIDR_EL1, LineSize, cur);
>  	u32 *ccsidr = vcpu->arch.ccsidr;
>  	u32 i;
>  
> -	if ((val & CCSIDR_EL1_RES0) || line_size < get_min_cache_line_size(csselr))
> +	if (cur == val)
> +		return 0;
> +
> +	if ((val & CCSIDR_EL1_RES0) || line_size < min_line_size)
>  		return -EINVAL;

This doesn't look right. You're comparing the value userspace is
trying to set for a given level with the value that is already set for
that level, and forbid the cache line size to be smaller. It works if
no value has been set yet (you fallback to something derived from
CTR_EL0), but this fails if userspace does multiple writes.

The original check is against CTR_EL0, which makes absolute sense
because we want to check across the whole hierarchy. It is just that
the original code has two bugs:

- It fails to convert the CCSIDR_EL1.LineSize value to a number of
  words (the missing +4). Admire how the architecture is actively
  designed to be hostile to SW by providing two different formats for
  the cache line size, none of which is in... bytes.

- It passes the full CSSELR value to get_min_cache_line_size(), while
  this function wants a bool... Yes, there are times where you'd want
  a stronger type system (did anyone say Rust? ;-)

I propose that we fold something like the patch below in instead
(tested with get-reg-list).

Thanks,

	M.

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 3b3024c42e61..ac943dcb4610 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -148,11 +148,12 @@ static u32 get_ccsidr(struct kvm_vcpu *vcpu, u32 csselr)
 
 static int set_ccsidr(struct kvm_vcpu *vcpu, u32 csselr, u32 val)
 {
-	u8 line_size = FIELD_GET(CCSIDR_EL1_LineSize, val);
+	u8 line_size = FIELD_GET(CCSIDR_EL1_LineSize, val) + 4;
 	u32 *ccsidr = vcpu->arch.ccsidr;
 	u32 i;
 
-	if ((val & CCSIDR_EL1_RES0) || line_size < get_min_cache_line_size(csselr))
+	if ((val & CCSIDR_EL1_RES0) ||
+	    line_size < get_min_cache_line_size(csselr & CSSELR_EL1_InD))
 		return -EINVAL;
 
 	if (!ccsidr) {

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