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

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

Jun 18, 2021, 3:24 AM

Post #1 of 4 (3 views)

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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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,
+ uint64_t arg1, uint64_t arg2);
+
/*
* Allocate and free memory which is suitable for use as a pointer
* argument to a hypercall.
--- 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);
+}
+
int xencall3(xencall_handle *xcall, unsigned int op,
uint64_t arg1, uint64_t arg2, uint64_t arg3)
{
--- a/tools/libs/call/libxencall.map
+++ b/tools/libs/call/libxencall.map
@@ -9,7 +9,6 @@ VERS_1.0 {
xencall3;
xencall4;
xencall5;
- xencall6;

xencall_alloc_buffer;
xencall_free_buffer;
@@ -27,3 +26,8 @@ VERS_1.2 {
global:
xencall_fd;
} VERS_1.1;
+
+VERS_1.3 {
+ global:
+ xencall2L;
+} VERS_1.2;

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

andrew.cooper3 at citrix

Jun 18, 2021, 6:46 AM

Post #2 of 4 (3 views)

Permalink

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/ ?

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

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.

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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?

> + uint64_t arg1, uint64_t arg2);
> +
> /*
> * Allocate and free memory which is suitable for use as a pointer
> * argument to a hypercall.
> --- 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?

~Andrew

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

jbeulich at suse

Jun 18, 2021, 8:03 AM

Post #3 of 4 (3 views)

Permalink

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@suse.com>
>> ---
>> 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

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

iwj at xenproject

Jun 22, 2021, 4:38 AM

Post #4 of 4 (3 views)

Permalink

Jan Beulich writes ("[PATCH 3/5] libxencall: introduce variant of xencall2() returning long"):
> 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.

Thanks for this.

All 5 patches:

Acked-by: Ian Jackson <iwj@xenproject.org>

Nit:

> 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.
^^^^

Typo for "land", I think.

Ian.