On Mon, Oct 11, 2021 at 09:35:12PM -0700, Reiji Watanabe wrote:
> Extend sys_regs[] of kvm_cpu_context for ID registers and save ID
> registers' sanitized value in the array for the vCPU at the first
> vCPU reset. Use the saved ones when ID registers are read by
> userspace (via KVM_GET_ONE_REG) or the guest.
>
> Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> ---
> arch/arm64/include/asm/kvm_host.h | 10 ++++++++++
> arch/arm64/kvm/sys_regs.c | 26 ++++++++++++++++++--------
> 2 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 9b5e7a3b6011..0cd351099adf 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -145,6 +145,14 @@ struct kvm_vcpu_fault_info {
> u64 disr_el1; /* Deferred [SError] Status Register */
> };
>
> +/*
> + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
> + * where 0<=crm<8, 0<=op2<8.
crm is 4 bits, so this should be 0 <= crm < 16 and...
> + */
> +#define KVM_ARM_ID_REG_MAX_NUM 64
...this should be 128. Or am I missing something?
> +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id))
> +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id))
> +
> enum vcpu_sysreg {
> __INVALID_SYSREG__, /* 0 is reserved as an invalid value */
> MPIDR_EL1, /* MultiProcessor Affinity Register */
> @@ -209,6 +217,8 @@ enum vcpu_sysreg {
> CNTP_CVAL_EL0,
> CNTP_CTL_EL0,
>
> + ID_REG_BASE,
> + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1,
> /* Memory Tagging Extension registers */
> RGSR_EL1, /* Random Allocation Tag Seed Register */
> GCR_EL1, /* Tag Control Register */
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 1d46e185f31e..72ca518e7944 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -273,7 +273,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
> struct sys_reg_params *p,
> const struct sys_reg_desc *r)
> {
> - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
> + u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(SYS_ID_AA64MMFR1_EL1));
> u32 sr = reg_to_encoding(r);
>
> if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) {
> @@ -1059,12 +1059,11 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
> return true;
> }
>
> -/* Read a sanitised cpufeature ID register by sys_reg_desc */
> static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> struct sys_reg_desc const *r, bool raz)
> {
> u32 id = reg_to_encoding(r);
> - u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
> + u64 val = raz ? 0 : __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id));
>
> switch (id) {
> case SYS_ID_AA64PFR0_EL1:
> @@ -1174,6 +1173,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> return REG_HIDDEN;
> }
>
> +static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
Since not all ID registers will use this, then maybe name it
reset_sanitised_id_reg?
> +{
> + u32 id = reg_to_encoding(rd);
> +
> + if (vcpu_has_reset_once(vcpu))
> + return;
Ah, I see my kvm_vcpu_initialized() won't work since vcpu->arch.target is
set before the first reset. While vcpu->arch.target is only being used
like a "is_initialized" boolean at this time, I guess we better keep it
in case we ever want to implement CPU models (which this series gets us a
step closer to).
> +
> + __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)) = read_sanitised_ftr_reg(id);
> +}
> +
> static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *rd,
> const struct kvm_one_reg *reg, void __user *uaddr)
> @@ -1219,9 +1228,7 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> /*
> * cpufeature ID register user accessors
> *
> - * For now, these registers are immutable for userspace, so no values
> - * are stored, and for set_id_reg() we don't allow the effective value
> - * to be changed.
> + * We don't allow the effective value to be changed.
> */
> static int __get_id_reg(const struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *rd, void __user *uaddr,
> @@ -1375,6 +1382,7 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
> #define ID_SANITISED(name) { \
> SYS_DESC(SYS_##name), \
> .access = access_id_reg, \
> + .reset = reset_id_reg, \
> .get_user = get_id_reg, \
> .set_user = set_id_reg, \
> .visibility = id_visibility, \
> @@ -1830,8 +1838,10 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu,
> if (p->is_write) {
> return ignore_write(vcpu, p);
> } else {
> - u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> - u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
> + u64 dfr = __vcpu_sys_reg(vcpu,
> + IDREG_SYS_IDX(SYS_ID_AA64DFR0_EL1));
> + u64 pfr = __vcpu_sys_reg(vcpu,
> + IDREG_SYS_IDX(SYS_ID_AA64PFR0_EL1));
Please avoid these ugly line breaks when we're well under Linux's max
length, which is 100.
> u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL3_SHIFT);
>
> p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) |
> --
> 2.33.0.882.g93a45727a2-goog
>
Thanks,
drew
