Re: [PATCH v5 5/6] KVM: arm64: Introduce ID register specific descriptor

From: Marc Zyngier <maz@kernel.org>
To: Jing Zhang <jingzhangos@google.com>
Cc: KVM <kvm@vger.kernel.org>, KVMARM <kvmarm@lists.linux.dev>,
	ARMLinux <linux-arm-kernel@lists.infradead.org>,
	Oliver Upton <oupton@google.com>, Will Deacon <will@kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Fuad Tabba <tabba@google.com>, Reiji Watanabe <reijiw@google.com>,
	Raghavendra Rao Ananta <rananta@google.com>
Subject: Re: [PATCH v5 5/6] KVM: arm64: Introduce ID register specific descriptor
Date: Mon, 03 Apr 2023 13:27:24 +0100	[thread overview]
Message-ID: <86y1n9up5f.wl-maz@kernel.org> (raw)
In-Reply-To: <20230402183735.3011540-6-jingzhangos@google.com>

On Sun, 02 Apr 2023 19:37:34 +0100,
Jing Zhang <jingzhangos@google.com> wrote:
> 
> Introduce an ID feature register specific descriptor to include ID
> register specific fields and callbacks besides its corresponding
> general system register descriptor.
> 
> No functional change intended.
> 
> Co-developed-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  arch/arm64/kvm/id_regs.c  | 233 ++++++++++++++++++++++++++++----------
>  arch/arm64/kvm/sys_regs.c |   2 +-
>  arch/arm64/kvm/sys_regs.h |   1 +
>  3 files changed, 178 insertions(+), 58 deletions(-)
> 
> diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> index e92eacb0ad32..af86001e2686 100644
> --- a/arch/arm64/kvm/id_regs.c
> +++ b/arch/arm64/kvm/id_regs.c
> @@ -18,6 +18,27 @@
>  
>  #include "sys_regs.h"
>  
> +struct id_reg_desc {
> +	const struct sys_reg_desc	reg_desc;
> +	/*
> +	 * ftr_bits points to the feature bits array defined in cpufeature.c for
> +	 * writable CPU ID feature register.
> +	 */
> +	const struct arm64_ftr_bits *ftr_bits;

Why do we need to keep this around? we already have all the required
infrastructure to lookup a full arm64_ftr_reg. So why the added stuff?

> +	/*
> +	 * Only bits with 1 are writable from userspace.
> +	 * This mask might not be necessary in the future whenever all ID
> +	 * registers are enabled as writable from userspace.
> +	 */
> +	const u64 writable_mask;
> +	/*
> +	 * This function returns the KVM sanitised register value.
> +	 * The value would be the same as the host kernel sanitised value if
> +	 * there is no KVM sanitisation for this id register.
> +	 */
> +	u64 (*read_kvm_sanitised_reg)(const struct id_reg_desc *idr);

Why can't this function return both the required value and a mask?
Why can't it live in the sys_reg_desc structure?

Frankly, I don't see a good reason to have this wrapper structure
which makes things pointlessly complicated and prevent code sharing
with the rest of the infrastructure.

	M.

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

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