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@xxxxxxxxxx>
> ---
> 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
