Re: [PATCH] docs: fusa: Add requirements for generic timer

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Bertrand,

I agree with all your comments with a few exceptions, as stated below.

On 21/08/2024 17:06, Bertrand Marquis wrote:
> 
> 
> Hi Ayan,
> 
>> On 20 Aug 2024, at 12:38, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> 
>> wrote:
>>
>> From: Michal Orzel <michal.orzel@xxxxxxx>
>>
>> Add the requirements for the use of generic timer by a domain
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
>> ---
>> .../reqs/design-reqs/arm64/generic-timer.rst  | 139 ++++++++++++++++++
>> docs/fusa/reqs/index.rst                      |   3 +
>> docs/fusa/reqs/intro.rst                      |   3 +-
>> docs/fusa/reqs/market-reqs/reqs.rst           |  34 +++++
>> docs/fusa/reqs/product-reqs/arm64/reqs.rst    |  24 +++
>> 5 files changed, 202 insertions(+), 1 deletion(-)
>> create mode 100644 docs/fusa/reqs/design-reqs/arm64/generic-timer.rst
>> create mode 100644 docs/fusa/reqs/market-reqs/reqs.rst
>> create mode 100644 docs/fusa/reqs/product-reqs/arm64/reqs.rst
>>
>> diff --git a/docs/fusa/reqs/design-reqs/arm64/generic-timer.rst 
>> b/docs/fusa/reqs/design-reqs/arm64/generic-timer.rst
>> new file mode 100644
>> index 0000000000..bdd4fbf696
>> --- /dev/null
>> +++ b/docs/fusa/reqs/design-reqs/arm64/generic-timer.rst
>> @@ -0,0 +1,139 @@
>> +.. SPDX-License-Identifier: CC-BY-4.0
>> +
>> +Generic Timer
>> +=============
>> +
>> +The following are the requirements related to ARM Generic Timer [1] 
>> interface
>> +exposed by Xen to Arm64 domains.
>> +
>> +Probe the Generic Timer device tree node from a domain
>> +------------------------------------------------------
>> +
>> +`XenSwdgn~arm64_generic_timer_probe_dt~1`
>> +
>> +Description:
>> +Xen shall generate a device tree node for the Generic Timer (in accordance 
>> to
>> +ARM architected timer device tree binding [2]).
> 
> You might want to say where here. The domain device tree ?
> 
>> +
>> +Rationale:
>> +
>> +Comments:
>> +Domains shall probe the Generic Timer device tree node.
> 
> Please prevent the use of "shall" here. I would use "can".
> Also detect the presence of might be better than probe.
> 
>> +
>> +Covers:
>> + - `XenProd~emulated_timer~1`
>> +
>> +Read system counter frequency
>> +-----------------------------
>> +
>> +`XenSwdgn~arm64_generic_timer_read_freq~1`
>> +
>> +Description:
>> +Xen shall expose the frequency of the system counter to the domains.
> 
> The requirement would need to say in CNTFRQ_EL0 and in the domain device tree 
> xxx entry.
> 
>> +
>> +Rationale:
>> +
>> +Comments:
>> +Domains shall read it via CNTFRQ_EL0 register or "clock-frequency" device 
>> tree
>> +property.
> 
> I do not think this comment is needed.
> 
>> +
>> +Covers:
>> + - `XenProd~emulated_timer~1`
>> +
>> +Access CNTKCTL_EL1 system register from a domain
>> +------------------------------------------------
>> +
>> +`XenSwdgn~arm64_generic_timer_access_cntkctlel1~1`
>> +
>> +Description:
>> +Xen shall expose counter-timer kernel control register to the domains.
> 
> "counter-timer kernel" is a bit odd here, is it the name from arm arm ?
> Generic Timer control registers ? or directly the register name.
This is the name from Arm ARM. See e.g.:
https://developer.arm.com/documentation/ddi0601/2023-12/AArch64-Registers/CNTKCTL-EL1--Counter-timer-Kernel-Control-Register?lang=en

> 
>> +
>> +Rationale:
>> +
>> +Comments:
>> +Domains shall access the counter-timer kernel control register to allow
>> +controlling the access to the timer from userspace (EL0).
> 
> This is documented in the arm arm, this comment is not needed.
> 
>> +
>> +Covers:
>> + - `XenProd~emulated_timer~1`
>> +
>> +Access virtual timer from a domain
>> +----------------------------------
>> +
>> +`XenSwdgn~arm64_generic_timer_access_virtual_timer~1`
>> +
>> +Description:
>> +Xen shall expose the virtual timer registers to the domains.
>> +
>> +Rationale:
>> +
>> +Comments:
>> +Domains shall access and make use of the virtual timer by accessing the
>> +following system registers:
>> +CNTVCT_EL0,
>> +CNTV_CTL_EL0,
>> +CNTV_CVAL_EL0,
>> +CNTV_TVAL_EL0.
> 
> The requirement to be complete should give this list, not the comment.
> 
>> +
>> +Covers:
>> + - `XenProd~emulated_timer~1`
>> +
>> +Access physical timer from a domain
>> +-----------------------------------
>> +
>> +`XenSwdgn~arm64_generic_timer_access_physical_timer~1`
>> +
>> +Description:
>> +Xen shall expose physical timer registers to the domains.
>> +
>> +Rationale:
>> +
>> +Comments:
>> +Domains shall access and make use of the physical timer by accessing the
>> +following system registers:
>> +CNTPCT_EL0,
>> +CNTP_CTL_EL0,
>> +CNTP_CVAL_EL0,
>> +CNTP_TVAL_EL0.
> 
> same as upper
> 
>> +
>> +Covers:
>> + - `XenProd~emulated_timer~1`
>> +
>> +Trigger the virtual timer interrupt from a domain
>> +-------------------------------------------------
>> +
>> +`XenSwdgn~arm64_generic_timer_trigger_virtual_interrupt~1`
>> +
>> +Description:
>> +Xen shall enable the domains to program the virtual timer to generate the
>> +interrupt.
> 
> I am not sure this is right here.
> You gave access to the registers upper so Xen responsibility is not really to
> enable anything but rather make sure that it generates an interrupt according 
> to
> how the registers have been programmed.
I'm in two minds about it. On one hand you're right and the IRQ trigger is a 
side-effect
of programming the registers correctly. On the other, these are design 
requirements which
according to "fusa/reqs/intro.rst" describe what SW implementation is doing. 
Our way of injecting
timer IRQs into guests is a bit different (phys timer is fully emulated and we 
use internal timers
and for virt timer we first route IRQ to Xen, mask the IRQ and inject to 
guest). If I were to write
tests to cover Generic Timer requirements I'd expect to cover whether r.g. 
masking/unmasking IRQ works,
whether IRQ was received, etc.

I'd like to know other opinions. @Stefano, @Artem

~Michal