Re: [PATCH v3 2/6] x86/vmware: Introduce vmware_hypercall API

[Alexey Makhalov]

On 12/19/23 4:51 PM, kirill.shutemov@xxxxxxxxxxxxxxx wrote:
> On Tue, Dec 19, 2023 at 04:17:40PM -0800, Alexey Makhalov wrote:
> > On 12/19/23 3:20 PM, kirill.shutemov@xxxxxxxxxxxxxxx wrote:
> > > On Tue, Dec 19, 2023 at 01:57:47PM -0800, Alexey Makhalov wrote:
> > > > +static inline
> > > > +unsigned long vmware_hypercall1(unsigned long cmd, unsigned long in1)
> > > ...
> > > > +static inline
> > > > +unsigned long vmware_hypercall3(unsigned long cmd, unsigned long in1,
> > > > +				uint32_t *out1, uint32_t *out2)
> > > ...
> > > > +static inline
> > > > +unsigned long vmware_hypercall4(unsigned long cmd, unsigned long in1,
> > > > +				uint32_t *out1, uint32_t *out2,
> > > > +				uint32_t *out3)
> > > ...
> > > > +static inline
> > > > +unsigned long vmware_hypercall5(unsigned long cmd, unsigned long in1,
> > > > +				unsigned long in3, unsigned long in4,
> > > > +				unsigned long in5, uint32_t *out2)
> > > ...
> > > > +static inline
> > > > +unsigned long vmware_hypercall6(unsigned long cmd, unsigned long in1,
> > > > +				unsigned long in3, uint32_t *out2,
> > > > +				uint32_t *out3, uint32_t *out4,
> > > > +				uint32_t *out5)
> > > ...
> > > > +static inline
> > > > +unsigned long vmware_hypercall7(unsigned long cmd, unsigned long in1,
> > > > +				unsigned long in3, unsigned long in4,
> > > > +				unsigned long in5, uint32_t *out1,
> > > > +				uint32_t *out2, uint32_t *out3)
> > >
> > > Naming is weird. The number in the name doesn't help much as there seems
> > > no system on how many of the parameters are ins and outs.
> >
> > There was internal discussion on hypercall API naming. One of proposals was
> > using 2 digits - number of input and number of output arguments.
> > And it definitely looked weird. So, we agreed to have just single number  -
> > total number of arguments excluding cmd.
>
> Have you considered naming them by number of input parameters? Number of
> output parameters as demanded by users.
>
> So vmware_hypercall4() will become vmware_hypercall1() and current
> vmware_hypercall1() and vmware_hypercall3() will go away.
>
> It is still awful, but /maybe/ better that this, I donno.

Deprecating vmware_hypercall1 and vmware_hypercall3 in favor of 
vmware_hypercall4 will generate less efficient code for the caller of 
first ones.
Using current vmware_hypercall4 instead of vmware_hypercall1 will force 
the caller to allocate additional variables (register or on stack 
memory) for hypercall asm inline to put additional output registers on. 
And specifically to 'usage' of *out3 - compiler will unnecessary 
'clobber' useful rdx, when hypervisor will keep it unchanged.

Unfortunately VMware hypercall ABI is not as beautiful as KVM one, 
especially in number of output arguments and their ordering. rbp 
register usage as an argument is a separate bummer((. So we have to work 
with what we have.

Current set of functions includes only 6 functions (for LB), which is 
the optimum between readability, maintainability and performance. It 
covers all current kernel callers and all new callers from yet to be 
upstreamed patches that we have in Photon OS including 2 patches for x86 
and arm64 guest support.

Regards,
--Alexey