Hi Ayan,
> On 3 Jun 2025, at 12:15, Ayan Kumar Halder <ayank...@amd.com> 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.hal...@amd.com>
>>> wrote:
>>>
>>> Define the requirements which are common for all the commands for
>>> XEN_VERSION
>>> hypercall.
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.hal...@amd.com>
>>> ---
>>> 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
