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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Julien,

On 09/09/2024 10:11, Julien Grall wrote:
> On 09/09/2024 09:56, Michal Orzel wrote:
> > Hi Julien,
> >
> > On 08/09/2024 23:05, Julien Grall wrote:
> > > Hi Ayan,
> > >
> > > On 06/09/2024 11:13, Ayan Kumar Halder wrote:
> > > > From: Michal Orzel <michal.orzel@xxxxxxx>
> > > >
> > > > AOU are the assumptions Xen relies on other components (eg 
> > > > platform, domains)
> > > Searching online, I think the abbrevition is AoU rather than AOU. This
> > > would also match how we abbreviate in Xen (IOW if we use a lower case
> > > letter from the expanded name, then the letter in the acronym is also
> > > lower case).
> > >
> > > > 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.
> > > > 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  | 19 
> > > > +++++++++++++++++++
> > > >    docs/fusa/reqs/intro.rst                      | 10 ++++++++++
> > > >    2 files changed, 29 insertions(+)
> > > >
> > > > diff --git a/docs/fusa/reqs/design-reqs/arm64/generic-timer.rst 
> > > > b/docs/fusa/reqs/design-reqs/arm64/generic-timer.rst
> > > > index f2a0cd7fb8..9df87cf4e0 100644
> > > > --- a/docs/fusa/reqs/design-reqs/arm64/generic-timer.rst
> > > > +++ b/docs/fusa/reqs/design-reqs/arm64/generic-timer.rst
> > > > @@ -116,6 +116,25 @@ 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 ensure that CNTFRQ_EL0 register contains 
> > > > the system
> > > > +timer frequency.
> > > The wording in [1] (not yet merged) implies that CNTFRQ_EL0 may be
> > It is merged:
> > https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=51ad2c57a2d21b583a5944a0dc21c709af022f3c 
> >
> > > invalid. This seems to contradict the Assumption of Use. Can you 
> > > explain
> > > the difference?
> > The requirement you refer to is written from a domain perspective and 
> > is about Xen exposing the frequency
> > to domains via CNTFRQ and/or dt property. In case of a presence of dt 
> > property in the host dtb, Xen could for instance decide
> > to emulate CNTFRQ instead of relying on the domain to parse the dt at 
> > runtime.
> AFAICT, you can't trap CNTFRQ access. So what you suggest would not work.
>
> > The AoU on the platform (hw/firmware/bootloader) is written from Xen 
> > perspective and is about the platform
> > exposing the correct frequency via register. This is Xen expected 
> > behavior on the platform. In other words, the platform should
> > expose the correct frequency via register.
> Xen is able to deal with broken CNTFRQ_EL0. 
Yes, this is correct if the user provides "clock-frequency" dt property.
> So I don't understand why we we would want to make an assumption that 
> it shall not be broken. What do you gain?
Refer 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
```

Use of this property is strongly discouraged; fix your firmware unless 
absolutely impossible.
```

We wish to put the onus on the platform/firmware provider to program the 
register correctly. Otherwise, we will have to document it somewhere 
(may be safety manual) that User needs to provide the "clock-frequency" 
dt property.
We wish to put as little responsibility on the user as 
possible(especially when the dt documentation discourages it as well).
> > > > +
> > > > +Rationale:
> > > This seems to be a bit odd to have an empty section. Can you explain 
> > > why?
> > That's the format we decided to go with. It's been documented in 
> > docs/fusa/reqs/intro.rst.
> > While AFAICT it is not strictly required for OFT, in the future we 
> > can decide to write our own parser to
> > present the requirements in a nicer form that OFT exporter. Then, it 
> > will be easier for use if each
> > requirement defines the same fields (I agree it's a matter of opinion 
> > but that's what we decided to use).
> So this is explaining why you decided to add a section "Rationale", 
> but this doesn't explain why you left it empty. Surely, if you write 
> an assumption, you want to explain why.
Agreed.

If you are fine with my rationale (explained before), I can put the 
exact same words.
- Ayan