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

[Marc Zyngier <maz@kernel.org>]

On Fri, 26 May 2023 22:37:04 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> 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...

I don't think this works just as suggested. It completely breaks all
the existing macros that use implicit (in order) initialisers. Not
hard to fix, but pretty invasive (I count 656 warnings in my current
tree).

I agree it is much cleaner though.

	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