Re: [PATCH 2/2] KVM: arm64: timers: Adjust CVAL of a ptimer across guest entry and exits

[Marc Zyngier]

On Thu, 17 Aug 2023 10:27:55 +0100,
Ganapatrao Kulkarni <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxx> 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."

I think this is just bad writing. The functionality isn't disabled,
since you can access CNTPOFF_EL2. It is just that, as for CNTVOFF, the
offset isn't applied to reads of the physical counter.

> 
> "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>

ECV is *not* disabled. It is just that CNTPOFF isn't applied to the
counter as viewed from EL2.

> 
> 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.

Again, you're misreading the spec. When the guest reads the counter,
it reads something that is offset by CNTPOFF. When the guest programs
CVAL, it is an offset from the value it has read. When the HW compares
the physical counter to CVAL, it applies CNTPOFF to the counter.

There is no context to these things, and the value of TGE should not
matter. And even if it did, at the point where we enter the guest,
everything should be hunky dory: the HW has all the information, and
no interrupt should fire if the offseted counter is less than the
deadline.

Again, this is exactly like CNTVOFF.

> 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.

We clearly are reading different things in the spec, as I cannot see
*any* difference in how they are described. If your HW has built them
differently, you have a problem.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.