Re: [PATCH v1 2/4] xen-version: Add third parameter (len) to the do_version hypercall.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Oct 09, 2015 at 08:38:34AM -0600, Jan Beulich wrote:
> >>> On 09.10.15 at 14:46, <ian.campbell@citrix.com> wrote:
> > On Fri, 2015-10-09 at 13:29 +0100, Andrew Cooper wrote:
> >> On 09/10/15 09:25, Jan Beulich wrote:
> >> > > > > On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
> >> > > All existing commands ignore the parameter so this does
> >> > > not break the ABI.
> >> > Does it not? What about the debug mode clobbering of hypercall
> >> > argument registers?
> >> 
> >> That is an implementation detail of the hypervisor.  It is irrelevant to
> >> guests whether Xen chooses to clobber the spare registers or not.
> > 
> > Or in other words the effect here is to clobber one _less_ register, and
> > the guest cannot have been relying on a register getting so clobbered (if
> > nothing else it doesn't happen in debug=n builds).
> 
> No, the one less register clobbered is in the first clobbering phase,
> where _unused_ inputs get clobbered (for hypervisor internal
> consumption). The second clobbering phase destroys all _used_
> input registers' contents (the guest visible values), and _this_ is
> what results in ABI breakage (because callers assuming the
> hypercall to take two arguments assume that the 3rd argument
> register will retain its contents.

Thanks for explaining it! I see my patch missed the change to
hypercall_table and along with compiling with debug=y it all worked
so I didn't get the 'len' parameter to be clobbered.

Ugh.

Then the right way to make this work would be to only clobber
the third argument if the XENVER_buildid command was used? And
preserve the third argument only if XENVER_buildid command was used.

And not clobber third argument in all other cases. That would
require some nasty tweaking of entry.S.

Ugh. I think going to the original idea of just having an
xen_build_id_t[1024] would be the easiest.

Or I can do a structure:

struct xen_build_id_t {
	uint32_t	len; /* IN: size of the buffer. */
	uint32_t 	_pad; /* IN: MUST be zero. */
	XEN_GUEST_HANDLE_64(char) buf; /* OUT: Buffer with build_id. */
}

Any preference? The 'xen_build_id_t[1024]' looks nicer.
> 
> Jan
>