Re: [PATCH v3 2/2] docs: fusa: Add the requirements for few commands of XEN_VERSION

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 21/05/2025 13:10, Bertrand Marquis wrote:
> Hi Ayan,
Hi Bertrand,
> > On 9 May 2025, at 13:24, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> wrote:
> >
> > Define requirements for specific commands.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> > Changes from -
> >
> > v1 - 1. Reworded the requirement so as to avoid mentioining variable names
> > or hardcoded strings. Otherwise, one would need to change the requirement
> > each time the code changes.
> >
> > v2 - 1. Moved few changes to previous patch.
> >
> > .../fusa/reqs/design-reqs/arm64/hypercall.rst | 15 ++++
> > .../design-reqs/arm64/version_hypercall.rst   | 34 ++++++++
> > .../reqs/design-reqs/version_hypercall.rst    | 82 ++++++++++++++++++
> > docs/fusa/reqs/index.rst                      |  3 +
> > docs/fusa/reqs/product-reqs/hypercall.rst     | 20 +++++
> > .../reqs/product-reqs/version_hypercall.rst   | 83 +++++++++++++++++++
> > 6 files changed, 237 insertions(+)
> > create mode 100644 docs/fusa/reqs/design-reqs/arm64/version_hypercall.rst
> > create mode 100644 docs/fusa/reqs/design-reqs/version_hypercall.rst
> > create mode 100644 docs/fusa/reqs/product-reqs/hypercall.rst
> >
> > diff --git a/docs/fusa/reqs/design-reqs/arm64/hypercall.rst 
> > b/docs/fusa/reqs/design-reqs/arm64/hypercall.rst
> > index f00b0b00f9..f58a9d50aa 100644
> > --- a/docs/fusa/reqs/design-reqs/arm64/hypercall.rst
> > +++ b/docs/fusa/reqs/design-reqs/arm64/hypercall.rst
> > @@ -56,3 +56,18 @@ Comments:
> > Covers:
> >   - `XenProd~version_hyp_first_param~1`
> >   - `XenProd~version_hyp_second_param~1`
> > +
> > +Return value
> > +------------
> > +
> > +`XenSwdgn~arm64_ret_val~1`
> > +
> > +Description:
> > +Xen shall store the return value in first cpu core register.
> use x0 instead.
Ack
> > +
> > +Rationale:
> > +
> > +Comments:
> > +
> > +Covers:
> > + - `XenProd~hyp_err_ret_val~1`
> > diff --git a/docs/fusa/reqs/design-reqs/arm64/version_hypercall.rst 
> > b/docs/fusa/reqs/design-reqs/arm64/version_hypercall.rst
> > new file mode 100644
> > index 0000000000..3aa12ea2c2
> > --- /dev/null
> > +++ b/docs/fusa/reqs/design-reqs/arm64/version_hypercall.rst
> > @@ -0,0 +1,34 @@
> > +.. SPDX-License-Identifier: CC-BY-4.0
> > +
> > +Capabilities
> > +------------
> > +
> > +`XenSwdgn~arm64_capabilities~1`
> > +
> > +Description:
> > +Xen shall have an internal constant string to denote that the cpu is running
> > +in arm64 mode.
> This is untestable if this is purely internal so this cannot be a requirement
> I am not quite sure why you need this, can you explain ?
We had agreed on this wording in "[PATCH v1 2/2] docs: fusa: Add the 
requirements for some of the commands of XEN_VERSION". You had mentioned 
this
```

All in all, the design just need to say that it must be stored somewhere 
"bounded" to the source code so that a tester can check it.

```

So, I infer that needs to be validated by code inspection. The string is 
set in init_xen_cap_info().
> > +
> > +Rationale:
> > +
> > +Comments:
> > +
> > +Covers:
> > + - `XenProd~version_hyp_capabilities_cmd~1`
> > +
> > +Capabilities AArch32
> > +--------------------
> > +
> > +`XenSwdgn~arm64_capabilities_aarch32~1`
> > +
> > +Description:
> > +Xen shall have a internal constant string to denote that the cpu is running in
> > +arm32 mode.
> > +
> > +Rationale:
> > +
> > +Comments:
> > +
> > +Covers:
> > + - `XenProd~version_hyp_capabilities_cmd~1`
> > +
> > diff --git a/docs/fusa/reqs/design-reqs/version_hypercall.rst 
> > b/docs/fusa/reqs/design-reqs/version_hypercall.rst
> > new file mode 100644
> > index 0000000000..aac5896965
> > --- /dev/null
> > +++ b/docs/fusa/reqs/design-reqs/version_hypercall.rst
> > @@ -0,0 +1,82 @@
> > +.. SPDX-License-Identifier: CC-BY-4.0
> > +
> > +Version
> > +-------
> > +
> > +`XenSwdgn~version~1`
> > +
> > +Description:
> > +Xen shall have a internal constant (XEN_VERSION) storing the version number
> > +coming from the Makefile.
> I really have doubts about this and the following one.
>
> If this only goal is to say what should be returned in the XEN_VERSION hypcall 
> you might
> just need something saying it and mention this as a comment because you will 
> have a very
> hard time to test such a requirement.
Should I say

Description:

Xen shall return its version when XENVER_version command is invoked.

If so , then ...

> > +
> > +Rationale:
> > +
> > +Comments:
> > +
> > +Covers:
> > + - `XenProd~version_hyp_version_cmd~1`
> > +
> > +Subversion
> > +----------
> > +
> > +`XenSwdgn~subversion~1`
> > +
> > +Description:
> > +Xen shall have a internal constant (XEN_SUBVERSION) storing the sub version
> > +number coming from the Makefile.
Xen shall return its extra version when XENVER_extraversion command is 
invoked.
> > +
> > +Rationale:
> > +
> > +Comments:
> > +
> > +Covers:
> > + - `XenProd~version_hyp_version_cmd~1`
> > +
> > +Error copying buffer
> > +--------------------
> > +
> > +`XenSwdgn~error_copy_buffer~1`
> > +
> > +Description:
> > +Xen shall return -EFAULT if it is not able to copy data to domain's buffer.
> > +
> > +Rationale:
> > +-EFAULT is one of the error code defined in
> > +http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/errno.h.
> > +
> > +Comments:
> > +
> > +Covers:
> > + - `XenProd~hyp_err_ret_val~1`
> > +
> > +Extraversion
> > +------------
> > +
> > +`XenSwdgn~extraversion~1`
> > +
> > +Description:
> > +Xen shall have a internal constant (XEN_EXTRAVERSION) storing the extraversion
> > +coming from the build environment.
> > +
> > +Rationale:
> > +
> > +Comments:
> > +
> > +Covers:
> > + - `XenProd~version_hyp_extraversion_cmd~1`
> > +
> > +Changeset
> > +---------
> > +
> > +`XenSwdgn~changeset~1`
> > +
> > +Description:
> > +Xen shall have a internal constant string (XEN_CHANGESET) storing the date,
> > +time and git hash of the last change made to Xen's codebase.
> > +
> > +Rationale:
> > +
> > +Comments:
> > +
> > +Covers:
> > + - `XenProd~version_hyp_changeset_cmd~1`
> > diff --git a/docs/fusa/reqs/index.rst b/docs/fusa/reqs/index.rst
> > index d8683edce7..de19b0cda2 100644
> > --- a/docs/fusa/reqs/index.rst
> > +++ b/docs/fusa/reqs/index.rst
> > @@ -11,6 +11,9 @@ Requirements documentation
> >     product-reqs/reqs
> >     product-reqs/arm64/reqs
> >     product-reqs/version_hypercall
> > +   product-reqs/hypercall
> >     design-reqs/arm64/generic-timer
> >     design-reqs/arm64/sbsa-uart
> >     design-reqs/arm64/hypercall
> > +   design-reqs/arm64/version_hypercall
> > +   design-reqs/version_hypercall
> > diff --git a/docs/fusa/reqs/product-reqs/hypercall.rst 
> > b/docs/fusa/reqs/product-reqs/hypercall.rst
> > new file mode 100644
> > index 0000000000..b57b9acde8
> > --- /dev/null
> > +++ b/docs/fusa/reqs/product-reqs/hypercall.rst
> > @@ -0,0 +1,20 @@
> > +.. SPDX-License-Identifier: CC-BY-4.0
> > +
> > +Error Return Value
> > +------------------
> > +
> > +`XenProd~hyp_err_ret_val~1`
> > +
> > +Description:
> > +In case the hypercall fails, Xen shall return one of the error codes defined
> > +in 
> > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/errno.h.
> s/the/an/ hypercall otherwise it is not quite clear which hypercall you mean.
Ack
> > +
> > +Rationale:
> > +
> > +Comments:
> > +
> > +Covers:
> > + - `XenMkt~version_hypercall~1`
> > +
> > +Needs:
> > + - XenSwdgn
> > diff --git a/docs/fusa/reqs/product-reqs/version_hypercall.rst 
> > b/docs/fusa/reqs/product-reqs/version_hypercall.rst
> > index 400d51bbeb..2ef1c4f9ca 100644
> > --- a/docs/fusa/reqs/product-reqs/version_hypercall.rst
> > +++ b/docs/fusa/reqs/product-reqs/version_hypercall.rst
> > @@ -41,3 +41,86 @@ Covers:
> >
> > Needs:
> >   - XenSwdgn
> > +
> > +Version command
> > +---------------
> > +
> > +`XenProd~version_hyp_version_cmd~1`
> > +
> > +Description:
> > +Xen shall provide a command (num 0) for  hypercall (num 17) to retrieve Xen's
> > +version in the domain's register 0.
> > +
> > +Rationale:
> > +
> > +Comments:
> > +Xen version is composed of major and minor number.
> How Xen version is encoded should be a requirement, you can add this directly 
> into
> the definition of this one in fact i think.
Do you mean the description should be :-

Description:

Xen shall provide a command (num 0) for  hypercall (num 17) to retrieve Xen's
version (composed of major and minor number) in the domain's register 0.

> > +
> > +Covers:
> > + - `XenMkt~version_hypercall~1`
> > +
> > +Needs:
> > + - XenSwdgn
> > +
> > +Extraversion command
> > +--------------------
> > +
> > +`XenProd~version_hyp_extraversion_cmd~1`
> > +
> > +Description:
> > +Xen shall provide a command (num 1) for hypercall (num 17) to copy its
> > +extraversion in the domain's buffer.
> > +
> > +Rationale:
> > +
> > +Comments:
> > +Xen's extra version consists of a string passed with 'XEN_VENDORVERSION' 
> > command
> > +line parameter while building Xen.
> > +
> > +Covers:
> > + - `XenMkt~version_hypercall~1`
> > +
> > +Needs:
> > + - XenSwdgn
> > +
> > +Capabilities command
> > +--------------------
> > +
> > +`XenProd~version_hyp_capabilities_cmd~1`
> > +
> > +Description:
> > +Xen shall provide a command (num 3) for hypercall (num 17) to copy its
> > +capabilities to the domain's buffer.
> > +
> > +Rationale:
> > +
> > +Comments:
> > +Capabilities related information is represented by char[1024].
> > +For Arm64, the capabilities should contain "xen-3.0-aarch64" string.
> Isn't it mandatory for xen to return aarch64 for arm64 ?
> This could be turned into a requirement (easily testable).
So should I change the following design requirement

"Xen shall have an internal constant string to denote that the cpu is 
running in arm64 mode."
to

"Xen shall return xen-3.0-aarch64 to denote that the cpu is running in 
arm64 mode."
> > +
> > +Covers:
> > + - `XenMkt~version_hypercall~1`
> > +
> > +Needs:
> > + - XenSwdgn
> > +
> > +Changeset command
> > +-----------------
> > +
> > +`XenProd~version_hyp_changeset_cmd~1`
> > +
> > +Description:
> > +Xen shall provide a command (num 4) for hypercall (num 17) to copy changeset
> > +to the domain's buffer.
> > +
> > +Rationale:
> > +
> > +Comments:
> > +Changeset is string denoting the date, time and git hash of the last change
> > +made to Xen's codebase.
> Does this has a standard format ? if so it should be explained.
This is a implementation level detail. The format is autogenerated in 
compile.h
"Mon Dec 30 22:00:31 2024 +0100 git:0d729221ab"

How do you want me explain this ?

- Ayan

> > +
> > +Covers:
> > + - `XenMkt~version_hypercall~1`
> > +
> > +Needs:
> > + - XenSwdgn
> > --
> > 2.25.1
> Cheers
> Bertrand