| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <946b0fc2-3264-d7ab-f5e1-7c9e76db6ebf@os.amperecomputing.com>
Date: Tue, 22 Aug 2023 20:27:21 +0530
From: Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>
To: Marc Zyngier <maz@...nel.org>
Cc: linux-kernel@...r.kernel.org, kvmarm@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, eauger@...hat.com,
miguel.luis@...cle.com, darren@...amperecomputing.com,
scott@...amperecomputing.com,
Christoffer Dall <Christoffer.Dall@....com>
Subject: Re: [PATCH 2/2] KVM: arm64: timers: Adjust CVAL of a ptimer across
guest entry and exits
On 22-08-2023 05:46 pm, Marc Zyngier wrote:
> On Thu, 17 Aug 2023 10:27:55 +0100,
> Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com> wrote:
>>
>>
>> Hi Marc,
>>
>> On 17-08-2023 01:57 pm, Marc Zyngier wrote:
>>> [Fixing Christoffer's email address]
>>
>> Thanks.
>>>
>>> On Thu, 17 Aug 2023 07:03:14 +0100,
>>> Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com> wrote:
>>>>
>>>> As per FEAT_ECV, when HCR_EL2.{E2H, TGE} == {1, 1}, Enhanced Counter
>>>> Virtualization functionality is disabled and CNTPOFF_EL2 value is treated
>>>> as zero. On VHE host, E2H and TGE are set, hence it is required
>>>> to adjust CVAL by incrementing it by CNTPOFF_EL2 after guest
>>>> exit to avoid false physical timer interrupts and also
>>>> decrement/restore CVAL before the guest entry.
>>>
>>> No, this is wrong. Neither E2H nor TGE have any impact on writing to
>>> CNTPOFF_EL2, nor does it have an impact on CNTP_CVAL_EL0. Just read
>>> the pseudocode to convince yourself.
>>>
>>> CNTPOFF_EL2 is applied at exactly two points: when SW is reading
>>> CNTPCT_EL0 from EL1 while {E2H,TGE}=={1, 0} and when the HW is
>>> comparing CNTPCT_EL0 with the CNTP_CVAL_EL0. In both cases the offset
>>> is subtracted from the counter. And that's the point where the running
>>> EL matters. Which means that CNTPOFF_EL2 behaves exactly like
>>> CNTVOFF_EL2. No ifs, no buts.
>>
>> As per ARM ARM (ARM DDI 0487J.a page D11-5989)
>> "When FEAT_ECV is implemented, the CNTPOFF_EL2 register allows an
>> offset to be applied to the physical counter, as viewed from EL1 and
>> EL0, and to the EL1 physical timer. The functionality of this 64-bit
>> register is affected by CNTHCTL_EL2.ECV."
>>
>> As per ARM ARM (ARM DDI 0487J.a page D19-7857)
>> "When HCR_EL2.{E2H, TGE} == {1, 1} or SCR_EL3.{NS, EEL2} == {0, 0}, then
>> Enhanced Counter Virtualization functionality is disabled."
>>
>> "The EL1 physical timer interrupt is triggered when ((PCount<63:0> -
>> CNTPOFF_EL2<63:0>) - PCVal<63:0>) is greater than or equal to 0."
>>
>> As per ARM ARM (ARM DDI 0487J.a page D19-7938)
>> "When EL2 is implemented and enabled in the current Security state,
>> the physical counter uses a fixed physical offset of *zero* if any of
>> the following are true:
>> • CNTHCTL_EL2.ECV is 0.
>> • SCR_EL3.ECVEn is 0.
>> • HCR_EL2.{E2H, TGE} is {1, 1}."
>>
>> In VHE host hypervisor, E2H=TGE=1 hence ECV is disabled and Ptimer
>> interrupt is triggered based on PCount<63:0> - PCVal<63:0>
>>
>> Since cval is set by Guest as per offsetted PCounter value and pCount
>> is not subtracted by CNTPOFF when in VHE-L0, results in cval becoming
>> much lesser than physical counter(bumped up since CNTPOFF is zero) and
>> timer interrupt trigger condition is met falsely.
>>
>> There is no issue/impact on cval due to ECV, however it can be/is
>> manipulated to handle this on and off of CNTPOFF/ECV.
>>
>> IIUC, CNTPOFF and CNTVOFF are not same as per specification.
>
> I owe you an apology. You are correct, and I was totally wrong. I'm
> truly amazed how wrong we got this part of the architecture, but it is
> way too late for any change, and we'll have to live with it.
>
Thanks Marc.
> Now, to the actual patch: I think the way you offset CVAL isn't
> great. You should never have to change it on entry, and you should
> instead read the correct value from memory. Then, save/restore of CVAL
> must be amended to always apply the offset. Can you give the hack
> below a go on your HW?
Sure, I will try this and update.
>
> Thanks,
>
> M.
>
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index ea46b4e1e7a8..bb80fdd84676 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -55,11 +55,6 @@ static struct irq_ops arch_timer_irq_ops = {
> .get_input_level = kvm_arch_timer_get_input_level,
> };
>
> -static bool has_cntpoff(void)
> -{
> - return (has_vhe() && cpus_have_final_cap(ARM64_HAS_ECV_CNTPOFF));
> -}
> -
> static int nr_timers(struct kvm_vcpu *vcpu)
> {
> if (!vcpu_has_nv2(vcpu))
> @@ -180,7 +175,7 @@ u64 kvm_phys_timer_read(void)
> return timecounter->cc->read(timecounter->cc);
> }
>
> -static void get_timer_map(struct kvm_vcpu *vcpu, struct timer_map *map)
> +void get_timer_map(struct kvm_vcpu *vcpu, struct timer_map *map)
> {
> if (vcpu_has_nv2(vcpu)) {
> if (is_hyp_ctxt(vcpu)) {
> @@ -569,8 +564,7 @@ static void timer_save_state(struct arch_timer_context *ctx)
> timer_set_ctl(ctx, read_sysreg_el0(SYS_CNTP_CTL));
> cval = read_sysreg_el0(SYS_CNTP_CVAL);
>
> - if (!has_cntpoff())
> - cval -= timer_get_offset(ctx);
> + cval -= timer_get_offset(ctx);
>
> timer_set_cval(ctx, cval);
>
> @@ -657,8 +651,7 @@ static void timer_restore_state(struct arch_timer_context *ctx)
> cval = timer_get_cval(ctx);
> offset = timer_get_offset(ctx);
> set_cntpoff(offset);
> - if (!has_cntpoff())
> - cval += offset;
> + cval += offset;
> write_sysreg_el0(cval, SYS_CNTP_CVAL);
> isb();
> write_sysreg_el0(timer_get_ctl(ctx), SYS_CNTP_CTL);
> diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> index 9611b4eaf661..6e3d3e16563f 100644
> --- a/arch/arm64/kvm/hyp/vhe/switch.c
> +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> @@ -90,6 +90,20 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
>
> ___activate_traps(vcpu, hcr);
>
> + if (has_cntpoff()) {
> + struct timer_map map;
> +
> + get_timer_map(vcpu, &map);
> +
> + if (map.direct_ptimer == vcpu_ptimer(vcpu))
> + val = __vcpu_sys_reg(vcpu, CNTP_CVAL_EL0);
> + else
> + val = __vcpu_sys_reg(vcpu, CNTHP_CVAL_EL2);
> +
> + isb();
> + write_sysreg_s(val, SYS_CNTP_CVAL_EL0);
> + }
> +
> val = read_sysreg(cpacr_el1);
> val |= CPACR_ELx_TTA;
> val &= ~(CPACR_EL1_ZEN_EL0EN | CPACR_EL1_ZEN_EL1EN |
> @@ -131,6 +145,23 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)
>
> write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
>
> + if (has_cntpoff()) {
> + struct timer_map map;
> + u64 val, offset;
> +
> + get_timer_map(vcpu, &map);
> +
> + val = read_sysreg_s(SYS_CNTP_CVAL_EL0);
> + if (map.direct_ptimer == vcpu_ptimer(vcpu))
> + __vcpu_sys_reg(vcpu, CNTP_CVAL_EL0) = val;
> + else
> + __vcpu_sys_reg(vcpu, CNTHP_CVAL_EL2) = val;
> +
> + offset = read_sysreg_s(SYS_CNTPOFF_EL2);
> + write_sysreg_s(val + offset, SYS_CNTP_CVAL_EL0);
> + isb();
> + }
> +
> /*
> * ARM errata 1165522 and 1530923 require the actual execution of the
> * above before we can switch to the EL2/EL0 translation regime used by
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index ea77a569a907..86a73ad1446a 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -82,6 +82,8 @@ struct timer_map {
> struct arch_timer_context *emul_ptimer;
> };
>
> +void get_timer_map(struct kvm_vcpu *vcpu, struct timer_map *map);
> +
> struct arch_timer_cpu {
> struct arch_timer_context timers[NR_KVM_TIMERS];
>
> @@ -149,4 +151,9 @@ void kvm_timer_cpu_down(void);
> /* CNTKCTL_EL1 valid bits as of DDI0476J.a */
> #define CNTKCTL_VALID_BITS (BIT(17) | GENMASK_ULL(9, 0))
>
> +static inline bool has_cntpoff(void)
> +{
> + return (has_vhe() && cpus_have_final_cap(ARM64_HAS_ECV_CNTPOFF));
> +}
> +
> #endif
>
Thanks,
Ganapat
Powered by blists - more mailing lists