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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Wed, Oct 28, 2015 at 06:34:37PM +0000, Andrew Cooper 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".
> 
> 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.

<sigh> Yak shaving..

If we are going that route may I suggest another option (less
animal interaction):

3) Stick XENVER_build_id under the xSplice hypercall?


> 
> ~Andrew

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