Re: [PATCH v10 4/5] KVM: arm64: Reuse fields of sys_reg_desc for idreg

[Oliver Upton <oliver.upton@linux.dev>]

Hi Jing,

On Mon, May 22, 2023 at 10:18:34PM +0000, Jing Zhang wrote:
> Since reset() and val are not used for idreg in sys_reg_desc, they would
> be used with other purposes for idregs.
> The callback reset() would be used to return KVM sanitised id register
> values. The u64 val would be used as mask for writable fields in idregs.
> Only bits with 1 in val are writable from userspace.

The tense of the changelog is wrong (should be in an imperative mood).
Maybe something like:

  sys_reg_desc::{reset, val} are presently unused for ID register
  descriptors. Repurpose these fields to support user-configurable ID
  registers.

  Use the ::reset() function pointer to return the sanitised value of a
  given ID register, optionally with KVM-specific feature sanitisation.
  Additionally, keep a mask of writable register fields in ::val.

> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 101 +++++++++++++++++++++++++++-----------
>  arch/arm64/kvm/sys_regs.h |  15 ++++--
>  2 files changed, 82 insertions(+), 34 deletions(-)
> 

[...]

> +/*
> + * Since reset() callback and field val are not used for idregs, they will be
> + * used for specific purposes for idregs.
> + * The reset() would return KVM sanitised register value. The value would be the
> + * same as the host kernel sanitised value if there is no KVM sanitisation.
> + * The val would be used as a mask indicating writable fields for the idreg.
> + * 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.
> + */
> +
>  /* sys_reg_desc initialiser for known cpufeature ID registers */
>  #define ID_SANITISED(name) {			\
>  	SYS_DESC(SYS_##name),			\
> @@ -1751,6 +1788,8 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
>  	.get_user = get_id_reg,			\
>  	.set_user = set_id_reg,			\
>  	.visibility = id_visibility,		\
> +	.reset = general_read_kvm_sanitised_reg,\
> +	.val = 0,				\

I generally think unions are more trouble than they're worth, but it
might make sense to throw the fields with dual meaning into one, like

  struct sys_reg_desc {

  	[...]
	union {
		struct {
			void (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);
			u64 val;
		};
		struct {
			u64 (*read_sanitised)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *);
			u64 mask;
		};
	};
  }

You could then avoid repainting the world to handle ->reset() returning
a value and usage of the fields in an id register context become a bit
more self-documenting. And you get to play with fire while you do it!

Let's see if the other side of the pond agrees with my bikeshedding...

-- 
Thanks,
Oliver