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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.10.15 at 19:34, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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".

While I partly disagree with this one in the context here (there are no
pointers being passed without known length) ...

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

... I agree that this would be a proper route forward. However, I
can't see any downside to the less involved alternative Konrad
suggests, perhaps even without any extra indirection:

struct xen_build_id_t {
        uint32_t        len; /* IN: size of buf[]. */
        unsigned char buf[1]; /* OUT: Variable length buffer with build_id. */
}

That approach could then also be used for new sub-ops deprecating
ones where the current fixed size is an issue.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel