From: Michal Orzel <michal.orzel@amd.com>

Add the requirements for the use of generic timer by a domain

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 .../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]).
+
+Rationale:
+
+Comments:
+Domains shall probe the Generic Timer device tree node.
+
+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.
+
+Rationale:
+
+Comments:
+Domains shall read it via CNTFRQ_EL0 register or "clock-frequency" device tree
+property.
+
+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.
+
+Rationale:
+
+Comments:
+Domains shall access the counter-timer kernel control register to allow
+controlling the access to the timer from userspace (EL0).
+
+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.
+
+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.
+
+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. 
+
+Rationale:
+
+Comments:
+Domains shall program the virtual timer to generate the interrupt when the
+asserted condition is met.
+
+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
+
+Rationale:
+
+Comments:
+Domains shall program the physical timer to generate the interrupt when the
+asserted condition is met.
+
+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.
+
+Rationale:
+
+Comments:
+Domains shall use it for scheduling and other needs.
+
+Covers:
+ - `XenMkt~run_arm64_vms~1`
+ - `XenMkt~provide_timer_vms~1`
+
+Needs:
+ - XenSwdgn
-- 
2.25.1

Hi Ayan,

> On 20 Aug 2024, at 12:38, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
> 
> From: Michal Orzel <michal.orzel@amd.com>
> 
> Add the requirements for the use of generic timer by a domain
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> .../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

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@amd.com> wrote:
>>
>> From: Michal Orzel <michal.orzel@amd.com>
>>
>> Add the requirements for the use of generic timer by a domain
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> .../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

Hi Michal,

> On 22 Aug 2024, at 11:00, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> 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@amd.com> wrote:
>>> 
>>> From: Michal Orzel <michal.orzel@amd.com>
>>> 
>>> Add the requirements for the use of generic timer by a domain
>>> 
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> ---
>>> .../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

Right then i would use the same upper cases: Counter-timer Kernel Control
register and still mention CNTKCTL_EL1 as it would be clearer.

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

This is true but i think it would be more logic in non design requirements to
phrase things to explain the domain point of view rather than how it is implemented.

Here the point is to have a timer fully functional from guest point of view, including
getting interrupts when the timer should generate one.

Maybe something like: Xen shall generate timer interrupts to domains when the timer condition asserts.

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

Cheers
Bertrand

On Thu, 21 Aug 2024, Bertrand Marquis wrote:
> > On 22 Aug 2024, at 11:00, Michal Orzel <michal.orzel@amd.com> wrote:
> > 
> > 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@amd.com> wrote:
> >>> 
> >>> From: Michal Orzel <michal.orzel@amd.com>
> >>> 
> >>> Add the requirements for the use of generic timer by a domain
> >>> 
> >>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> >>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> >>> ---
> >>> .../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
> 
> Right then i would use the same upper cases: Counter-timer Kernel Control
> register and still mention CNTKCTL_EL1 as it would be clearer.
> 
> > 
> >> 
> >>> +
> >>> +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.
> 
> This is true but i think it would be more logic in non design requirements to
> phrase things to explain the domain point of view rather than how it is implemented.
> 
> Here the point is to have a timer fully functional from guest point of view, including
> getting interrupts when the timer should generate one.
> 
> Maybe something like: Xen shall generate timer interrupts to domains when the timer condition asserts.

Both statements are correct.

Michal's original statement "Xen shall enable the domains to program the
virtual timer to generate the interrupt" is correct. The timer interrupt
will be generated by the hardware to Xen, if the guest programs the
registers correctly. If Xen does nothing, the interrupt is still
generated and received by Xen.

Bertrand's statement is also correct. Xen needs to generate a virtual
timer interrupt equivalent to the physical timer interrupt, after
receiving the physical interrupt.

I think the best statement would be a mix of the two, something like:

Xen shall enable the domain to program the virtual timer to generate
the interrupt, which Xen shall inject as virtual interrupt into the
domain.


That said, I also think that any one of these three statements is good
enough.

Hi Stefano,

> On 22 Aug 2024, at 22:29, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Thu, 21 Aug 2024, Bertrand Marquis wrote:
>>> On 22 Aug 2024, at 11:00, Michal Orzel <michal.orzel@amd.com> wrote:
>>> 
>>> 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@amd.com> wrote:
>>>>> 
>>>>> From: Michal Orzel <michal.orzel@amd.com>
>>>>> 
>>>>> Add the requirements for the use of generic timer by a domain
>>>>> 
>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>>> ---
>>>>> .../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
>> 
>> Right then i would use the same upper cases: Counter-timer Kernel Control
>> register and still mention CNTKCTL_EL1 as it would be clearer.
>> 
>>> 
>>>> 
>>>>> +
>>>>> +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.
>> 
>> This is true but i think it would be more logic in non design requirements to
>> phrase things to explain the domain point of view rather than how it is implemented.
>> 
>> Here the point is to have a timer fully functional from guest point of view, including
>> getting interrupts when the timer should generate one.
>> 
>> Maybe something like: Xen shall generate timer interrupts to domains when the timer condition asserts.
> 
> Both statements are correct.
> 
> Michal's original statement "Xen shall enable the domains to program the
> virtual timer to generate the interrupt" is correct. The timer interrupt
> will be generated by the hardware to Xen, if the guest programs the
> registers correctly. If Xen does nothing, the interrupt is still
> generated and received by Xen.
> 
> Bertrand's statement is also correct. Xen needs to generate a virtual
> timer interrupt equivalent to the physical timer interrupt, after
> receiving the physical interrupt.
> 
> I think the best statement would be a mix of the two, something like:
> 
> Xen shall enable the domain to program the virtual timer to generate
> the interrupt, which Xen shall inject as virtual interrupt into the
> domain.

This should be split into 2 reqs (2 shall) and the second one might in
fact be a generic one for interrupt injections into guests.

Cheers
Bertrand

> 
> 
> That said, I also think that any one of these three statements is good
> enough.

Hi Bertrand/Stefano/Michal,

On 23/08/2024 08:22, Bertrand Marquis wrote:
> Hi Stefano,
>
>> On 22 Aug 2024, at 22:29, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>
>> On Thu, 21 Aug 2024, Bertrand Marquis wrote:
>>>> On 22 Aug 2024, at 11:00, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>
>>>> 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@amd.com> wrote:
>>>>>>
>>>>>> From: Michal Orzel <michal.orzel@amd.com>
>>>>>>
>>>>>> Add the requirements for the use of generic timer by a domain
>>>>>>
>>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>>>> ---
>>>>>> .../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
>>> Right then i would use the same upper cases: Counter-timer Kernel Control
>>> register and still mention CNTKCTL_EL1 as it would be clearer.
>>>
>>>>>> +
>>>>>> +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.
>>> This is true but i think it would be more logic in non design requirements to
>>> phrase things to explain the domain point of view rather than how it is implemented.
>>>
>>> Here the point is to have a timer fully functional from guest point of view, including
>>> getting interrupts when the timer should generate one.
>>>
>>> Maybe something like: Xen shall generate timer interrupts to domains when the timer condition asserts.
>> Both statements are correct.
>>
>> Michal's original statement "Xen shall enable the domains to program the
>> virtual timer to generate the interrupt" is correct. The timer interrupt
>> will be generated by the hardware to Xen, if the guest programs the
>> registers correctly. If Xen does nothing, the interrupt is still
>> generated and received by Xen.
>>
>> Bertrand's statement is also correct. Xen needs to generate a virtual
>> timer interrupt equivalent to the physical timer interrupt, after
>> receiving the physical interrupt.
>>
>> I think the best statement would be a mix of the two, something like:
>>
>> Xen shall enable the domain to program the virtual timer to generate
>> the interrupt, which Xen shall inject as virtual interrupt into the
>> domain.
> This should be split into 2 reqs (2 shall) and the second one might in
> fact be a generic one for interrupt injections into guests.

I agree with you that the second statement shall be a requirement for 
vGIC (as it has nothing to do with the timer).

So, do we agree that the requirements should be

1. Xen shall generate physical timer interrupts to domains when the 
physical timer condition is met.

2. Xen shall generate virtual timer interrupts to domains when the 
virtual timer condition is met.

The important thing here is that Xen can generate both physical timer 
and virtual timer IRQs. It is left to the guests to use one or both.

We can drop the comments here if it is causing confusion.

Let me know your thoughts.

- Ayan

On Fri, 23 Aug 2024, Ayan Kumar Halder wrote:
> Hi Bertrand/Stefano/Michal,
> 
> On 23/08/2024 08:22, Bertrand Marquis wrote:
> > Hi Stefano,
> > 
> > > On 22 Aug 2024, at 22:29, Stefano Stabellini <sstabellini@kernel.org>
> > > wrote:
> > > 
> > > On Thu, 21 Aug 2024, Bertrand Marquis wrote:
> > > > > On 22 Aug 2024, at 11:00, Michal Orzel <michal.orzel@amd.com> wrote:
> > > > > 
> > > > > 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@amd.com> wrote:
> > > > > > > 
> > > > > > > From: Michal Orzel <michal.orzel@amd.com>
> > > > > > > 
> > > > > > > Add the requirements for the use of generic timer by a domain
> > > > > > > 
> > > > > > > Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> > > > > > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> > > > > > > ---
> > > > > > > .../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
> > > > Right then i would use the same upper cases: Counter-timer Kernel
> > > > Control
> > > > register and still mention CNTKCTL_EL1 as it would be clearer.
> > > > 
> > > > > > > +
> > > > > > > +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.
> > > > This is true but i think it would be more logic in non design
> > > > requirements to
> > > > phrase things to explain the domain point of view rather than how it is
> > > > implemented.
> > > > 
> > > > Here the point is to have a timer fully functional from guest point of
> > > > view, including
> > > > getting interrupts when the timer should generate one.
> > > > 
> > > > Maybe something like: Xen shall generate timer interrupts to domains
> > > > when the timer condition asserts.
> > > Both statements are correct.
> > > 
> > > Michal's original statement "Xen shall enable the domains to program the
> > > virtual timer to generate the interrupt" is correct. The timer interrupt
> > > will be generated by the hardware to Xen, if the guest programs the
> > > registers correctly. If Xen does nothing, the interrupt is still
> > > generated and received by Xen.
> > > 
> > > Bertrand's statement is also correct. Xen needs to generate a virtual
> > > timer interrupt equivalent to the physical timer interrupt, after
> > > receiving the physical interrupt.
> > > 
> > > I think the best statement would be a mix of the two, something like:
> > > 
> > > Xen shall enable the domain to program the virtual timer to generate
> > > the interrupt, which Xen shall inject as virtual interrupt into the
> > > domain.
> > This should be split into 2 reqs (2 shall) and the second one might in
> > fact be a generic one for interrupt injections into guests.
> 
> I agree with you that the second statement shall be a requirement for vGIC (as
> it has nothing to do with the timer).
> 
> So, do we agree that the requirements should be
> 
> 1. Xen shall generate physical timer interrupts to domains when the physical
> timer condition is met.
> 
> 2. Xen shall generate virtual timer interrupts to domains when the virtual
> timer condition is met.
> 
> The important thing here is that Xen can generate both physical timer and
> virtual timer IRQs. It is left to the guests to use one or both.
> 
> We can drop the comments here if it is causing confusion.
> 
> Let me know your thoughts.

I'm happy to give my approval to any and all versions discussed in this
thread:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


I could suggest further improvements or point out minor issues with any
of the wordings (including my wording), but I don't think it would be
useful. Any of these statements is good. I don't believe we need to
refine the English text any further.

Unlike code, the potential for text revisions in English is limitless.
It's always possible to find things not quite clear enough, or rephrase
in a way that is clearer and more comprehensive. Also because "clear" is
subjective.

I think it is important that we do not put too much effort into this
because the reward is not proportional.