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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 28/02/2025 07:54, Bertrand Marquis wrote:
> Hi Ayan,
Hi Bertrand,
> > On 27 Feb 2025, at 20:29, Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
> >
> >
> > On 27/02/2025 17:15, Bertrand Marquis wrote:
> > > Hi Ayan,
> > Hi Bertrand,
> >
> > I have just some clarifications.
> >
> > > > On 27 Feb 2025, at 16:09, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> wrote:
> > > >
> > > > In the current patch, we have defined the requirements which are common for
> > > > all the commands.
> > > >
> > > > 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).
> > > >
> > > > .../fusa/reqs/design-reqs/arm64/hypercall.rst | 55 +++++++++++++++++
> > > > docs/fusa/reqs/index.rst                      |  2 +
> > > > docs/fusa/reqs/market-reqs/reqs.rst           | 16 +++++
> > > > .../reqs/product-reqs/version_hypercall.rst   | 61 +++++++++++++++++++
> > > > 4 files changed, 134 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..ffd883260c
> > > > --- /dev/null
> > > > +++ b/docs/fusa/reqs/design-reqs/arm64/hypercall.rst
> > > > @@ -0,0 +1,55 @@
> > > > +.. SPDX-License-Identifier: CC-BY-4.0
> > > > +
> > > > +Hypercall
> > > > +=========
> > > > +
> > > > +Instruction
> > > > +-----------
> > > > +
> > > > +`XenSwdgn~arm64_hyp_instr~1`
> > > > +
> > > > +Description:
> > > > +Xen shall treat domain hypercall exception as hypercall requests.
> > > > +
> > > > +Rationale:
> > > > +
> > > > +Comments:
> > > > +Hypercall is one of the communication mechanism between Xen and domains.
> > > > +Domains use hypercalls for various requests to Xen.
> > > > +Domains use 'hvc' 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 x0 to read the first parameter, x1 for second parameter and so
> > > > +on, for domain hypercall requests.
> > > > +
> > > > +Rationale:
> > > > +
> > > > +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 x0 register.
> > > > +
> > > > +Rationale:
> > > > +
> > > > +Comments:
> > > > +
> > > > +Covers:
> > > > + - `XenProd~version_hyp_ret_val~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.
> > > > +
> > > > +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..03221f70c3
> > > > --- /dev/null
> > > > +++ b/docs/fusa/reqs/product-reqs/version_hypercall.rst
> > > > @@ -0,0 +1,61 @@
> > > > +.. 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.
> > > You speak of argument here and parameter earlier.
> > > I would rephrase to: the first argument of an hypercall exception as the 
> > > command number for the hypercall.
> > Ack
> >
> > Should I do this everywhere ?
> >
> > s/parameter/argument
> >
> > That would make it consistent.
> Yes definitely
>
> > > > +
> > > > +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 to buffer in domain's
> > > > +memory.
> > > > +
> > > Same here on argument vs parameter.
> > >
> > > > +Rationale:
> > > > +
> > > > +Comments:
> > > > +
> > > > +Covers:
> > > > + - `XenMkt~version_hypercall~1`
> > > > +
> > > > +Needs:
> > > > + - XenSwdgn
> > > > +
> > > > +Return Value
> > > > +------------
> > > > +
> > > > +`XenProd~version_hyp_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.
> > > This is a very imprecise req as it does not states what can fail and what 
> > > should be returned exactly.
> > > Do we want to be that generic ? if yes then this might be a requirement valid 
> > > for any hypercall.
> > Yes, you are correct.
> >
> > I am thinking of pushing this further up ie have this requirement at market 
> > level and leave it **unlinked** to any product requirement.
> >
> > IOW, we don't need to validate each and every error code returned by the 
> > hypercall.
> >
> > Or should we just drop this requirement ?
> I think you should move this requirement and make it a generic one valid for 
> all hypercalls.
Yes, I will place it under hypercall.rst.
> Now at some point you might have to describe which error is caused by what 
> problem as part of your hypercall interface definition.
> ie: one generic product req and per hypercall design req describing the error 
> cases.
Agreed.

And an example design requirement which will be linked is :-

Xen shall return -EFAULT if it encounters an error while copying the 
extraversion string to domain's buffer.
Is this what you have in mind ?

> At the end you should have a test to trigger each error condition and that test 
> will be linked to the design req corresponding.
Ack.

- Ayan

> Cheers
> Bertrand
>
> > - Ayan
> >
> > > Cheers
> > > Bertrand
> > >
> > > > +
> > > > +Rationale:
> > > > +
> > > > +Comments:
> > > > +
> > > > +Covers:
> > > > + - `XenMkt~version_hypercall~1`
> > > > +
> > > > +Needs:
> > > > + - XenSwdgn
> > > > \ No newline at end of file
> > > > --
> > > > 2.25.1