On Mon, Oct 11, 2021 at 09:35:14PM -0700, Reiji Watanabe wrote:
> This patch lays the groundwork to make ID registers writable.
>
> Introduce struct id_reg_info for an ID register to manage the
> register specific control of its value for the guest.
> It is used to do register specific initialization, validation
> of the ID register and etc. Not all ID registers must have
> the id_reg_info. ID registers that don't have the id_reg_info
> are handled in a common way that is applied to all other
> ID registers.
>
> At present, changing an ID register from userspace is allowed only
> if the ID register has the id_reg_info, but that will be changed
> by the following patches.
>
> No ID register has the structure yet and the following patches
> will add the id_reg_info for some ID registers.
>
> Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> ---
> arch/arm64/kvm/sys_regs.c | 120 +++++++++++++++++++++++++++++++++++---
> 1 file changed, 111 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 72ca518e7944..8a0b88f9a975 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -263,6 +263,76 @@ static bool trap_raz_wi(struct kvm_vcpu *vcpu,
> return read_zero(vcpu, p);
> }
>
> +struct id_reg_info {
> + u32 sys_reg; /* Register ID */
> + u64 sys_val; /* Sanitized system value */
> +
> + /*
> + * Limit value of the register for a vcpu. The value is sys_val
> + * with bits cleared for unsupported features for the guest.
> + */
> + u64 vcpu_limit_val;
Maybe I'll see a need for both later, but at the moment I'd think we only
need sys_val with the bits cleared for disabled features.
> +
> + /* Bits that are not validated when userspace sets the register. */
> + u64 ignore_mask;
> +
> + /* Initialization function of the id_reg_info */
> + void (*init)(struct id_reg_info *id_reg);
> +
> + /* Register specific validation function */
> + int (*validate)(struct kvm_vcpu *vcpu, u64 val);
> +
> + /* Return the reset value of the register for the vCPU */
> + u64 (*get_reset_val)(struct kvm_vcpu *vcpu, struct id_reg_info *id_reg);
> +};
> +
> +static void id_reg_info_init(struct id_reg_info *id_reg)
> +{
> + id_reg->sys_val = read_sanitised_ftr_reg(id_reg->sys_reg);
> + id_reg->vcpu_limit_val = id_reg->sys_val;
> +}
> +
> +/*
> + * An ID register that needs special handling to control the value for the
> + * guest must have its own id_reg_info in id_reg_info_table.
> + * (i.e. the reset value is different from the host's sanitized value,
> + * the value is affected by opt-in features, some fields needs specific
> + * validation, etc.)
> + */
> +#define GET_ID_REG_INFO(id) (id_reg_info_table[IDREG_IDX(id)])
> +static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = {};
> +
> +static int validate_id_reg(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *rd, u64 val)
> +{
> + u32 id = reg_to_encoding(rd);
> + const struct id_reg_info *id_reg = GET_ID_REG_INFO(id);
> + u64 limit;
> + u64 check_val = val;
> + int err;
> +
> + if (id_reg) {
> + /*
> + * Clear bits with ignore_mask, which we don't want
> + * arm64_check_features to check.
> + */
> + check_val &= ~id_reg->ignore_mask;
> + limit = id_reg->vcpu_limit_val;
> + } else
> + limit = read_sanitised_ftr_reg(id);
> +
> + /* Check if the value indicates any feature that is not in the limit. */
> + err = arm64_check_features(id, check_val, limit);
> + if (err)
> + return err;
> +
> + if (id_reg && id_reg->validate)
> + /* Run the ID register specific validity check. */
> + err = id_reg->validate(vcpu, val);
> +
> + return err;
> +}
> +
> /*
> * ARMv8.1 mandates at least a trivial LORegion implementation, where all the
> * RW registers are RES0 (which we can implement as RAZ/WI). On an ARMv8.0
> @@ -1176,11 +1246,19 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
> {
> u32 id = reg_to_encoding(rd);
> + struct id_reg_info *id_reg;
> + u64 val;
>
> if (vcpu_has_reset_once(vcpu))
> return;
>
> - __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)) = read_sanitised_ftr_reg(id);
> + id_reg = GET_ID_REG_INFO(id);
> + if (id_reg && id_reg->get_reset_val)
> + val = id_reg->get_reset_val(vcpu, id_reg);
> + else
> + val = read_sanitised_ftr_reg(id);
> +
> + __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)) = val;
> }
>
> static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> @@ -1225,11 +1303,7 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> -/*
> - * cpufeature ID register user accessors
> - *
> - * We don't allow the effective value to be changed.
> - */
> +/* cpufeature ID register user accessors */
> static int __get_id_reg(const struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *rd, void __user *uaddr,
> bool raz)
> @@ -1240,11 +1314,12 @@ static int __get_id_reg(const struct kvm_vcpu *vcpu,
> return reg_to_user(uaddr, &val, id);
> }
>
> -static int __set_id_reg(const struct kvm_vcpu *vcpu,
> +static int __set_id_reg(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *rd, void __user *uaddr,
> bool raz)
> {
> const u64 id = sys_reg_to_index(rd);
> + u32 encoding = reg_to_encoding(rd);
> int err;
> u64 val;
>
> @@ -1252,10 +1327,18 @@ static int __set_id_reg(const struct kvm_vcpu *vcpu,
> if (err)
> return err;
>
> - /* This is what we mean by invariant: you can't change it. */
> - if (val != read_id_reg(vcpu, rd, raz))
> + /* Don't allow to change the reg unless the reg has id_reg_info */
> + if (val != read_id_reg(vcpu, rd, raz) && !GET_ID_REG_INFO(encoding))
> return -EINVAL;
>
> + if (raz)
> + return (val == 0) ? 0 : -EINVAL;
This is already covered by the val != read_id_reg(vcpu, rd, raz) check.
> +
> + err = validate_id_reg(vcpu, rd, val);
> + if (err)
> + return err;
> +
> + __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(encoding)) = val;
> return 0;
> }
>
> @@ -2818,6 +2901,23 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> return write_demux_regids(uindices);
> }
>
> +static void id_reg_info_init_all(void)
> +{
> + int i;
> + struct id_reg_info *id_reg;
> +
> + for (i = 0; i < ARRAY_SIZE(id_reg_info_table); i++) {
> + id_reg = (struct id_reg_info *)id_reg_info_table[i];
> + if (!id_reg)
> + continue;
> +
> + if (id_reg->init)
> + id_reg->init(id_reg);
> + else
> + id_reg_info_init(id_reg);
Maybe call id_reg->init(id_reg) from within id_reg_info_init() in case we
wanted to apply some common id register initialization at some point?
> + }
> +}
> +
> void kvm_sys_reg_table_init(void)
> {
> unsigned int i;
> @@ -2852,4 +2952,6 @@ void kvm_sys_reg_table_init(void)
> break;
> /* Clear all higher bits. */
> cache_levels &= (1 << (i*3))-1;
> +
> + id_reg_info_init_all();
> }
> --
> 2.33.0.882.g93a45727a2-goog
>
Thanks,
drew
