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