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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Stefano,

> On 22 Aug 2024, at 22:29, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Thu, 21 Aug 2024, Bertrand Marquis wrote:
>>> On 22 Aug 2024, at 11:00, Michal Orzel <michal.orzel@xxxxxxx> 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@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
>> 
>> 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.