Re: [PATCH v3 1/2] docs: fusa: Define the requirements for XEN_VERSION hypercall.

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Ayan,

> On 3 Jun 2025, at 12:15, Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
> 
> 
> On 21/05/2025 13:02, Bertrand Marquis wrote:
>> Hi Ayan,
> 
> Hi Bertrand,
> 
>> 
>>> On 9 May 2025, at 13:24, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> 
>>> wrote:
>>> 
>>> Define the requirements which are common for all the commands for 
>>> XEN_VERSION
>>> hypercall.
>>> 
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
>>> ---
>>> Changes from -
>>> 
>>> v1 - 1. Fixed `XenProd~version_hyp_ret_val~1` requirement as Xen does not 
>>> return
>>> 0 for success in all the cases.
>>> 2. Reworded the requirements so as to write them from Xen's perspective (not
>>> domain's perspective).
>>> 
>>> v2 - 1. Specified the register details.
>>> 2. Specified the type of buffer.
>>> 
>>> .../fusa/reqs/design-reqs/arm64/hypercall.rst | 58 +++++++++++++++++++
>>> docs/fusa/reqs/index.rst                      |  2 +
>>> docs/fusa/reqs/market-reqs/reqs.rst           | 16 +++++
>>> .../reqs/product-reqs/version_hypercall.rst   | 43 ++++++++++++++
>>> 4 files changed, 119 insertions(+)
>>> create mode 100644 docs/fusa/reqs/design-reqs/arm64/hypercall.rst
>>> create mode 100644 docs/fusa/reqs/product-reqs/version_hypercall.rst
>>> 
>>> diff --git a/docs/fusa/reqs/design-reqs/arm64/hypercall.rst 
>>> b/docs/fusa/reqs/design-reqs/arm64/hypercall.rst
>>> new file mode 100644
>>> index 0000000000..f00b0b00f9
>>> --- /dev/null
>>> +++ b/docs/fusa/reqs/design-reqs/arm64/hypercall.rst
>>> @@ -0,0 +1,58 @@
>>> +.. SPDX-License-Identifier: CC-BY-4.0
>>> +
>>> +Hypercall
>>> +=========
>>> +
>>> +Instruction
>>> +-----------
>>> +
>>> +`XenSwdgn~arm64_hyp_instr~1`
>>> +
>>> +Description:
>>> +Xen shall treat domain hypercall exception as hypercall requests.
>> This really reads weirdly.
>> Maybe: Xen shall treat domain hvc instruction execution as hypercall 
>> requests.
>> 
>> Then you can add a comment to explain that this is detected through a 
>> specific exception.
>> 
>> Also this is not completely true as hvc is also used in other scenarios:
>> - PSCI calls
>> - Firmware calls
>> 
>> So i would put the 0xEA1 value as part of the requirement.
> 
> Description:
> 
> Xen shall treat domain hvc instruction execution (with 0xEA1) as hypercall 
> requests.
> 
> Comments:
> The exception syndrome register should have the following values :-
> ESR_EL2.ISS should be 0xEA1.
> ESR_EL2.EC should be 0x16.

yes a lot better :-)

> 
>> 
>>> +
>>> +Rationale:
>>> +
>>> +Comments:
>>> +Hypercall is one of the communication mechanism between Xen and domains.
>>> +Domains use hypercalls for various requests to Xen.
>>> +Domains use 'hvc #0xEA1' instruction to invoke hypercalls.
>>> +
>>> +Covers:
>>> + - `XenProd~version_hyp_first_param~1`
>>> + - `XenProd~version_hyp_second_param~1`
>>> +
>>> +Parameters
>>> +----------
>>> +
>>> +`XenSwdgn~arm64_hyp_param~1`
>>> +
>>> +Description:
>>> +Xen shall use the first five cpu core registers to obtain the arguments for
>>> +domain hypercall requests.
>> Why not say x0 to x4 registers instead ? You use x16 after anyway
> Ack
>> 
>>> +
>>> +Rationale:
>>> +
>>> +Comments:
>>> +Xen shall read the first register for the first argument, second register 
>>> for
>>> +the second argument and so on.
>>> +
>>> +Covers:
>>> + - `XenProd~version_hyp_first_param~1`
>>> + - `XenProd~version_hyp_second_param~1`
>>> +
>>> +Hypercall number
>>> +----------------
>>> +
>>> +`XenSwdgn~arm64_hyp_num~1`
>>> +
>>> +Description:
>>> +Xen shall read x16 to obtain the hypercall number.
>>> +
>>> +Rationale:
>>> +
>>> +Comments:
>>> +
>>> +Covers:
>>> + - `XenProd~version_hyp_first_param~1`
>>> + - `XenProd~version_hyp_second_param~1`
>>> diff --git a/docs/fusa/reqs/index.rst b/docs/fusa/reqs/index.rst
>>> index 1088a51d52..d8683edce7 100644
>>> --- a/docs/fusa/reqs/index.rst
>>> +++ b/docs/fusa/reqs/index.rst
>>> @@ -10,5 +10,7 @@ Requirements documentation
>>>    market-reqs/reqs
>>>    product-reqs/reqs
>>>    product-reqs/arm64/reqs
>>> +   product-reqs/version_hypercall
>>>    design-reqs/arm64/generic-timer
>>>    design-reqs/arm64/sbsa-uart
>>> +   design-reqs/arm64/hypercall
>>> diff --git a/docs/fusa/reqs/market-reqs/reqs.rst 
>>> b/docs/fusa/reqs/market-reqs/reqs.rst
>>> index 2d297ecc13..0e29fe5362 100644
>>> --- a/docs/fusa/reqs/market-reqs/reqs.rst
>>> +++ b/docs/fusa/reqs/market-reqs/reqs.rst
>>> @@ -79,3 +79,19 @@ Comments:
>>> 
>>> Needs:
>>>  - XenProd
>>> +
>>> +Version hypercall
>>> +-----------------
>>> +
>>> +`XenMkt~version_hypercall~1`
>>> +
>>> +Description:
>>> +Xen shall provide an interface for the domains to retrieve Xen's version, 
>>> type
>>> +and compilation information.
>> I would say hypercall instead of interface here
> Ack
>> 
>>> +
>>> +Rationale:
>>> +
>>> +Comments:
>>> +
>>> +Needs:
>>> + - XenProd
>>> diff --git a/docs/fusa/reqs/product-reqs/version_hypercall.rst 
>>> b/docs/fusa/reqs/product-reqs/version_hypercall.rst
>>> new file mode 100644
>>> index 0000000000..400d51bbeb
>>> --- /dev/null
>>> +++ b/docs/fusa/reqs/product-reqs/version_hypercall.rst
>>> @@ -0,0 +1,43 @@
>>> +.. SPDX-License-Identifier: CC-BY-4.0
>>> +
>>> +Version hypercall
>>> +=================
>>> +
>>> +First Parameter
>>> +---------------
>>> +
>>> +`XenProd~version_hyp_first_param~1`
>>> +
>>> +Description:
>>> +Xen shall treat the first argument (as an integer) to denote the command 
>>> number
>>> +for the hypercall.
>> I would be more precise and say x0 value.
> Ack
>> 
>> Also "integer" is unspecified, use a more specific type definition (32/64 
>> bit signed/unsigned integer).
> 
> It is a signed integer. The size is not mentioned.
> 
> ret = do_xen_version((int)(a1), (XEN_GUEST_HANDLE_PARAM(void)){ _p(a2) });

This is compiler dependent and architecture dependent so sounds weird.
I would expect this to be int32_t.
Maybe something to check here.

> 
>> 
>>> +
>>> +Rationale:
>>> +
>>> +Comments:
>>> +
>>> +Covers:
>>> + - `XenMkt~version_hypercall~1`
>>> +
>>> +Needs:
>>> + - XenSwdgn
>>> +
>>> +Second Parameter
>>> +----------------
>>> +
>>> +`XenProd~version_hyp_second_param~1`
>>> +
>>> +Description:
>>> +Xen shall treat the second argument as a virtual address (mapped as Normal
>>> +Inner Write-Back Outer Write-Back Inner-Shareable) to buffer in domain's
>>> +memory.
>> You should say "guest virtual address" to be more precise.
> 
> Ack
> 
> s/guest/domain
> 

yes

Cheers
Bertrand

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