On 17/10/17 14:51, Christoffer Dall wrote:
> On Tue, Oct 10, 2017 at 07:38:19PM +0100, Dave Martin wrote:
>> Currently, a guest kernel sees the true CPU feature registers
>> (ID_*_EL1) when it reads them using MRS instructions. This means
>> that the guest will observe features that are present in the
>> hardware but the host doesn't understand or doesn't provide support
>> for. A guest may legimitately try to use such a feature as per the
>> architecture, but use of the feature may trap instead of working
>> normally, triggering undef injection into the guest.
>>
>> This is not a problem for the host, but the guest may go wrong when
>> running on newer hardware than the host knows about.
>>
>> This patch hides from guest VMs any AArch64-specific CPU features
>> that the host doesn't support, by exposing to the guest the
>> sanitised versions of the registers computed by the cpufeatures
>> framework, instead of the true hardware registers. To achieve
>> this, HCR_EL2.TID3 is now set for AArch64 guests, and emulation
>> code is added to KVM to report the sanitised versions of the
>> affected registers in response to MRS and register reads from
>> userspace.
>>
>> The affected registers are removed from invariant_sys_regs[] (since
>> the invariant_sys_regs handling is no longer quite correct for
>> them) and added to sys_reg_desgs[], with appropriate access(),
>> get_user() and set_user() methods. No runtime vcpu storage is
>> allocated for the registers: instead, they are read on demand from
>> the cpufeatures framework. This may need modification in the
>> future if there is a need for userspace to customise the features
>> visible to the guest.
>>
>> Attempts by userspace to write the registers are handled similarly
>> to the current invariant_sys_regs handling: writes are permitted,
>> but only if they don't attempt to change the value. This is
>> sufficient to support VM snapshot/restore from userspace.
>>
>> Because of the additional registers, restoring a VM on an older
>> kernel may not work unless userspace knows how to handle the extra
>> VM registers exposed to the KVM user ABI by this patch.
>>
>> Under the principle of least damage, this patch makes no attempt to
>> handle any of the other registers currently in
>> invariant_sys_regs[], or to emulate registers for AArch32: however,
>> these could be handled in a similar way in future, as necessary.
>>
>> Signed-off-by: Dave Martin <Dave.Martin@xxxxxxx>
>> Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> arch/arm64/include/asm/sysreg.h | 3 +
>> arch/arm64/kvm/hyp/switch.c | 6 +
>> arch/arm64/kvm/sys_regs.c | 282 +++++++++++++++++++++++++++++++++-------
>> 3 files changed, 246 insertions(+), 45 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
>> index f707fed..480ecd6 100644
>> --- a/arch/arm64/include/asm/sysreg.h
>> +++ b/arch/arm64/include/asm/sysreg.h
>> @@ -149,6 +149,9 @@
>> #define SYS_ID_AA64DFR0_EL1 sys_reg(3, 0, 0, 5, 0)
>> #define SYS_ID_AA64DFR1_EL1 sys_reg(3, 0, 0, 5, 1)
>>
>> +#define SYS_ID_AA64AFR0_EL1 sys_reg(3, 0, 0, 5, 4)
>> +#define SYS_ID_AA64AFR1_EL1 sys_reg(3, 0, 0, 5, 5)
>> +
>> #define SYS_ID_AA64ISAR0_EL1 sys_reg(3, 0, 0, 6, 0)
>> #define SYS_ID_AA64ISAR1_EL1 sys_reg(3, 0, 0, 6, 1)
>>
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index 945e79c..35a90b8 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -81,11 +81,17 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
>> * it will cause an exception.
>> */
>> val = vcpu->arch.hcr_el2;
>> +
>> if (!(val & HCR_RW) && system_supports_fpsimd()) {
>> write_sysreg(1 << 30, fpexc32_el2);
>> isb();
>> }
>> +
>> + if (val & HCR_RW) /* for AArch64 only: */
>> + val |= HCR_TID3; /* TID3: trap feature register accesses */
>> +
>
> Since we're setting this for all 64-bit VMs, can we not set this in
> vcpu_reset_hcr instead?
>
>> write_sysreg(val, hcr_el2);
>> +
>> /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */
>> write_sysreg(1 << 15, hstr_el2);
>> /*
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 2e070d3..b1f7552 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -892,6 +892,137 @@ static bool access_cntp_cval(struct kvm_vcpu *vcpu,
>> return true;
>> }
>>
>> +/* Read a sanitised cpufeature ID register by sys_reg_desc */
>> +static u64 read_id_reg(struct sys_reg_desc const *r, bool raz)
>> +{
>> + u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
>> + (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
>> +
>> + return raz ? 0 : read_sanitised_ftr_reg(id);
>> +}
>> +
>> +/* cpufeature ID register access trap handlers */
>> +
>> +static bool __access_id_reg(struct kvm_vcpu *vcpu,
>> + struct sys_reg_params *p,
>> + const struct sys_reg_desc *r,
>> + bool raz)
>> +{
>> + if (p->is_write)
>> + return write_to_read_only(vcpu, p, r);
>> +
>> + p->regval = read_id_reg(r, raz);
>> + return true;
>> +}
>> +
>> +static bool access_id_reg(struct kvm_vcpu *vcpu,
>> + struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + return __access_id_reg(vcpu, p, r, false);
>> +}
>> +
>> +static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
>> + struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + return __access_id_reg(vcpu, p, r, true);
>> +}
>> +
>> +static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
>> +static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
>> +static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
>> +
>> +/*
>> + * 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.
>> + */
>> +static int __get_id_reg(const struct sys_reg_desc *rd, void __user *uaddr,
>> + bool raz)
>> +{
>> + const u64 id = sys_reg_to_index(rd);
>> + const u64 val = read_id_reg(rd, raz);
>> +
>> + return reg_to_user(uaddr, &val, id);
>> +}
>> +
>> +static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr,
>> + bool raz)
>> +{
>> + const u64 id = sys_reg_to_index(rd);
>> + int err;
>> + u64 val;
>> +
>> + err = reg_from_user(&val, uaddr, id);
>> + if (err)
>> + return err;
>> +
>> + /* This is what we mean by invariant: you can't change it. */
>> + if (val != read_id_reg(rd, raz))
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> + const struct kvm_one_reg *reg, void __user *uaddr)
>> +{
>> + return __get_id_reg(rd, uaddr, false);
>> +}
>> +
>> +static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> + const struct kvm_one_reg *reg, void __user *uaddr)
>> +{
>> + return __set_id_reg(rd, uaddr, false);
>> +}
>> +
>> +static int get_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> + const struct kvm_one_reg *reg, void __user *uaddr)
>> +{
>> + return __get_id_reg(rd, uaddr, true);
>> +}
>> +
>> +static int set_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> + const struct kvm_one_reg *reg, void __user *uaddr)
>> +{
>> + return __set_id_reg(rd, uaddr, true);
>> +}
>> +
>> +/* sys_reg_desc initialiser for known cpufeature ID registers */
>> +#define ID_SANITISED(name) { \
>> + SYS_DESC(SYS_##name), \
>> + .access = access_id_reg, \
>> + .get_user = get_id_reg, \
>> + .set_user = set_id_reg, \
>> +}
>> +
>> +/*
>> + * sys_reg_desc initialiser for architecturally unallocated cpufeature ID
>> + * register with encoding Op0=3, Op1=0, CRn=0, CRm=crm, Op2=op2
>> + * (1 <= crm < 8, 0 <= Op2 < 8).
>> + */
>> +#define ID_UNALLOCATED(crm, op2) { \
>> + Op0(3), Op1(0), CRn(0), CRm(crm), Op2(op2), \
>> + .access = access_raz_id_reg, \
>> + .get_user = get_raz_id_reg, \
>> + .set_user = set_raz_id_reg, \
>> +}
>> +
>> +/*
>> + * sys_reg_desc initialiser for known ID registers that we hide from guests.
>> + * For now, these are exposed just like unallocated ID regs: they appear
>> + * RAZ for the guest.
>> + */
>
> What is a hidden ID register as opposed to an unallocated one?
A hidden register is one where all the features have been removed (RAZ),
making it similar to an unallocated one.
> Shouldn't one of them presumably cause an undefined exception in the
> guest?
No, that'd be a violation of the architecture. The unallocated ID
registers are required to be RAZ (see table D9-2 in D9.3.1), so that
software can probe for feature without running the risk of getting an UNDEF.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
