Re: [PATCH v3] docs: fusa: Add Assumption of Use (AoU)

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Ayan,

> On 23 Sep 2024, at 18:57, Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
> 
> 
> On 19/09/2024 13:01, Bertrand Marquis wrote:
>> Hi Ayan,
> Hi Bertrand,
>> 
>>> On 16 Sep 2024, at 14:18, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> 
>>> wrote:
>>> 
>>> From: Michal Orzel <michal.orzel@xxxxxxx>
>>> 
>>> AoU are the assumptions that Xen relies on other components (eg platform
>>> platform, domains)
>>> to fulfill its requirements. In our case, platform means a combination
>>> of hardware, firmware and bootloader.
>>> 
>>> We have defined AoU in the intro.rst and added AoU for the generic
>>> timer.
>>> 
>>> Also, fixed a requirement to denote that Xen shall **not** expose the
>>> system counter frequency via the "clock-frequency" device tree property.
>>> The reason being the device tree documentation strongly discourages the
>>> use of this peoperty. Further if the "clock-frequency" is exposed, then
>>> it overrides the value programmed in the CNTFRQ_EL0 register.
>>> 
>>> So, the frequency shall be exposed via the CNTFRQ_EL0 register only and
>>> consequently there is an assumption on the platform to program the
>>> register correctly.
>>> 
>>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
>>> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>
>>> ---
>>> Changes from :-
>>> 
>>> v1 - 1. Removed the part of requirement which states that Xen exposes the
>>> frequency of the system timer by reading the "clock-frequency" property.
>>> 
>>> 2. Added a rationale for AoU.
>>> 
>>> 3. Reworded the AoU.
>>> 
>>> v2 - 1. Reworded the commit message. Added R-b.
>>> 
>>> .../reqs/design-reqs/arm64/generic-timer.rst  | 24 ++++++++++++++++++-
>>> docs/fusa/reqs/intro.rst                      | 10 ++++++++
>>> 2 files changed, 33 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/docs/fusa/reqs/design-reqs/arm64/generic-timer.rst 
>>> b/docs/fusa/reqs/design-reqs/arm64/generic-timer.rst
>>> index f2a0cd7fb8..86d84a3c40 100644
>>> --- a/docs/fusa/reqs/design-reqs/arm64/generic-timer.rst
>>> +++ b/docs/fusa/reqs/design-reqs/arm64/generic-timer.rst
>>> @@ -30,7 +30,7 @@ Read system counter frequency
>>> 
>>> Description:
>>> Xen shall expose the frequency of the system counter to the domains in
>>> -CNTFRQ_EL0 register and/or domain device tree's "clock-frequency" property.
>>> +CNTFRQ_EL0 register.
>>> 
>>> Rationale:
>>> 
>>> @@ -116,6 +116,28 @@ Rationale:
>>> 
>>> Comments:
>>> 
>>> +Covers:
>>> + - `XenProd~emulated_timer~1`
>>> +
>>> +Assumption of Use on the Platform
>>> +=================================
>>> +
>>> +Expose system timer frequency via register
>>> +------------------------------------------
>>> +
>>> +`XenSwdgn~arm64_generic_timer_pf_program_cntfrq_el0~1`
>>> +
>>> +Description:
>>> +Underlying platform shall program CNTFRQ_EL0 register with the value of 
>>> system
>>> +timer frequency.
>> How about: CNTFRQ_EL0 register shall be programmed with the value of the 
>> system timer frequency.
>> 
>> It prevent to use "platform" which is quite undefined here.
>> 
>>> +
>>> +Rationale:
>>> +Xen reads the CNTFRQ_EL0 register to get the value of system timer 
>>> frequency.
>>> +While there is a provision to get this value by reading the 
>>> "clock-frequency"
>>> +dt property [2], the use of this property is strongly discouraged.
>> I would put the second sentence as a comment as only the first one is the 
>> rationale explaining
>> why we need it to be correct.
>> 
>>> +
>>> +Comments:
>>> +
>>> Covers:
>>>  - `XenProd~emulated_timer~1`
>>> 
>>> diff --git a/docs/fusa/reqs/intro.rst b/docs/fusa/reqs/intro.rst
>>> index 245a219ff2..aa85ff821c 100644
>>> --- a/docs/fusa/reqs/intro.rst
>>> +++ b/docs/fusa/reqs/intro.rst
>>> @@ -38,6 +38,16 @@ The requirements are linked using OpenFastTrace
>>> OpenFastTrace parses through the requirements and generates a traceability
>>> report.
>>> 
>>> +Assumption of Use
>>> +=================
>>> +
>>> +To fulfill one or more design requirements, there may be underlying 
>>> assumptions
>>> +on one or more components that Xen interacts with directly or indirectly. 
>>> For
>>> +eg, there may be assumptions on the underlying platform (hardware + 
>>> firmware +
>>> +bootloader) to set certain registers, etc. The important thing here is that
>>> +anyone who validates these requirements, need to consider the assumption 
>>> on the
>>> +other components.
> I think there is a typo.
>> I would simplify a bit:
>> Xen is making several assumptions on the status of the platform or on some
>> functions being present and functional.
> s/functional/functionality.

no that sounds weird, functional is right in the sentence i think.

>> For example, Xen might assume that
>> some registers are set.
>> Anybody who wants to use Xen must validate that the platform it is used on
>> (meaning the hardware and any software running before Xen like the firmware)
>> fulfils all the AoU described by Xen.
>> 
>> What do you think ?
> 
> It sounds ok.

Good
Cheers
Bertrand

> 
> - Ayan