Hi Reiji,
On Tue, Nov 16, 2021 at 10:43:33PM -0800, 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, and provide set
> of functions commonly used for ID registers to make them writable.
>
> The id_reg_info 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 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/include/asm/sysreg.h | 1 +
> arch/arm64/kvm/sys_regs.c | 226 ++++++++++++++++++++++++++++++--
> 2 files changed, 218 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 16b3f1a1d468..597609f26331 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -1197,6 +1197,7 @@
> #define ICH_VTR_TDS_MASK (1 << ICH_VTR_TDS_SHIFT)
>
> #define ARM64_FEATURE_FIELD_BITS 4
> +#define ARM64_FEATURE_FIELD_MASK ((1ull << ARM64_FEATURE_FIELD_BITS) - 1)
>
> /* Create a mask for the feature bits of the specified feature. */
> #define ARM64_FEATURE_MASK(x) (GENMASK_ULL(x##_SHIFT + ARM64_FEATURE_FIELD_BITS - 1, x##_SHIFT))
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 5608d3410660..1552cd5581b7 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -265,6 +265,181 @@ static bool trap_raz_wi(struct kvm_vcpu *vcpu,
> return read_zero(vcpu, p);
> }
>
> +/*
> + * A value for FCT_LOWER_SAFE must be zero and changing that will affect
> + * ftr_check_types of id_reg_info.
> + */
> +enum feature_check_type {
> + FCT_LOWER_SAFE = 0,
> + FCT_HIGHER_SAFE,
> + FCT_HIGHER_OR_ZERO_SAFE,
> + FCT_EXACT,
> + FCT_EXACT_OR_ZERO_SAFE,
> + FCT_IGNORE, /* Don't check (any value is fine) */
> +};
> +
> +static int arm64_check_feature_one(enum feature_check_type type, int val,
> + int limit)
> +{
> + bool is_safe = false;
> +
> + if (val == limit)
> + return 0;
> +
> + switch (type) {
> + case FCT_LOWER_SAFE:
> + is_safe = (val <= limit);
> + break;
> + case FCT_HIGHER_OR_ZERO_SAFE:
> + if (val == 0) {
> + is_safe = true;
> + break;
> + }
> + fallthrough;
> + case FCT_HIGHER_SAFE:
> + is_safe = (val >= limit);
> + break;
> + case FCT_EXACT:
> + break;
> + case FCT_EXACT_OR_ZERO_SAFE:
> + is_safe = (val == 0);
> + break;
> + case FCT_IGNORE:
What happens if the a new feature is added and the field has a particular
meaning? How are you going to deal with old userspace implementations that
use a value here which now is not allowed or it affects the guest?
Thanks,
Alex
