Re: [PATCH v2 11/11] KVM: arm64: Get rid of the AArch32 register mapping code - Nina Schoetterl-Glausch

From: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
To: Marc Zyngier <maz@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org
Cc: James Morse <james.morse@arm.com>,
	Julien Thierry <julien.thierry.kdev@gmail.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Andrew Scull <ascull@google.com>, Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Quentin Perret <qperret@google.com>,
	David Brazdil <dbrazdil@google.com>,
	kernel-team@android.com
Subject: Re: [PATCH v2 11/11] KVM: arm64: Get rid of the AArch32 register mapping code
Date: Thu, 23 May 2024 16:25:21 +0200	[thread overview]
Message-ID: <66a7077c5df86d0a541237996382ae583d690a14.camel@linux.ibm.com> (raw)
In-Reply-To: <20201102164045.264512-12-maz@kernel.org>

On Mon, 2020-11-02 at 16:40 +0000, Marc Zyngier wrote:

[...]

> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index dfb5218137ca..3f23f7478d2a 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -252,10 +252,32 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  	memcpy(addr, valp, KVM_REG_SIZE(reg->id));

I was looking at KVM_(G|S)ET_ONE_REG implementations and something looks off to me here:

...

	if (off == KVM_REG_ARM_CORE_REG(regs.pstate)) {
		u64 mode = (*(u64 *)valp) & PSR_AA32_MODE_MASK;
		switch (mode) {

Masking and switch over mode here...

		case PSR_AA32_MODE_USR:
			if (!kvm_supports_32bit_el0())
				return -EINVAL;
			break;
		case PSR_AA32_MODE_FIQ:
		case PSR_AA32_MODE_IRQ:
...
>  
>  	if (*vcpu_cpsr(vcpu) & PSR_MODE32_BIT) {
> -		int i;
> +		int i, nr_reg;
> +
> +		switch (*vcpu_cpsr(vcpu)) {

...but switching over mode without masking here.
I don't know if this is as intended, but I thought I'd mention it.

> +		/*
> +		 * Either we are dealing with user mode, and only the
> +		 * first 15 registers (+ PC) must be narrowed to 32bit.
> +		 * AArch32 r0-r14 conveniently map to AArch64 x0-x14.
> +		 */
> +		case PSR_AA32_MODE_USR:
> +		case PSR_AA32_MODE_SYS:
> +			nr_reg = 15;
> +			break;
> +
> +		/*
> +		 * Otherwide, this is a priviledged mode, and *all* the
> +		 * registers must be narrowed to 32bit.
> +		 */
> +		default:
> +			nr_reg = 31;
> +			break;
> +		}
> +
> +		for (i = 0; i < nr_reg; i++)
> +			vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i));
>  
> -		for (i = 0; i < 16; i++)
> -			*vcpu_reg32(vcpu, i) = (u32)*vcpu_reg32(vcpu, i);
> +		*vcpu_pc(vcpu) = (u32)*vcpu_pc(vcpu);
>  	}
>  out:
>  	return err;
> diff --git a/arch/arm64/kvm/regmap.c b/arch/arm64/kvm/regmap.c
> deleted file mode 100644
> index ae7e290bb017..000000000000
> --- a/arch/arm64/kvm/regmap.c
> +++ /dev/null
> @@ -1,128 +0,0 @@

[...]

> -unsigned long *vcpu_reg32(const struct kvm_vcpu *vcpu, u8 reg_num)
> -{
> -	unsigned long *reg_array = (unsigned long *)&vcpu->arch.ctxt.regs;
> -	unsigned long mode = *vcpu_cpsr(vcpu) & PSR_AA32_MODE_MASK;

There used to be masking here at least.
> -
> -	switch (mode) {
> -	case PSR_AA32_MODE_USR ... PSR_AA32_MODE_SVC:
> -		mode &= ~PSR_MODE32_BIT; /* 0 ... 3 */
> -		break;
> -
> -	case PSR_AA32_MODE_ABT:
> -		mode = 4;
> -		break;
> -
> -	case PSR_AA32_MODE_UND:
> -		mode = 5;
> -		break;
> -
> -	case PSR_AA32_MODE_SYS:
> -		mode = 0;	/* SYS maps to USR */
> -		break;
> -
> -	default:
> -		BUG();
> -	}
> -
> -	return reg_array + vcpu_reg_offsets[mode][reg_num];
> -}

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