Hi Stefano, > On 22 Aug 2024, at 22:29, Stefano Stabellini <sstabell...@kernel.org> wrote: > > On Thu, 21 Aug 2024, Bertrand Marquis wrote: >>> On 22 Aug 2024, at 11:00, Michal Orzel <michal.or...@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.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 >> >> 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.
