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

[Marc Zyngier <maz@kernel.org>]

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.