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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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.

> > +
> > +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) });

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

- Ayan

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