Message ID | c7f93b66-bc4d-708a-6936-e0eac9e36cfa@suse.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | allow xc_domain_maximum_gpfn() to observe full GFN value | expand |
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
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
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.
--- 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;
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 ...