Hi james,
Thanks for the mail and sorry for my late response.
2017-10-19 1:21 GMT+08:00, James Morse <james.morse@xxxxxxx>:
> Hi Dongjiu Geng,
>
> On 17/10/17 15:14, Dongjiu Geng wrote:
>> ARMv8.2 adds a new bit HCR_EL2.TEA which controls to
>> route synchronous external aborts to EL2, and adds a
>> trap control bit HCR_EL2.TERR which controls to
>> trap all Non-secure EL1&0 error record accesses to EL2.
>
> The bulk of this patch is about trap-and-emulating these ERR registers, but
> that's not reflected in the title:
>> KVM: arm64: Emulate RAS error registers and set HCR_EL2's TERR & TEA
>
>
>> This patch enables the two bits for the guest OS.
>> when an synchronous abort is generated in the guest OS,
>> it will trap to EL3 firmware, EL3 firmware will check the
>
> *buzz*
> This depends on SCR_EL3.EA, which this patch doesn't touch and the
> normal-world
> can't even know about. This is what your system does, the commit message
> should
> be about the change to Linux.
>
> (I've said this before)
Thanks for the point out, I make this series in a hurry(you are
waiting this patch), forget to check again your comments before.
>
>
>> HCR_EL2.TEA value to decide to jump to hypervisor or host
>> OS. Enabling HCR_EL2.TERR makes error record access
>> from guest trap to EL2.
>>
>> Add some minimal emulation for RAS-Error-Record registers.
>> In the emulation, ERRIDR_EL1 and ERRSELR_EL1 are zero.
>> Then, the others ERX* registers are RAZ/WI.
>
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h
>> b/arch/arm64/include/asm/kvm_emulate.h
>> index fe39e68..47983db 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -47,6 +47,13 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu
>> *vcpu)
>> vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS;
>> if (is_kernel_in_hyp_mode())
>> vcpu->arch.hcr_el2 |= HCR_E2H;
>> + if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN)) {
>
> This ARM64_HAS_RAS_EXTN isn't in mainline, nor is it added by your series.
> I
> know where it comes from, but other reviewers may not. If you have
> dependencies
> on another series, please call them out in the cover letter.
yes, thanks for the point out.
>
> This is the first cpus_have_const_cap() user in this header file, it
> probably needs:
> #include <asm/cpufeature.h>
OK
>
>
>> + /* route synchronous external abort exceptions to EL2 */
>> + vcpu->arch.hcr_el2 |= HCR_TEA;
>> + /* trap error record accesses */
>> + vcpu->arch.hcr_el2 |= HCR_TERR;
>> + }
>> +
>> if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
>> vcpu->arch.hcr_el2 &= ~HCR_RW;
>> }
>> diff --git a/arch/arm64/include/asm/kvm_host.h
>> b/arch/arm64/include/asm/kvm_host.h
>> index d686300..af55b3bc 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -105,6 +105,8 @@ enum vcpu_sysreg {
>> TTBR1_EL1, /* Translation Table Base Register 1 */
>> TCR_EL1, /* Translation Control Register */
>> ESR_EL1, /* Exception Syndrome Register */
>
>> + ERRIDR_EL1, /* Error Record ID Register */
>
> Page 39 of [0]: 'ERRIDR_EL1 is a 64-bit read-only ...'.
yes, it is read-only.
>
>
>> + ERRSELR_EL1, /* Error Record Select Register */
>
> We're emulating these as RAZ/WI, do we really need to allocate
> vcpu->arch.ctxt.sys_regs space for them? If we always return 0 for ERRIDR,
> then
> we don't need to keep ERRSELR as 'the value read back [..] is UNKNOWN'.
https://lists.cs.columbia.edu/pipermail/kvmarm/2017-September/027176.html
" 'If ERRSELR_EL1.SEL is
[>=] ERRIDR_EL1.NUM' that makes the ERX* registers RAZ/WI"
This is because I want to make above simulation as you suggested, if
want to make above simulation, it needs set "vcpu->arch.ctxt.sys_regs"
to 0, instead of reading from system register.
so need a space to store it
>
> I think we only need space for these once their value needs to be migrated,
> user-space doesn't need to know they exist until then.
>
>
>> AFSR0_EL1, /* Auxiliary Fault Status Register 0 */
>> AFSR1_EL1, /* Auxiliary Fault Status Register 1 */
>> FAR_EL1, /* Fault Address Register */
>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 2e070d3..a74617b 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -775,6 +775,36 @@ static bool access_pmovs(struct kvm_vcpu *vcpu,
>> struct sys_reg_params *p,
>> return true;
>> }
>>
>> +static bool access_error_reg(struct kvm_vcpu *vcpu, struct sys_reg_params
>> *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + /* accessing ERRIDR_EL1 */
>> + if (r->CRm == 3 && r->Op2 == 0) {
>> + if (p->is_write)
>> + vcpu_sys_reg(vcpu, ERRIDR_EL1) = 0;
>
> As above, this register is read-only.
yes, it is my mistake.
>
>
>> + return trap_raz_wi(vcpu, p, r);
>
> If we can get rid of the vcpu storage this just becomes trap_raz_wi() .
yes
>
>
>> + }
>> +
>> + /* accessing ERRSELR_EL1 */
>> + if (r->CRm == 3 && r->Op2 == 1) {
>> + if (p->is_write)
>> + vcpu_sys_reg(vcpu, ERRSELR_EL1) = 0;
>> +
>> + return trap_raz_wi(vcpu, p, r);
>> + }
>
> Same here.
>
>
>> +
>> + /*
>> + * If ERRSELR_EL1.SEL is greater than or equal to ERRIDR_EL1.NUM,
>> + * the ERX* registers are RAZ/WI.
>> + */
>> + if ((vcpu_sys_reg(vcpu, ERRSELR_EL1) & 0xff) >=
>> + (vcpu_sys_reg(vcpu, ERRIDR_EL1) && 0xff))
>> + return trap_raz_wi(vcpu, p, r);
>
> The trick here is ERRIDR_EL1 is read only, KVM can choose how many records
> it
> emulates. Let's pick zero:
yes, if it is read only, just pick zero.
>> Zero indicates no records can be accessed through the Error Record System
>> registers.
>
> Which lets us collapse this entire function down to trap_raz_wi().
>
>
> I agree we will need this code once we need to allow user-space to populate
> values for these registers. (and I bet no-one will want to do that until we
> get
> kernel-first support)
>
yes.
>
>> + return true;
>> +}
>> +
>> static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params
>> *p,
>> const struct sys_reg_desc *r)
>> {
>> @@ -953,6 +983,16 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>> { SYS_DESC(SYS_AFSR0_EL1), access_vm_reg, reset_unknown, AFSR0_EL1 },
>> { SYS_DESC(SYS_AFSR1_EL1), access_vm_reg, reset_unknown, AFSR1_EL1 },
>> { SYS_DESC(SYS_ESR_EL1), access_vm_reg, reset_unknown, ESR_EL1 },
>> +
>> + { SYS_DESC(SYS_ERRIDR_EL1), access_error_reg, reset_val, ERRIDR_EL1, 0
>> },
>> + { SYS_DESC(SYS_ERRSELR_EL1), access_error_reg, reset_val, ERRSELR_EL1, 0
>> },
>> + { SYS_DESC(SYS_ERXFR_EL1), access_error_reg },
>> + { SYS_DESC(SYS_ERXCTLR_EL1), access_error_reg },
>> + { SYS_DESC(SYS_ERXSTATUS_EL1), access_error_reg },
>> + { SYS_DESC(SYS_ERXADDR_EL1), access_error_reg },
>> + { SYS_DESC(SYS_ERXMISC0_EL1), access_error_reg },
>> + { SYS_DESC(SYS_ERXMISC1_EL1), access_error_reg },
>
> Why can't we just make all these trap_raz_wi()?
https://lists.cs.columbia.edu/pipermail/kvmarm/2017-September/027176.html
you ever have below suggestion, so use a function "access_error_reg" to do it.
'If ERRSELR_EL1.SEL is
[>=] ERRIDR_EL1.NUM' that makes the ERX* registers RAZ/WI too.
make all to trap_raz_wi() is simple.
>
>
>> { SYS_DESC(SYS_FAR_EL1), access_vm_reg, reset_unknown, FAR_EL1 },
>> { SYS_DESC(SYS_PAR_EL1), NULL, reset_unknown, PAR_EL1 },
>
>
> I'll make these changes and then repost this as part of the RAS+IESB series
> it
> depends on.
Ok, thanks
>
>
> Thanks!
>
> James
>
>
> [0]
> https://static.docs.arm.com/ddi0587/a/RAS%20Extension-release%20candidate_march_29.pdf
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
