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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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.

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

> +
> +Rationale:
> +
> +Comments:
> +Domains shall program the virtual timer to generate the interrupt when the
> +asserted condition is met.

What a timer is used for should not really be documented here

> +
> +Covers:
> + - `XenProd~emulated_timer~1`
> +
> +Trigger the physical timer interrupt from a domain
> +--------------------------------------------------
> +
> +`XenSwdgn~arm64_generic_timer_trigger_physical_interrupt~1`
> +
> +Description:
> +Xen shall enable the domains to program the physical timer to generate the
> +interrupt

same as upper

> +
> +Rationale:
> +
> +Comments:
> +Domains shall program the physical timer to generate the interrupt when the
> +asserted condition is met.

same as upper

> +
> +Covers:
> + - `XenProd~emulated_timer~1`
> +
> +[1] Arm Architecture Reference Manual for A-profile architecture, Chapter 11
> +[2] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
> diff --git a/docs/fusa/reqs/index.rst b/docs/fusa/reqs/index.rst
> index 78c02b1d9b..183f183b1f 100644
> --- a/docs/fusa/reqs/index.rst
> +++ b/docs/fusa/reqs/index.rst
> @@ -7,3 +7,6 @@ Requirements documentation
>    :maxdepth: 2
> 
>    intro
> +   market-reqs
> +   product-reqs
> +   design-reqs/arm64
> diff --git a/docs/fusa/reqs/intro.rst b/docs/fusa/reqs/intro.rst
> index d67b18dd9f..245a219ff2 100644
> --- a/docs/fusa/reqs/intro.rst
> +++ b/docs/fusa/reqs/intro.rst
> @@ -55,7 +55,8 @@ Title of the requirement
>   be 'XenMkt', 'XenProd' or 'XenSwdgn' to denote market, product or design
>   requirement.
>   name - This denotes name of the requirement. In case of architecture 
> specific
> -  requirements, this starts with the architecture type (ie x86_64, arm64).
> +  requirements, this starts with the architecture type (eg x86_64, arm64)
> +  followed by component name (eg generic_timer) and action (eg read_xxx).
>   revision number - This gets incremented each time the requirement is 
> modified.
> 
> 
> diff --git a/docs/fusa/reqs/market-reqs/reqs.rst 
> b/docs/fusa/reqs/market-reqs/reqs.rst
> new file mode 100644
> index 0000000000..9c98c84a9a
> --- /dev/null
> +++ b/docs/fusa/reqs/market-reqs/reqs.rst
> @@ -0,0 +1,34 @@
> +.. SPDX-License-Identifier: CC-BY-4.0
> +
> +Functional Requirements
> +=======================
> +
> +Run Arm64 VMs
> +-------------
> +
> +`XenMkt~run_arm64_vms~1`
> +
> +Description:
> +Xen shall run Arm64 VMs.
> +
> +Rationale:
> +
> +Comments:
> +
> +Needs:
> + - XenProd
> +
> +Provide timer to the VMs
> +------------------------
> +
> +`XenMkt~provide_timer_vms~1`
> +
> +Description:
> +Xen shall provide a timer to a VM.
> +
> +Rationale:
> +
> +Comments:
> +
> +Needs:
> + - XenProd
> diff --git a/docs/fusa/reqs/product-reqs/arm64/reqs.rst 
> b/docs/fusa/reqs/product-reqs/arm64/reqs.rst
> new file mode 100644
> index 0000000000..9b579cc606
> --- /dev/null
> +++ b/docs/fusa/reqs/product-reqs/arm64/reqs.rst
> @@ -0,0 +1,24 @@
> +.. SPDX-License-Identifier: CC-BY-4.0
> +
> +Domain Creation And Runtime
> +===========================
> +
> +Emulated Timer
> +--------------
> +
> +`XenProd~emulated_timer~1`
> +
> +Description:
> +Xen shall emulate Arm Generic Timer timer on behalf of domains.

Xen is not emulating but giving access or providing one.
How it is done is down to the implementation.

> +
> +Rationale:
> +
> +Comments:
> +Domains shall use it for scheduling and other needs.

This comment should be removed.

> +
> +Covers:
> + - `XenMkt~run_arm64_vms~1`
> + - `XenMkt~provide_timer_vms~1`
> +
> +Needs:
> + - XenSwdgn
> -- 
> 2.25.1
> 

Cheers
Bertrand