Re: [PATCH 3/5] libxencall: introduce variant of xencall2() returning long

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.06.2021 15:46, Andrew Cooper wrote:
> On 18/06/2021 11:24, Jan Beulich wrote:
>> Some hypercalls, memory-op in particular, can return values requiring
>> more than 31 bits to represent. Hence the underlying layers need to make
>> sure they won't truncate such values.
>>
>> While adding the new function to the map file, I noticed the stray
>> xencall6 there. I'm taking the liberty to remove it at this occasion. If
>> such a function would ever appear, it shouldn't lane in version 1.0.
> 
> s/lane/land/ ?

Yeah, spotted this already.

> I'm tempted to suggest spitting this out into a separate change anyway. 
> I'm not sure of the implications on the ABI.

There are none, as a non-existing symbol can't possibly appear in a
DSO's symbol table. But well, yes, I can certainly make this a
separate change; it merely seemed excessive to me because of the
no-op effect the change has at this point in time.

> ABI-dumper appears not to have picked anything up, nor has readelf on
> the object itself, so we're probably ok ABI wise.
> 
> That said, I would really have expected a compile/link error for a bad
> symbol in a map file.

So would I, but reality tells us otherwise.

>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> I wasn't sure whether euqivalents for the other xencall<N>() should also
>> be introduced, and hence went for the minimal solution first. Otoh there
>> is also xencall0() which has no users ...
>>
>> --- a/tools/include/xencall.h
>> +++ b/tools/include/xencall.h
>> @@ -113,6 +113,10 @@ int xencall5(xencall_handle *xcall, unsi
>>               uint64_t arg1, uint64_t arg2, uint64_t arg3,
>>               uint64_t arg4, uint64_t arg5);
>>  
>> +/* Variant(s) of the above, as needed, returning "long" instead of "int". */
>> +long xencall2L(xencall_handle *xcall, unsigned int op,
> 
> If we're fixing ABIs, can we see about not truncate op on the way up?

You mean making it unsigned long, when I don't see us ever
gathering enough hypercalls. Even if it were flags to add in, they
would surely fit in the low 32 bits. I'm afraid there's too much
code out there assuming the hypercall numbers can be passed in the
low half of a 64-bit register.

But of course, if you insist ...

>> --- a/tools/libs/call/core.c
>> +++ b/tools/libs/call/core.c
>> @@ -127,6 +127,17 @@ int xencall2(xencall_handle *xcall, unsi
>>      return osdep_hypercall(xcall, &call);
>>  }
>>  
>> +long xencall2L(xencall_handle *xcall, unsigned int op,
>> +               uint64_t arg1, uint64_t arg2)
>> +{
>> +    privcmd_hypercall_t call = {
>> +        .op = op,
>> +        .arg = { arg1, arg2 },
>> +    };
>> +
>> +    return osdep_hypercall(xcall, &call);
> 
> (If we're not changing op), I take it there are no alias tricks we can
> play to reuse the same implementation?

Re-use would only be possible if we knew that all psABI-s match up
wrt the treatment of a "long" value becoming the return value of
a function returning "int". An ABI might require sign-extension to
register width (leaving aside yet more exotic options). Then, yes,
the "int" returning one(s) could become alias(es) of the "long"
returning ones.

Jan