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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Ayan,

Sorry for the delay.

> On 3 Jun 2025, at 18:11, Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
> 
> 
> 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().

ok at the end this might be tested by inspecting the image ro data section.

> 
>> 
>>> +
>>> +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.

yes that sounds better.

> 
> 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.

yes

>>> +
>>> +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.

If you want to test that the version is what you expect you need to know how it 
is encoded.
So might be something like major number in bits x to y, minor in xxx etc.

> 
>> 
>>> +
>>> +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."

yes

> 
>> 
>>> +
>>> +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 ?

No then the sentence is ok as is i would say.

Cheers
Bertrand

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