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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 28/10/15 17:55, Konrad Rzeszutek Wilk wrote:
> 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.

The current interface the xen_version hypercall is mad.  It has the same
shortcoming as the C library function gets().

It is unfortunate that our current debug register-clobbering strategy
prevents altering the number of parameters in a forwards compatible.

There are two options to move forwards:
1) Introduce a new hypercall and relegate the existing one to being a
_compat.
2) Change our debug clobbering strategy.

We definitely dont any further contritions to the school of "pass a
pointer without a length and trust the other side".

I think it might be prudent to follow option 1), as that also allows to
fix other issues such as the arbitrary (and unreasonable IMO) 1k
restriction on the length of the hypervisor command line.

~Andrew