Re: [PATCH for-4.18 v2 2/2] x86: Clarify that only 5 hypercall parameters are supported

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Fri, Oct 6, 2023 at 4:29 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> From: Michal Orzel <michal.orzel@xxxxxxx>
>
> The x86 hypercall ABI really used to have 6-argument hypercalls.  V4V, the
> downstream predecessor to Argo did take 6th args.
>
> However, the 6th arg being %ebp in the 32bit ABI makes it unusable in
> practice, because that's the frame pointer in builds with frame pointers
> enabled.  Therefore Argo was altered to being a 5-arg hypercall when it was
> upstreamed.
>
> c/s 2f531c122e95 ("x86: limit number of hypercall parameters to 5") removed
> the ability for hypercalls to take 6 arguments.
>
> Update the documentation to match reality.
>
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Michal Orzel <michal.orzel@xxxxxxx>
> CC: Henry Wang <Henry.Wang@xxxxxxx>
>
> v2:
>  * Extend with the historical context of how 6-arg hypercalls have come and 
> gone.
> ---
>  docs/guest-guide/x86/hypercall-abi.rst   | 15 +++++++++++----
>  xen/include/public/arch-x86/xen-x86_32.h |  2 +-
>  xen/include/public/arch-x86/xen-x86_64.h |  2 +-
>  3 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/docs/guest-guide/x86/hypercall-abi.rst 
> b/docs/guest-guide/x86/hypercall-abi.rst
> index 42a820386b68..c7a11a76712f 100644
> --- a/docs/guest-guide/x86/hypercall-abi.rst
> +++ b/docs/guest-guide/x86/hypercall-abi.rst

> @@ -119,6 +119,13 @@ means.
>  .. [#kern] For HVM guests, ``HVMOP_guest_request_vm_event`` may be configured
>     to be usable from userspace, but this behaviour is not default.
>
> +.. [#params] Xen's ABI used to declare support for 6 hypercall arguments,
> +   using ``r9`` and ``ebp``.  However, such an ABI clobbers the frame pointer
> +   in the 32bit code and does interact nicely with guest-side debugging.  The

I think you want s/does/does not/.

With that,

Reviewed-by: Jason Andryuk <jandryuk@xxxxxxxxx>

> +   predecessor to ``HYPERCALL_argo_op`` was a 6-argument hypercall, but the

Also, I think it would be worth just naming v4v with "...predecessor
to ``HYPERCALL_argo_op``, v4v, was...", so a future reader doesn't
have to investigate to find out what the predecessor was.

Regards,
Jason

> +   ABI was intentionally altered when Argo was upstreamed (Xen 4.13) to be 
> the
> +   5-argument hypercall it now is.
> +
>  .. [#mode] While it is possible to use compatibility mode segments in a 64bit
>     kernel, hypercalls issues from such a mode will be interpreted with the
>     32bit ABI.  Such a setup is not expected in production scenarios.