| Message ID | 20160324212905.GA9589@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 24.03.16 at 22:30, <konrad@kernel.org> wrote: > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Date: Tue, 22 Mar 2016 16:53:19 -0400 > Subject: [PATCH] HYPERCALL_version_op. New hypercall mirroring XENVER_ but > sane. > > This hypercall mirrors the XENVER_ in that it has similar functionality. > However it is designed differently: > - No compat layer. The data structures are the same size on 32 > as on 64-bit. > - The hypercall accepts three arguments - the command, pointer to > an buffer, and the length of the buffer. > - Each sub-ops can be "probed" for size by returning the size of > buffer that will be needed - if the buffer is NULL. > - Subops can complete even if the buffer is too small - truncated > data will be filled and hypercall will return -ENOBUFS. > - VERSION_commandline, VERSION_changeset are privileged. > - There is no XENVER_compile_info equivalent. > - The hypercall can return -EPERM and toolstack/OSes are expected > to deal with. However there are three subops: XEN_VERSION_version, > XEN_VERSION_platform_parameters and XEN_VERSION_get_features > that will always return an value as guests cannot survive without them. > > While we combine some of the common code between XENVER_ and VERSION_ > take the liberty of moving pae_extended_cr3 in x86 area. > > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> [XSM bits] > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> This last line doesn't seem to match up with ... > v1-v3: Was not part of the series. > v4: New posting. > v5: Remove memset and use {}. Tweak copy_to_guest and capabilities_info, > add ASSERT(sz) per Andrew's review. Add cached=1 back in. > Per Jan, s/VERSION_OP/VERSION/, squash size check with do_version_op, > update the comments. Dropped Andrew's Review-by. Ate newlines. ... the middle sentence here. > +DO(version_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg, > + unsigned int len) > +{ > + union { > + xen_version_op_val_t val; > + xen_feature_info_t fi; > + } u = {}; > + unsigned int sz = 0; > + const void *ptr = NULL; > + int rc = xsm_version_op(XSM_OTHER, cmd); > + > + /* We can safely return -EPERM! */ So what again was this comment supposed to tell us? > + if ( rc ) > + return rc; > + > + /* > + * The HYPERVISOR_xen_version differs in that some return the value, I think there's a "subop" missing somewhere. > +static int __init capabilities_cache_init(void) > +{ > + /* > + * Pre-allocate the cache so we do not have to worry about "Pre-fill" or "Pre-populate"? > --- a/xen/include/public/version.h > +++ b/xen/include/public/version.h > @@ -30,7 +30,14 @@ > > #include "xen.h" > > -/* NB. All ops return zero on success, except XENVER_{version,pagesize} */ Perhaps this comment doesn't belong here anymore, but I don't think it should be deleted altogether. > +/* > + * There are two hypercalls mentioned in here. The XENVER_ are for > + * HYPERCALL_xen_version (17), while VERSION_ are for the XEN_VERSION_ > @@ -87,6 +94,67 @@ typedef struct xen_feature_info xen_feature_info_t; > #define XENVER_commandline 9 > typedef char xen_commandline_t[1024]; > > +/* > + * The HYPERCALL_version_op has a set of sub-ops which mirror the > + * sub-ops of HYPERCALL_xen_version. However this hypercall differs > + * radically from the former: > + * - It returns the amount of bytes returned. "returns ... returned" is a little strange. Perhaps "returns ... copied"? > + * - It will return -XEN_EPERM if the guest is not permitted > + * (Albeit XEN_VERSION_version, XEN_VERSION_platform_parameters, and > + * XEN_VERSION_get_features will always return an value as guest cannot > + * survive without this information). Isn't there an object missing here, to say what is not permitted? > + * - It will return the requested data in arg. > + * - It requires an third argument (len) for the length of the > + * arg. Naturally the arg has to fit the requested data otherwise > + * -XEN_ENOBUFS is returned. > + * > + * It also offers an mechanism to probe for the amount of bytes an ... a mechanism ... > + * sub-op will require. Having the arg have an NULL handle will ... a NULL ... > + * return the number of bytes requested for the operation. Or an > + * negative value if an error is encountered. ... a negative ... Since they're all cosmetic, if you take care of all of them, feel free to stick my ack on the result. Jan
>>> On 30.03.16 at 17:43, <JBeulich@suse.com> wrote: > Since they're all cosmetic, if you take care of all of them, feel free > to stick my ack on the result. Actually - no, please don't. While the patch is fine content wise then from my perspective, I'm still lacking a convincing argument of why this new hypercall is needed in the first place. If others are convinced by the argumentation between (mostly, iirc) you and Andrew, I'm not going to stand in the way, but I'm also not going to approve of the code addition without being myself convinced. Jan
On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: > >>> On 30.03.16 at 17:43, <JBeulich@suse.com> wrote: > > Since they're all cosmetic, if you take care of all of them, feel free > > to stick my ack on the result. > > Actually - no, please don't. While the patch is fine content wise > then from my perspective, I'm still lacking a convincing argument > of why this new hypercall is needed in the first place. If others > are convinced by the argumentation between (mostly, iirc) you > and Andrew, I'm not going to stand in the way, but I'm also not > going to approve of the code addition without being myself > convinced. Damm. I pushed the patch in yesterday in 'staging'! We can always revert them.. "Others" being other maintainers I presume? The underlaying reason for me doing this is to expose the build-id. It (build-id) originally was in sysctl, then folks wanted it in XENVER_. Got it working in there as sub-ops, but Andrew last minute decided that it should not really be there but in a new hypercall that can do three arguments (the length) and be able to return -EPERM. A sane one, not the cobbled up XENVER one.
>>> On 31.03.16 at 13:43, <konrad@kernel.org> wrote: > On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: >> >>> On 30.03.16 at 17:43, <JBeulich@suse.com> wrote: >> > Since they're all cosmetic, if you take care of all of them, feel free >> > to stick my ack on the result. >> >> Actually - no, please don't. While the patch is fine content wise >> then from my perspective, I'm still lacking a convincing argument >> of why this new hypercall is needed in the first place. If others >> are convinced by the argumentation between (mostly, iirc) you >> and Andrew, I'm not going to stand in the way, but I'm also not >> going to approve of the code addition without being myself >> convinced. > > Damm. I pushed the patch in yesterday in 'staging'! > > We can always revert them.. > > "Others" being other maintainers I presume? Any one of the REST maintainers, yes. > The underlaying reason for me doing this is to expose the build-id. > > It (build-id) originally was in sysctl, then folks wanted it in XENVER_. > Got it working in there as sub-ops, but Andrew last minute decided that > it should not really be there but in a new hypercall that can do > three arguments (the length) and be able to return -EPERM. A sane > one, not the cobbled up XENVER one. Well, -EPERM is now possible with the old one too. And nothing in that existing interface prevents a length to be passed in/out for new sub-ops. Nor do I really see anything truly insane with that existing interface. Jan
On Thu, Mar 31, 2016 at 06:07:58AM -0600, Jan Beulich wrote: > >>> On 31.03.16 at 13:43, <konrad@kernel.org> wrote: > > On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: > >> >>> On 30.03.16 at 17:43, <JBeulich@suse.com> wrote: > >> > Since they're all cosmetic, if you take care of all of them, feel free > >> > to stick my ack on the result. > >> > >> Actually - no, please don't. While the patch is fine content wise > >> then from my perspective, I'm still lacking a convincing argument > >> of why this new hypercall is needed in the first place. If others > >> are convinced by the argumentation between (mostly, iirc) you > >> and Andrew, I'm not going to stand in the way, but I'm also not > >> going to approve of the code addition without being myself > >> convinced. > > > > Damm. I pushed the patch in yesterday in 'staging'! > > > > We can always revert them.. > > > > "Others" being other maintainers I presume? > > Any one of the REST maintainers, yes. Changing the title to get their attention. > > > The underlaying reason for me doing this is to expose the build-id. > > > > It (build-id) originally was in sysctl, then folks wanted it in XENVER_. > > Got it working in there as sub-ops, but Andrew last minute decided that Here is the link to v3 which had it in XENVER_ subops: http://lists.xen.org/archives/html/xen-devel/2016-02/msg04110.html And this one v5 (in case folks had deleted this thread, v4 is almost the same except it had VERSION instead of XEN_VERSION): http://lists.xen.org/archives/html/xen-devel/2016-03/msg03302.html > > it should not really be there but in a new hypercall that can do > > three arguments (the length) and be able to return -EPERM. A sane > > one, not the cobbled up XENVER one. > > Well, -EPERM is now possible with the old one too. And nothing > in that existing interface prevents a length to be passed in/out > for new sub-ops. Nor do I really see anything truly insane with We cannot expand the hypercall to have three arguments - it MUST have two (as you had pointed out earlier). The length must be jammed in the sub-ops: /* Return value is the number of bytes written, or XEN_Exx on error. * Calling with empty parameter returns the size of build_id. */ #define XENVER_build_id 10 struct xen_build_id { uint32_t len; /* IN: size of buf[]. */ #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L unsigned char buf[]; #elif defined(__GNUC__) unsigned char buf[1]; /* OUT: Variable length buffer with build_id. */ #endif }; typedef struct xen_build_id xen_build_id_t; > that existing interface. > > Jan >
>>> On 31.03.16 at 15:28, <konrad.wilk@oracle.com> wrote: > On Thu, Mar 31, 2016 at 06:07:58AM -0600, Jan Beulich wrote: >> >>> On 31.03.16 at 13:43, <konrad@kernel.org> wrote: >> > it should not really be there but in a new hypercall that can do >> > three arguments (the length) and be able to return -EPERM. A sane >> > one, not the cobbled up XENVER one. >> >> Well, -EPERM is now possible with the old one too. And nothing >> in that existing interface prevents a length to be passed in/out >> for new sub-ops. Nor do I really see anything truly insane with > > We cannot expand the hypercall to have three arguments - it MUST > have two (as you had pointed out earlier). The length must be jammed > in the sub-ops: Right, that's what I had implied. Jan > /* Return value is the number of bytes written, or XEN_Exx on error. > * Calling with empty parameter returns the size of build_id. */ > > #define XENVER_build_id 10 > struct xen_build_id { > uint32_t len; /* IN: size of buf[]. */ > #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L > unsigned char buf[]; > #elif defined(__GNUC__) > unsigned char buf[1]; /* OUT: Variable length buffer with > build_id. */ > #endif > }; > typedef struct xen_build_id xen_build_id_t; > >> that existing interface. >> >> Jan >>
>>> On 31.03.16 at 15:28, <konrad.wilk@oracle.com> wrote: > On Thu, Mar 31, 2016 at 06:07:58AM -0600, Jan Beulich wrote: >> >>> On 31.03.16 at 13:43, <konrad@kernel.org> wrote: >> > On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: >> >> >>> On 30.03.16 at 17:43, <JBeulich@suse.com> wrote: >> >> > Since they're all cosmetic, if you take care of all of them, feel free >> >> > to stick my ack on the result. >> >> >> >> Actually - no, please don't. While the patch is fine content wise >> >> then from my perspective, I'm still lacking a convincing argument >> >> of why this new hypercall is needed in the first place. If others >> >> are convinced by the argumentation between (mostly, iirc) you >> >> and Andrew, I'm not going to stand in the way, but I'm also not >> >> going to approve of the code addition without being myself >> >> convinced. >> > >> > Damm. I pushed the patch in yesterday in 'staging'! >> > >> > We can always revert them.. >> > >> > "Others" being other maintainers I presume? >> >> Any one of the REST maintainers, yes. > > Changing the title to get their attention. Yet nothing has happened, so I think the patch needs to be reverted (at least for the time being). Jan
On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote: > >>> On 31.03.16 at 15:28, <konrad.wilk@oracle.com> wrote: > > On Thu, Mar 31, 2016 at 06:07:58AM -0600, Jan Beulich wrote: > >> >>> On 31.03.16 at 13:43, <konrad@kernel.org> wrote: > >> > On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: > >> >> >>> On 30.03.16 at 17:43, <JBeulich@suse.com> wrote: > >> >> > Since they're all cosmetic, if you take care of all of them, feel free > >> >> > to stick my ack on the result. > >> >> > >> >> Actually - no, please don't. While the patch is fine content wise > >> >> then from my perspective, I'm still lacking a convincing argument > >> >> of why this new hypercall is needed in the first place. If others > >> >> are convinced by the argumentation between (mostly, iirc) you > >> >> and Andrew, I'm not going to stand in the way, but I'm also not > >> >> going to approve of the code addition without being myself > >> >> convinced. > >> > > >> > Damm. I pushed the patch in yesterday in 'staging'! > >> > > >> > We can always revert them.. > >> > > >> > "Others" being other maintainers I presume? > >> > >> Any one of the REST maintainers, yes. > > > > Changing the title to get their attention. > > Yet nothing has happened, so I think the patch needs to be > reverted (at least for the time being). Wait what?! The natural consensus mechanism we use is lazy. If nobody objects then it is Acked. Anyhow pinged Ian Jackson on IRC. > > Jan >
>>> On 08.04.16 at 19:09, <konrad.wilk@oracle.com> wrote: > On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote: >> >>> On 31.03.16 at 15:28, <konrad.wilk@oracle.com> wrote: >> > On Thu, Mar 31, 2016 at 06:07:58AM -0600, Jan Beulich wrote: >> >> >>> On 31.03.16 at 13:43, <konrad@kernel.org> wrote: >> >> > On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: >> >> >> >>> On 30.03.16 at 17:43, <JBeulich@suse.com> wrote: >> >> >> > Since they're all cosmetic, if you take care of all of them, feel free >> >> >> > to stick my ack on the result. >> >> >> >> >> >> Actually - no, please don't. While the patch is fine content wise >> >> >> then from my perspective, I'm still lacking a convincing argument >> >> >> of why this new hypercall is needed in the first place. If others >> >> >> are convinced by the argumentation between (mostly, iirc) you >> >> >> and Andrew, I'm not going to stand in the way, but I'm also not >> >> >> going to approve of the code addition without being myself >> >> >> convinced. >> >> > >> >> > Damm. I pushed the patch in yesterday in 'staging'! >> >> > >> >> > We can always revert them.. >> >> > >> >> > "Others" being other maintainers I presume? >> >> >> >> Any one of the REST maintainers, yes. >> > >> > Changing the title to get their attention. >> >> Yet nothing has happened, so I think the patch needs to be >> reverted (at least for the time being). > > Wait what?! > > The natural consensus mechanism we use is lazy. If nobody > objects then it is Acked. Since when can patches go in without any ack(s) of relevant maintainers? Jan
Konrad Rzeszutek Wilk writes ("Re: REST MAINTAINERS feedback requested Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."):
> On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote:
> > Yet nothing has happened, so I think the patch needs to be
> > reverted (at least for the time being).
>
> Wait what?!
I'm sorry that I didn't understand that we were being asked for a
second opinion about this disagreement. I'm afriad that the original
email wasn't really comprehensible to me as a summary of the
disagreement.
Would someone please summarise ? Especially, since Jan is AFAICT
saying that this new hypercall is not needed, it would be helpful to
know why those who think it is needed want it.
In the meantime I think it would be best to avoid churn by leaving the
patch in tree for now. I promise that I won't let those "facts on the
ground" influence my views about whether this hypercall is needed.
Thanks,
Ian.
On Fri, Apr 08, 2016 at 11:13:08AM -0600, Jan Beulich wrote: > >>> On 08.04.16 at 19:09, <konrad.wilk@oracle.com> wrote: > > On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote: > >> >>> On 31.03.16 at 15:28, <konrad.wilk@oracle.com> wrote: > >> > On Thu, Mar 31, 2016 at 06:07:58AM -0600, Jan Beulich wrote: > >> >> >>> On 31.03.16 at 13:43, <konrad@kernel.org> wrote: > >> >> > On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: > >> >> >> >>> On 30.03.16 at 17:43, <JBeulich@suse.com> wrote: > >> >> >> > Since they're all cosmetic, if you take care of all of them, feel free > >> >> >> > to stick my ack on the result. > >> >> >> > >> >> >> Actually - no, please don't. While the patch is fine content wise > >> >> >> then from my perspective, I'm still lacking a convincing argument > >> >> >> of why this new hypercall is needed in the first place. If others > >> >> >> are convinced by the argumentation between (mostly, iirc) you > >> >> >> and Andrew, I'm not going to stand in the way, but I'm also not > >> >> >> going to approve of the code addition without being myself > >> >> >> convinced. > >> >> > > >> >> > Damm. I pushed the patch in yesterday in 'staging'! > >> >> > > >> >> > We can always revert them.. > >> >> > > >> >> > "Others" being other maintainers I presume? > >> >> > >> >> Any one of the REST maintainers, yes. > >> > > >> > Changing the title to get their attention. > >> > >> Yet nothing has happened, so I think the patch needs to be > >> reverted (at least for the time being). > > > > Wait what?! > > > > The natural consensus mechanism we use is lazy. If nobody > > objects then it is Acked. > > Since when can patches go in without any ack(s) of relevant > maintainers? > Urgh, at the risk of pointing out the obvious -- it does seem to have your ack... Wei. > Jan >
On Fri, Apr 08, 2016 at 06:21:27PM +0100, Wei Liu wrote: > On Fri, Apr 08, 2016 at 11:13:08AM -0600, Jan Beulich wrote: > > >>> On 08.04.16 at 19:09, <konrad.wilk@oracle.com> wrote: > > > On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote: > > >> >>> On 31.03.16 at 15:28, <konrad.wilk@oracle.com> wrote: > > >> > On Thu, Mar 31, 2016 at 06:07:58AM -0600, Jan Beulich wrote: > > >> >> >>> On 31.03.16 at 13:43, <konrad@kernel.org> wrote: > > >> >> > On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: > > >> >> >> >>> On 30.03.16 at 17:43, <JBeulich@suse.com> wrote: > > >> >> >> > Since they're all cosmetic, if you take care of all of them, feel free > > >> >> >> > to stick my ack on the result. > > >> >> >> > > >> >> >> Actually - no, please don't. While the patch is fine content wise > > >> >> >> then from my perspective, I'm still lacking a convincing argument > > >> >> >> of why this new hypercall is needed in the first place. If others > > >> >> >> are convinced by the argumentation between (mostly, iirc) you > > >> >> >> and Andrew, I'm not going to stand in the way, but I'm also not > > >> >> >> going to approve of the code addition without being myself > > >> >> >> convinced. > > >> >> > > > >> >> > Damm. I pushed the patch in yesterday in 'staging'! > > >> >> > > > >> >> > We can always revert them.. > > >> >> > > > >> >> > "Others" being other maintainers I presume? > > >> >> > > >> >> Any one of the REST maintainers, yes. > > >> > > > >> > Changing the title to get their attention. > > >> > > >> Yet nothing has happened, so I think the patch needs to be > > >> reverted (at least for the time being). > > > > > > Wait what?! > > > > > > The natural consensus mechanism we use is lazy. If nobody > > > objects then it is Acked. > > > > Since when can patches go in without any ack(s) of relevant > > maintainers? > > > > Urgh, at the risk of pointing out the obvious -- it does seem to have > your ack... Which was given at night before Jan retracted it in the morning. This feels like one of those Catch-22 :-)
On Thu, Mar 31, 2016 at 7:30 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 30.03.16 at 17:43, <JBeulich@suse.com> wrote: >> Since they're all cosmetic, if you take care of all of them, feel free >> to stick my ack on the result. > > Actually - no, please don't. While the patch is fine content wise > then from my perspective, I'm still lacking a convincing argument > of why this new hypercall is needed in the first place. If others > are convinced by the argumentation between (mostly, iirc) you > and Andrew, I'm not going to stand in the way, but I'm also not > going to approve of the code addition without being myself > convinced. I don't see in this patch a justification for why Konrad (and/or Andrew) think the new version is needed, nor do I see in this particular thread why Jan thinks it's not necessary, so I don't really know what's going on. I'm happy to give my opinion of someone wants to catch me up. -George
On Fri, Apr 08, 2016 at 01:23:23PM -0400, Konrad Rzeszutek Wilk wrote: > On Fri, Apr 08, 2016 at 06:21:27PM +0100, Wei Liu wrote: > > On Fri, Apr 08, 2016 at 11:13:08AM -0600, Jan Beulich wrote: > > > >>> On 08.04.16 at 19:09, <konrad.wilk@oracle.com> wrote: > > > > On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote: > > > >> >>> On 31.03.16 at 15:28, <konrad.wilk@oracle.com> wrote: > > > >> > On Thu, Mar 31, 2016 at 06:07:58AM -0600, Jan Beulich wrote: > > > >> >> >>> On 31.03.16 at 13:43, <konrad@kernel.org> wrote: > > > >> >> > On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: > > > >> >> >> >>> On 30.03.16 at 17:43, <JBeulich@suse.com> wrote: > > > >> >> >> > Since they're all cosmetic, if you take care of all of them, feel free > > > >> >> >> > to stick my ack on the result. > > > >> >> >> > > > >> >> >> Actually - no, please don't. While the patch is fine content wise > > > >> >> >> then from my perspective, I'm still lacking a convincing argument > > > >> >> >> of why this new hypercall is needed in the first place. If others > > > >> >> >> are convinced by the argumentation between (mostly, iirc) you > > > >> >> >> and Andrew, I'm not going to stand in the way, but I'm also not > > > >> >> >> going to approve of the code addition without being myself > > > >> >> >> convinced. > > > >> >> > > > > >> >> > Damm. I pushed the patch in yesterday in 'staging'! > > > >> >> > > > > >> >> > We can always revert them.. > > > >> >> > > > > >> >> > "Others" being other maintainers I presume? > > > >> >> > > > >> >> Any one of the REST maintainers, yes. > > > >> > > > > >> > Changing the title to get their attention. > > > >> > > > >> Yet nothing has happened, so I think the patch needs to be > > > >> reverted (at least for the time being). > > > > > > > > Wait what?! > > > > > > > > The natural consensus mechanism we use is lazy. If nobody > > > > objects then it is Acked. > > > > > > Since when can patches go in without any ack(s) of relevant > > > maintainers? > > > > > > > Urgh, at the risk of pointing out the obvious -- it does seem to have > > your ack... > > Which was given at night before Jan retracted it in the morning. > > This feels like one of those Catch-22 :-) Ah, OK then, so Jan has a point. This needs to be properly discussed and refereed. Wei.
>>> On 08.04.16 at 19:24, <george.dunlap@citrix.com> wrote: > On Thu, Mar 31, 2016 at 7:30 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 30.03.16 at 17:43, <JBeulich@suse.com> wrote: >>> Since they're all cosmetic, if you take care of all of them, feel free >>> to stick my ack on the result. >> >> Actually - no, please don't. While the patch is fine content wise >> then from my perspective, I'm still lacking a convincing argument >> of why this new hypercall is needed in the first place. If others >> are convinced by the argumentation between (mostly, iirc) you >> and Andrew, I'm not going to stand in the way, but I'm also not >> going to approve of the code addition without being myself >> convinced. > > I don't see in this patch a justification for why Konrad (and/or > Andrew) think the new version is needed, nor do I see in this > particular thread why Jan thinks it's not necessary, so I don't really > know what's going on. I'm happy to give my opinion of someone wants > to catch me up. Well, the hypercall is redundant with an existing one, and the semantics needed for adding the build-id sub-op don't really require this new hypercall either. So it not adding new functionality, I think it simply needs to be demonstrated that the new variant is needed, not that it's not needed. Jan
On 08/04/16 18:21, Ian Jackson wrote: > Konrad Rzeszutek Wilk writes ("Re: REST MAINTAINERS feedback requested Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."): >> On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote: >>> Yet nothing has happened, so I think the patch needs to be >>> reverted (at least for the time being). >> Wait what?! > I'm sorry that I didn't understand that we were being asked for a > second opinion about this disagreement. I'm afriad that the original > email wasn't really comprehensible to me as a summary of the > disagreement. > > Would someone please summarise ? Especially, since Jan is AFAICT > saying that this new hypercall is not needed, it would be helpful to > know why those who think it is needed want it. The new hypercall is very definitely needed, which is why I requested it during earlier revisions of the xsplice series. The interface for the old version was sufficiently useless that build_id can't be added to it. (Specifically, there is no ability to return varialble length data). Also, by its design, it has some unreasonably-short limits on extraversion and changesetinfo, both of which could do with being longer for distros trying to encode "delta from upstream" information. The new hypercall has a ration interface where you don't blindly trust that the caller passed you a pointer to a suitably-sized structure. I am very much +10 keep to the patch. ~Andrew
>>> On 08.04.16 at 19:41, <andrew.cooper3@citrix.com> wrote: > On 08/04/16 18:21, Ian Jackson wrote: >> Konrad Rzeszutek Wilk writes ("Re: REST MAINTAINERS feedback requested > Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall > mirroring XENVER_ but sane."): >>> On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote: >>>> Yet nothing has happened, so I think the patch needs to be >>>> reverted (at least for the time being). >>> Wait what?! >> I'm sorry that I didn't understand that we were being asked for a >> second opinion about this disagreement. I'm afriad that the original >> email wasn't really comprehensible to me as a summary of the >> disagreement. >> >> Would someone please summarise ? Especially, since Jan is AFAICT >> saying that this new hypercall is not needed, it would be helpful to >> know why those who think it is needed want it. > > The new hypercall is very definitely needed, which is why I requested it > during earlier revisions of the xsplice series. > > The interface for the old version was sufficiently useless that build_id > can't be added to it. (Specifically, there is no ability to return > varialble length data). This is simply not true: The hypercall being passed a void handle, everything can be arranged for without introducing a new hypercall. > Also, by its design, it has some > unreasonably-short limits on extraversion and changesetinfo, both of > which could do with being longer for distros trying to encode "delta > from upstream" information. > > The new hypercall has a ration interface where you don't blindly trust > that the caller passed you a pointer to a suitably-sized structure. While the new one is indeed slightly neater, that's not sufficient for such redundancy imo. That's the whole reason for withdrawing my ack _without_ making it an explicit NAK. Jan
Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."):
> On 08.04.16 at 19:41, <andrew.cooper3@citrix.com> wrote:
> > The interface for the old version was sufficiently useless that build_id
> > can't be added to it. (Specifically, there is no ability to return
> > varialble length data).
>
> This is simply not true: The hypercall being passed a void handle,
> everything can be arranged for without introducing a new
> hypercall.
I'm trying to understand what your alternative suggestion is. Could
you please be more explicit ?
I'm reading xen/include/public/version.h. (Sadly it doesn't have the
API doc markup.) AFAICT there is a XENVER_xxxx sub-op namespace which
permits extensions to the XENVER hypercall.
But does that permit the caller to specify their buffer size ?
> > The new hypercall has a ration interface where you don't blindly trust
> > that the caller passed you a pointer to a suitably-sized structure.
Ie I think ^ this is for me a key question.
> While the new one is indeed slightly neater, that's not sufficient
> for such redundancy imo. That's the whole reason for withdrawing
> my ack _without_ making it an explicit NAK.
I don't think I would be content with simply adding a new sub-op with
bigger fixed-length fields.
Ian.
On Mon, Apr 11, 2016 at 11:50:25AM +0100, Ian Jackson wrote: > Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."): > > On 08.04.16 at 19:41, <andrew.cooper3@citrix.com> wrote: > > > The interface for the old version was sufficiently useless that build_id > > > can't be added to it. (Specifically, there is no ability to return > > > varialble length data). > > > > This is simply not true: The hypercall being passed a void handle, > > everything can be arranged for without introducing a new > > hypercall. > > I'm trying to understand what your alternative suggestion is. Could > you please be more explicit ? > > I'm reading xen/include/public/version.h. (Sadly it doesn't have the > API doc markup.) AFAICT there is a XENVER_xxxx sub-op namespace which > permits extensions to the XENVER hypercall. > > But does that permit the caller to specify their buffer size ? Only via the sub-op parameters itself. As in you cannot expand the amount of parameters the XENVER_hypercall can do (which is two - subops number and the pointer to arguments). > > > > The new hypercall has a ration interface where you don't blindly trust > > > that the caller passed you a pointer to a suitably-sized structure. > > Ie I think ^ this is for me a key question. > > > While the new one is indeed slightly neater, that's not sufficient > > for such redundancy imo. That's the whole reason for withdrawing > > my ack _without_ making it an explicit NAK. > > I don't think I would be content with simply adding a new sub-op with > bigger fixed-length fields. It was variable-ish. The new-subop (xsplice.v3) ended up 'lets-try-this-size' subop. Meaning: /* Return value is the number of bytes written, or XEN_Exx on error. * Calling with empty parameter returns the size of build_id. */ #define XENVER_build_id 10 struct xen_build_id { uint32_t len; /* IN: size of buf[]. */ #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L unsigned char buf[]; #elif defined(__GNUC__) unsigned char buf[1]; /* OUT: Variable length buffer with build_id. */ #endif }; When constructing this in libxl we could do: xen_build_id_t *build_id; ret= xc_version(ctx->xch, XENVER_build_id, NULL); if ( ret != -ENOSYS && ret > 0 ) build_id = libxl__zalloc(gc, ret + sizeof(*build_id)); build_id->len = info->pagesize - sizeof(*build_id); ret= xc_version(ctx->xch, XENVER_build_id, build_id); .. parse it... } For the default case, ret would be 20, which meant the structure had to be 24 bytes. 4 bytes were for 'len' (which had the value of 20) and the rest inside the build_id were to be filled with the hypervisor. For glory details: http://xenbits.xen.org/gitweb/?p=people/konradwilk/xen.git;a=commitdiff;h=63681414ede6e38c1910077d7a225e0d67f0ff2e;hp=37efc2ac64b9ecf0cd49fb65aa7c7659467c9318 and http://xenbits.xen.org/gitweb/?p=people/konradwilk/xen.git;a=commitdiff;h=176c63f3c0db430c70c28fe81cb8d039ae459c66;hp=63681414ede6e38c1910077d7a225e0d67f0ff2e (albeit we first tried with the default 1020 bytes we have on the stack).
Konrad Rzeszutek Wilk writes ("Re: REST MAINTAINERS feedback requested Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."):
> On Mon, Apr 11, 2016 at 11:50:25AM +0100, Ian Jackson wrote:
> > I don't think I would be content with simply adding a new sub-op with
> > bigger fixed-length fields.
>
> It was variable-ish.
...
> /* Return value is the number of bytes written, or XEN_Exx on error.
> * Calling with empty parameter returns the size of build_id. */
...
> #define XENVER_build_id 10
> struct xen_build_id {
> uint32_t len; /* IN: size of buf[]. */
> #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
> unsigned char buf[];
This is pretty ugly but tolerable.
The comment introducing the new HYPERCALL_version_op mentions some
other differences with HYPERCALL_xen_version, which seem to suggest
other deficiencies in the latter. Those deficiencies, together with
the ugliness of the above, would tend to suggest to me that a cleaner
new interface is warranted.
But to an extent some of this conversation seems to be on matters of
taste.
Jan, what is the downside of introducing a new hypercall ?
Ian.
>>> On 11.04.16 at 16:22, <Ian.Jackson@eu.citrix.com> wrote: > Konrad Rzeszutek Wilk writes ("Re: REST MAINTAINERS feedback requested > Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall > mirroring XENVER_ but sane."): >> On Mon, Apr 11, 2016 at 11:50:25AM +0100, Ian Jackson wrote: >> > I don't think I would be content with simply adding a new sub-op with >> > bigger fixed-length fields. >> >> It was variable-ish. > ... >> /* Return value is the number of bytes written, or XEN_Exx on error. >> * Calling with empty parameter returns the size of build_id. */ > ... >> #define XENVER_build_id 10 >> struct xen_build_id { >> uint32_t len; /* IN: size of buf[]. */ >> #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L >> unsigned char buf[]; > > This is pretty ugly but tolerable. > > The comment introducing the new HYPERCALL_version_op mentions some > other differences with HYPERCALL_xen_version, which seem to suggest > other deficiencies in the latter. Those deficiencies, together with > the ugliness of the above, would tend to suggest to me that a cleaner > new interface is warranted. > > But to an extent some of this conversation seems to be on matters of > taste. Agreed. > Jan, what is the downside of introducing a new hypercall ? Duplicate code effectively doing the same thing. Jan
Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."):
> On 11.04.16 at 16:22, <Ian.Jackson@eu.citrix.com> wrote:
> > But to an extent some of this conversation seems to be on matters of
> > taste.
>
> Agreed.
>
> > Jan, what is the downside of introducing a new hypercall ?
>
> Duplicate code effectively doing the same thing.
I agree that duplication is bad, all other things being equal.
But any improvement from an old API to a new one necessarily involves
providing a dual facility during a transition period.
I don't see an explicit deprecation in the patch that is in tree, but
it seems to me to be intended (and, perhaps, implied). Certainly if
we are going to permit these strings etc. to be bigger than fits in
the old hypercall, the old hypercall needs to be deprecated on the
grounds that it can provide incomplete or inaccurate information.
Does this way of looking at it help ?
Thanks,
Ian.
On Mon, Apr 11, 2016 at 05:25:04PM +0100, Ian Jackson wrote: > Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."): > > On 11.04.16 at 16:22, <Ian.Jackson@eu.citrix.com> wrote: > > > But to an extent some of this conversation seems to be on matters of > > > taste. > > > > Agreed. > > > > > Jan, what is the downside of introducing a new hypercall ? > > > > Duplicate code effectively doing the same thing. > > I agree that duplication is bad, all other things being equal. > > But any improvement from an old API to a new one necessarily involves > providing a dual facility during a transition period. > > I don't see an explicit deprecation in the patch that is in tree, but > it seems to me to be intended (and, perhaps, implied). Certainly if I tried it at some point by adding the suffix 'compat' to it. The compat layer did not like the extra compat string and all kinds of compilation issues arose. I put it on the backburner. > we are going to permit these strings etc. to be bigger than fits in > the old hypercall, the old hypercall needs to be deprecated on the > grounds that it can provide incomplete or inaccurate information. The build-id in Config.mk is set to use sha1. Which produces 20 bytes. You (or anybody else) can modify Config.mk to modify --build-id as per man ld (there is an uuid or md5 or): "0xhexstring" to use a chosen bit string specified as an even number of hexadecimal digits ("-" and ":" characters between digit pairs are ignored)." which does not impose any limits. Albeit 2967 characters of 0xdeadbeef is all I seem to be able jam on the line. Weird. Anyhow: build_id : deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeef0000deadbeefdeadbeef0000deadbeef0000deadbeef0000dead is possible. > > Does this way of looking at it help ? > > Thanks, > Ian.
>>> On 11.04.16 at 18:25, <Ian.Jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re: > [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring > XENVER_ but sane."): >> On 11.04.16 at 16:22, <Ian.Jackson@eu.citrix.com> wrote: >> > But to an extent some of this conversation seems to be on matters of >> > taste. >> >> Agreed. >> >> > Jan, what is the downside of introducing a new hypercall ? >> >> Duplicate code effectively doing the same thing. > > I agree that duplication is bad, all other things being equal. > > But any improvement from an old API to a new one necessarily involves > providing a dual facility during a transition period. Except that, at least for most sub-ops, the new one doesn't really provide much advantage, and hence dealing with the lack of size for those sub-ops where it matters within the existing hypercall (perhaps by adding one or two new sub-ops) would limit duplication quite a bit. > I don't see an explicit deprecation in the patch that is in tree, but > it seems to me to be intended (and, perhaps, implied). Certainly if > we are going to permit these strings etc. to be bigger than fits in > the old hypercall, the old hypercall needs to be deprecated on the > grounds that it can provide incomplete or inaccurate information. > > Does this way of looking at it help ? If that means you approve of the introduction of the new hypercall, yes. After all the goal of this whole discussion is to determine whether another maintainer is willing to provide a replacement ack for the withdrawn one. Jan
>>> On 11.04.16 at 18:53, <konrad.wilk@oracle.com> wrote: > On Mon, Apr 11, 2016 at 05:25:04PM +0100, Ian Jackson wrote: >> Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re: >> Certainly if >> we are going to permit these strings etc. to be bigger than fits in >> the old hypercall, the old hypercall needs to be deprecated on the >> grounds that it can provide incomplete or inaccurate information. > > The build-id in Config.mk is set to use sha1. Which produces 20 bytes. > You (or anybody else) can modify Config.mk to modify --build-id > as per man ld (there is an uuid or md5 or): > > "0xhexstring" to use a chosen bit string specified as an even number of > hexadecimal > digits ("-" and ":" characters between digit pairs are ignored)." > > which does not impose any limits. This has nothing to do with the discussion here, imo. The new build-id sub-op can, as said numerous times before, easily have a buffer length communicated. Jan
Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."):
> On 11.04.16 at 18:25, <Ian.Jackson@eu.citrix.com> wrote:
> > But any improvement from an old API to a new one necessarily involves
> > providing a dual facility during a transition period.
>
> Except that, at least for most sub-ops, the new one doesn't really
> provide much advantage, and hence dealing with the lack of size
> for those sub-ops where it matters within the existing hypercall
> (perhaps by adding one or two new sub-ops) would limit duplication
> quite a bit.
ISTM that the lower duplication (which in principle is an advantage
which will be time limited if we are ever able to completely remove
teh old hypercall) comes with the cost of (in the long term) increased
mess in this particular subop.
Is that right ? Obviously this cost is not very significant. But
maybe the duplication isn't that significant either. I was kind of
expecting to find stronger arguments on both sides in this
discussion...
Andrew, can you please confirm what you think of Jan's suggestion to
instead add a new sub-op where the sub-op structure contains a
guest-provided length field ? Are there other reasons to want to
introduce a new hypercall ?
> > I don't see an explicit deprecation in the patch that is in tree, but
> > it seems to me to be intended (and, perhaps, implied). Certainly if
> > we are going to permit these strings etc. to be bigger than fits in
> > the old hypercall, the old hypercall needs to be deprecated on the
> > grounds that it can provide incomplete or inaccurate information.
> >
> > Does this way of looking at it help ?
>
> If that means you approve of the introduction of the new hypercall,
> yes. After all the goal of this whole discussion is to determine
> whether another maintainer is willing to provide a replacement ack
> for the withdrawn one.
Err. Well, I am still asking questions. When I said "does that help"
I was meaning something like "does that help bring you closer to
agreement with Andrew". But I don't want to focus on procedure here.
If we're going to have the argument over this bike shed I'd like to
figure out what colour I myself prefer :-). But I need to know what
the reasons are behind people's preferences.
Ian.
>>> On 11.04.16 at 19:13, <Ian.Jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re: > [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring > XENVER_ but sane."): >> On 11.04.16 at 18:25, <Ian.Jackson@eu.citrix.com> wrote: >> > But any improvement from an old API to a new one necessarily involves >> > providing a dual facility during a transition period. >> >> Except that, at least for most sub-ops, the new one doesn't really >> provide much advantage, and hence dealing with the lack of size >> for those sub-ops where it matters within the existing hypercall >> (perhaps by adding one or two new sub-ops) would limit duplication >> quite a bit. > > ISTM that the lower duplication (which in principle is an advantage > which will be time limited if we are ever able to completely remove > teh old hypercall) comes with the cost of (in the long term) increased > mess in this particular subop. We cannot, ever, remove a not toolstack limited hypercall completely (or if, then only by Kconfig option so that people knowing none of the guests they care about use such deprecated ones). >> > I don't see an explicit deprecation in the patch that is in tree, but >> > it seems to me to be intended (and, perhaps, implied). Certainly if >> > we are going to permit these strings etc. to be bigger than fits in >> > the old hypercall, the old hypercall needs to be deprecated on the >> > grounds that it can provide incomplete or inaccurate information. >> > >> > Does this way of looking at it help ? >> >> If that means you approve of the introduction of the new hypercall, >> yes. After all the goal of this whole discussion is to determine >> whether another maintainer is willing to provide a replacement ack >> for the withdrawn one. > > Err. Well, I am still asking questions. When I said "does that help" > I was meaning something like "does that help bring you closer to > agreement with Andrew". But I don't want to focus on procedure here. Oh, well, in that case the answer is "no". Bike shed issue or not, having to maintain two pieces of code with similar functionality is undesirable. And it's not like we didn't have any XSA for that (apparently trivial) piece of code already. Jan
>>> On 11.04.16 at 19:13, <Ian.Jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re: > [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring > XENVER_ but sane."): >> On 11.04.16 at 18:25, <Ian.Jackson@eu.citrix.com> wrote: >> > But any improvement from an old API to a new one necessarily involves >> > providing a dual facility during a transition period. >> >> Except that, at least for most sub-ops, the new one doesn't really >> provide much advantage, and hence dealing with the lack of size >> for those sub-ops where it matters within the existing hypercall >> (perhaps by adding one or two new sub-ops) would limit duplication >> quite a bit. > > ISTM that the lower duplication (which in principle is an advantage > which will be time limited if we are ever able to completely remove > teh old hypercall) comes with the cost of (in the long term) increased > mess in this particular subop. > > Is that right ? Obviously this cost is not very significant. But > maybe the duplication isn't that significant either. I was kind of > expecting to find stronger arguments on both sides in this > discussion... If, btw, the cost of having to read the length argument from guest memory was deemed undesirable, we'd certainly have the option of specifying it to be passed through, say, the high half of the current "cmd" parameter. Jan
On Mon, Apr 11, 2016 at 6:46 PM, Jan Beulich <JBeulich@suse.com> wrote: [snip] >> ISTM that the lower duplication (which in principle is an advantage >> which will be time limited if we are ever able to completely remove >> teh old hypercall) comes with the cost of (in the long term) increased >> mess in this particular subop. > > We cannot, ever, remove a not toolstack limited hypercall completely > (or if, then only by Kconfig option so that people knowing none of the > guests they care about use such deprecated ones). We need to keep the hypercall around and functioning for ABI compatibility, but we could at least remove it from the public headers and point people to using the new one instead. (Discussion about the merit of this idea below.) [snip] >> ISTM that the lower duplication (which in principle is an advantage >> which will be time limited if we are ever able to completely remove >> teh old hypercall) comes with the cost of (in the long term) increased >> mess in this particular subop. >> >> Is that right ? Obviously this cost is not very significant. But >> maybe the duplication isn't that significant either. I was kind of >> expecting to find stronger arguments on both sides in this >> discussion... > > If, btw, the cost of having to read the length argument from guest > memory was deemed undesirable, we'd certainly have the option > of specifying it to be passed through, say, the high half of the > current "cmd" parameter. OK, so here are the options I see: 1. Use the existing hypercall with the existing call semantics for build-id -- i.e., require the caller to have a large but fixed-length buffer (maybe 1024 or 2048). 2. Use the existing hypercall but wedge in different calling semantics for the build-id subop. We could have just that subop pay attention to an extra argument as a length, for example, and return an error if the length is too short. Or we could make essentially a flag that asks, "How much space if I were to execute this subop for real?" 3. We could use a new hypercall only for new functionality, with the proposed new semantics. This would at minimum be build-id, but probably also extraversion, compileinfo, changeset, maybe capabilities_info. 4. Have the new hypercall replace the old hypercall. The new hypercall will duplicate all the functionality of the old hypercall. Deprecate the hypercall for a release or two, then remove it from the public headers (although keep the code, because we need to maintain backwards compatibility). Honestly I still don't quite understand what the problem is with #1 -- if build-id is mainly meant to be a UUID or a hash of the build to make sure that you're applying the right hotfix (please correct me if I'm wrong here -- I haven't had time to actually follow the patch series), 256 bytes should be enough for a properly hashed build, and 2048 should be more than enough. Requiring the caller to have a 2048-byte buffer before calling doesn't really seem like that much of a hardship to me. Basically: a. It's nicer to have arbitrary-length strings (2-4), but reasonably large fixed-length ones aren't awful (1) b. It's nicer for hypercall consumers to have a single hypercall with consistent semantics (1,4), but having two hypercalls (3) or a single one with inconsistent semantics (2) aren't that bad either. c. It's nicer for hypervisor maintainers to have a single place to support any given bit of functionality (1-3), but having a slightly duplicated functionality that only has to be supported in an ABI backwards-compatible manner isn't that bad either (4). This does seem to me an awful lot like a bike shed. :-) All of the options (1-4) seem perfectly fine to me. FWIW my preferred color would probably be 1 because it's the easiest and least inconsistent with the current state of things. My least favorite would be 3, because although each individual piece of information is only in one place, the path to get there is duplicated; both the kernel developer and the hypervisor developer are forced to continue to deal with both. -George
On Tue, Apr 12, 2016 at 10:58:09AM +0100, George Dunlap wrote: > On Mon, Apr 11, 2016 at 6:46 PM, Jan Beulich <JBeulich@suse.com> wrote: > [snip] > >> ISTM that the lower duplication (which in principle is an advantage > >> which will be time limited if we are ever able to completely remove > >> teh old hypercall) comes with the cost of (in the long term) increased > >> mess in this particular subop. > > > > We cannot, ever, remove a not toolstack limited hypercall completely > > (or if, then only by Kconfig option so that people knowing none of the > > guests they care about use such deprecated ones). > > We need to keep the hypercall around and functioning for ABI > compatibility, but we could at least remove it from the public headers > and point people to using the new one instead. (Discussion about the > merit of this idea below.) > > [snip] > >> ISTM that the lower duplication (which in principle is an advantage > >> which will be time limited if we are ever able to completely remove > >> teh old hypercall) comes with the cost of (in the long term) increased > >> mess in this particular subop. > >> > >> Is that right ? Obviously this cost is not very significant. But > >> maybe the duplication isn't that significant either. I was kind of > >> expecting to find stronger arguments on both sides in this > >> discussion... > > > > If, btw, the cost of having to read the length argument from guest > > memory was deemed undesirable, we'd certainly have the option > > of specifying it to be passed through, say, the high half of the > > current "cmd" parameter. > > OK, so here are the options I see: > > 1. Use the existing hypercall with the existing call semantics for > build-id -- i.e., require the caller to have a large but fixed-length > buffer (maybe 1024 or 2048). > > 2. Use the existing hypercall but wedge in different calling semantics > for the build-id subop. We could have just that subop pay attention > to an extra argument as a length, for example, and return an error if > the length is too short. Or we could make essentially a flag that > asks, "How much space if I were to execute this subop for real?" You can't wedge in the extra argument. Tried that an Jan pointed out that we clobber it (specifically we clobber any non-used arguments). > > 3. We could use a new hypercall only for new functionality, with the > proposed new semantics. This would at minimum be build-id, but > probably also extraversion, compileinfo, changeset, maybe > capabilities_info. > > 4. Have the new hypercall replace the old hypercall. The new > hypercall will duplicate all the functionality of the old hypercall. > Deprecate the hypercall for a release or two, then remove it from the > public headers (although keep the code, because we need to maintain > backwards compatibility). 5). Stick the build-id in the xsplice sysctl. Or just in the sysctl. > > Honestly I still don't quite understand what the problem is with #1 -- > if build-id is mainly meant to be a UUID or a hash of the build to > make sure that you're applying the right hotfix (please correct me if > I'm wrong here -- I haven't had time to actually follow the patch > series), 256 bytes should be enough for a properly hashed build, and > 2048 should be more than enough. Requiring the caller to have a > 2048-byte buffer before calling doesn't really seem like that much of > a hardship to me. Basically: > > a. It's nicer to have arbitrary-length strings (2-4), but reasonably > large fixed-length ones aren't awful (1) > > b. It's nicer for hypercall consumers to have a single hypercall with > consistent semantics (1,4), but having two hypercalls (3) or a single > one with inconsistent semantics (2) aren't that bad either. > > c. It's nicer for hypervisor maintainers to have a single place to > support any given bit of functionality (1-3), but having a slightly > duplicated functionality that only has to be supported in an ABI > backwards-compatible manner isn't that bad either (4). > > This does seem to me an awful lot like a bike shed. :-) All of the This is really past bike shedding - all the bikes shed have been already built (for all those options). > options (1-4) seem perfectly fine to me. FWIW my preferred color > would probably be 1 because it's the easiest and least inconsistent > with the current state of things. My least favorite would be 3, > because although each individual piece of information is only in one > place, the path to get there is duplicated; both the kernel developer > and the hypervisor developer are forced to continue to deal with both. > The state is that 1-3 have been Nacked by Andrew, 4 has been Nacked by Jan. And 5) (the original way) was way way back Nacked as well. I believe this conversation is to break a tie-breaker between maintainers. (See http://www.xenproject.org/governance.html, Refereeing). That is any of the REST maintainers or Project Leads will override the Nack/Acks. Granted, Jan, me, and Andrew are excluded as we are most certainly not neutral. That means Ian, Tim, and Keir. And then we all can go to the pub.
On Tue, Apr 12, 2016 at 2:56 PM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >> 3. We could use a new hypercall only for new functionality, with the >> proposed new semantics. This would at minimum be build-id, but >> probably also extraversion, compileinfo, changeset, maybe >> capabilities_info. >> >> 4. Have the new hypercall replace the old hypercall. The new >> hypercall will duplicate all the functionality of the old hypercall. >> Deprecate the hypercall for a release or two, then remove it from the >> public headers (although keep the code, because we need to maintain >> backwards compatibility). > > 5). Stick the build-id in the xsplice sysctl. Or just in the sysctl. Which is somewhat similar to #3, except that instead of being in its own hypercall, it's with a bunch of other related functionality. I'd be OK with that color too; it's not a great color but I think it's better than 3. :-) >> This does seem to me an awful lot like a bike shed. :-) All of the > > This is really past bike shedding - all the bikes shed have been > already built (for all those options). You mean, the shed already has 3 layers of paint, a layer of wallpaper, and a layer of plastic siding? :-) >> options (1-4) seem perfectly fine to me. FWIW my preferred color >> would probably be 1 because it's the easiest and least inconsistent >> with the current state of things. My least favorite would be 3, >> because although each individual piece of information is only in one >> place, the path to get there is duplicated; both the kernel developer >> and the hypervisor developer are forced to continue to deal with both. >> > > > The state is that 1-3 have been Nacked by Andrew, 4 has been > Nacked by Jan. And 5) (the original way) was way way back Nacked > as well. There's a difference between "I think this other way is better" and "I am absolutely opposed to this". Well we know which option Andy prefers, but are there other options that Andy is not absolutely opposed to? And we don't know anything about which option Jan prefers at all, except that it's not #4. -George
On Tue, Apr 12, 2016 at 03:38:31PM +0100, George Dunlap wrote: > On Tue, Apr 12, 2016 at 2:56 PM, Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com> wrote: > >> 3. We could use a new hypercall only for new functionality, with the > >> proposed new semantics. This would at minimum be build-id, but > >> probably also extraversion, compileinfo, changeset, maybe > >> capabilities_info. > >> > >> 4. Have the new hypercall replace the old hypercall. The new > >> hypercall will duplicate all the functionality of the old hypercall. > >> Deprecate the hypercall for a release or two, then remove it from the > >> public headers (although keep the code, because we need to maintain > >> backwards compatibility). > > > > 5). Stick the build-id in the xsplice sysctl. Or just in the sysctl. > > Which is somewhat similar to #3, except that instead of being in its > own hypercall, it's with a bunch of other related functionality. I'd > be OK with that color too; it's not a great color but I think it's > better than 3. :-) > > >> This does seem to me an awful lot like a bike shed. :-) All of the > > > > This is really past bike shedding - all the bikes shed have been > > already built (for all those options). > > You mean, the shed already has 3 layers of paint, a layer of > wallpaper, and a layer of plastic siding? :-) <laughs> Yes. Very much so! > > >> options (1-4) seem perfectly fine to me. FWIW my preferred color > >> would probably be 1 because it's the easiest and least inconsistent > >> with the current state of things. My least favorite would be 3, > >> because although each individual piece of information is only in one > >> place, the path to get there is duplicated; both the kernel developer > >> and the hypervisor developer are forced to continue to deal with both. > >> > > > > > > The state is that 1-3 have been Nacked by Andrew, 4 has been > > Nacked by Jan. And 5) (the original way) was way way back Nacked > > as well. > > There's a difference between "I think this other way is better" and "I > am absolutely opposed to this". > > Well we know which option Andy prefers, but are there other options > that Andy is not absolutely opposed to? And we don't know anything > about which option Jan prefers at all, except that it's not #4. /me nods. Thank you for pointing that out.
>>> George Dunlap <dunlapg@umich.edu> 04/12/16 11:58 AM >>> >On Mon, Apr 11, 2016 at 6:46 PM, Jan Beulich <JBeulich@suse.com> wrote: >>> ISTM that the lower duplication (which in principle is an advantage >>> which will be time limited if we are ever able to completely remove >>> teh old hypercall) comes with the cost of (in the long term) increased >>> mess in this particular subop. >> >> We cannot, ever, remove a not toolstack limited hypercall completely >> (or if, then only by Kconfig option so that people knowing none of the >> guests they care about use such deprecated ones). > >We need to keep the hypercall around and functioning for ABI >compatibility, but we could at least remove it from the public headers >and point people to using the new one instead. (Discussion about the >merit of this idea below.) Please don't forget that guests use this to be deprecated hypercall as one of the cheapest possible ways to call into the hypervisor just to get events delivered. I'm afraid the new hypercall would mean (slightly) more overhead. In fact I'm afraid the addition of the XSM call to the old one already had some of this effect namely when XSM is enabled: Konrad - was this considered when you added that? >OK, so here are the options I see: > >1. Use the existing hypercall with the existing call semantics for >build-id -- i.e., require the caller to have a large but fixed-length >buffer (maybe 1024 or 2048). I think it was explained even on this thread that a fixed size may indeed not be suitable here. >2. Use the existing hypercall but wedge in different calling semantics >for the build-id subop. We could have just that subop pay attention >to an extra argument as a length, for example, and return an error if >the length is too short. Or we could make essentially a flag that >asks, "How much space if I were to execute this subop for real?" A suitable variant of this is what I've been arguing for. Jan
George Dunlap writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."):
> Well we know which option Andy prefers, but are there other options
> that Andy is not absolutely opposed to? And we don't know anything
> about which option Jan prefers at all, except that it's not #4.
Let me go a bit further than George.
It's clear that there are various options, most of which are
tolerable. Buit if I'm trying to help referee a disagreement between
Andrew and Jan I would prefer to be choosing between Andrew's
preferred answer and Jan's preferred answer.
Jan: AFAICT it's clear that you would still like the current patch
reverted. Can you please say what, if anything, you would like to
replace it with ?
Thanks,
Ian.
On Tue, Apr 12, 2016 at 09:17:29AM -0600, Jan Beulich wrote: > >>> George Dunlap <dunlapg@umich.edu> 04/12/16 11:58 AM >>> > >On Mon, Apr 11, 2016 at 6:46 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> ISTM that the lower duplication (which in principle is an advantage > >>> which will be time limited if we are ever able to completely remove > >>> teh old hypercall) comes with the cost of (in the long term) increased > >>> mess in this particular subop. > >> > >> We cannot, ever, remove a not toolstack limited hypercall completely > >> (or if, then only by Kconfig option so that people knowing none of the > >> guests they care about use such deprecated ones). > > > >We need to keep the hypercall around and functioning for ABI > >compatibility, but we could at least remove it from the public headers > >and point people to using the new one instead. (Discussion about the > >merit of this idea below.) > > Please don't forget that guests use this to be deprecated hypercall as one of the > cheapest possible ways to call into the hypervisor just to get events delivered. > I'm afraid the new hypercall would mean (slightly) more overhead. In fact I'm > afraid the addition of the XSM call to the old one already had some of this effect > namely when XSM is enabled: Konrad - was this considered when you added > that? Yes, and I raised it up as well :-) However, now that we dropped the XSM checks for some of the sub-ops, it is pointless to do the XSM checks for them (they just do function functions calls that end up with return 0). I can most certainly add back the mask of flags - so only specific sub-ops go through the XSM check. Let me write a patch for this and send it out for review shortly. > > >OK, so here are the options I see: > > > >1. Use the existing hypercall with the existing call semantics for > >build-id -- i.e., require the caller to have a large but fixed-length > >buffer (maybe 1024 or 2048). > > I think it was explained even on this thread that a fixed size may indeed not > be suitable here. > > >2. Use the existing hypercall but wedge in different calling semantics > >for the build-id subop. We could have just that subop pay attention > >to an extra argument as a length, for example, and return an error if > >the length is too short. Or we could make essentially a flag that > >asks, "How much space if I were to execute this subop for real?" > > A suitable variant of this is what I've been arguing for. > > Jan >
>>> George Dunlap <dunlapg@umich.edu> 04/12/16 4:38 PM >>> >On Tue, Apr 12, 2016 at 2:56 PM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >> options (1-4) seem perfectly fine to me. FWIW my preferred color >> would probably be 1 because it's the easiest and least inconsistent >> with the current state of things. My least favorite would be 3, >> because although each individual piece of information is only in one >> place, the path to get there is duplicated; both the kernel developer >> and the hypervisor developer are forced to continue to deal with both. > > The state is that 1-3 have been Nacked by Andrew, 4 has been > Nacked by Jan. And 5) (the original way) was way way back Nacked > as well. No, I only withdrew my ack. Hence an ack from another REST maintainer would suffice for it to stay in as it is now. >There's a difference between "I think this other way is better" and "I >am absolutely opposed to this". > >Well we know which option Andy prefers, but are there other options >that Andy is not absolutely opposed to? And we don't know anything >about which option Jan prefers at all, except that it's not #4. As said in another reply - a suitable variant of 2. Jan
>>> Ian Jackson <Ian.Jackson@eu.citrix.com> 04/12/16 6:47 PM >>> >George Dunlap writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ >but sane."): >> Well we know which option Andy prefers, but are there other options >> that Andy is not absolutely opposed to? And we don't know anything >> about which option Jan prefers at all, except that it's not #4. > >Let me go a bit further than George. > >It's clear that there are various options, most of which are >tolerable. Buit if I'm trying to help referee a disagreement between >Andrew and Jan I would prefer to be choosing between Andrew's >preferred answer and Jan's preferred answer. > >Jan: AFAICT it's clear that you would still like the current patch >reverted. Can you please say what, if anything, you would like to >replace it with ? That patch doesn't need replacing by anything. It's the follow-up patch adding support to retrieve the build-id which would need a replacement, and several options have been put on the table. As mentioned before, I'd prefer the variant of the new sub-op getting added to the existing version hypercall, with the needed length argument passed either via a structure element, with the structure pointed to by the 2nd hypercall argument, or with the high bits of the first hypercall argument re-purposed to allow (and for this sub-op require) holding a length. Which of these two sub-options is chosen I don't really care much. Jan
Jan Beulich writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."):
> George Dunlap <dunlapg@umich.edu> 04/12/16 11:58 AM >>>
> >2. Use the existing hypercall but wedge in different calling semantics
> >for the build-id subop. We could have just that subop pay attention
> >to an extra argument as a length, for example, and return an error if
> >the length is too short. Or we could make essentially a flag that
> >asks, "How much space if I were to execute this subop for real?"
>
> A suitable variant of this is what I've been arguing for.
Earlier I wrote:
It's clear that there are various options, most of which are
tolerable. Buit if I'm trying to help referee a disagreement between
Andrew and Jan I would prefer to be choosing between Andrew's
preferred answer and Jan's preferred answer.
So as I see it we have two options actually seriously proposed:
Andrew's new hypercall, and Jan's additional argument (in the struct,
seems to be what Jan is mainly suggesting).
The new hypercall is neater but more new code - and involves a
deprecation plan; the additional argument is more messy but less
duplication.
I think either of these options is tolerable. I don't see the need to
look further.
Frankly, I find the choice difficult. But the bikeshed has to be
painted /some/ colour and we should make these decisions in a sensible
way and that means I and George (who've been called on to help decide)
need to put forward an opinion.
On balance I think I prefer Jan's suggestion, mostly on the grounds
that in case of dispute, disagreement, or uncertainty, it is (all
other things being equal) better to make smaller changes. And if this
hypercall becomes _too_ much of a mess we can always replace it later
along the lines that Andrew suggests.
I look forward to hearing from George.
Thanks,
Ian.
On Wed, Apr 13, 2016 at 5:07 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."): >> George Dunlap <dunlapg@umich.edu> 04/12/16 11:58 AM >>> >> >2. Use the existing hypercall but wedge in different calling semantics >> >for the build-id subop. We could have just that subop pay attention >> >to an extra argument as a length, for example, and return an error if >> >the length is too short. Or we could make essentially a flag that >> >asks, "How much space if I were to execute this subop for real?" >> >> A suitable variant of this is what I've been arguing for. > > Earlier I wrote: > > It's clear that there are various options, most of which are > tolerable. Buit if I'm trying to help referee a disagreement between > Andrew and Jan I would prefer to be choosing between Andrew's > preferred answer and Jan's preferred answer. > > So as I see it we have two options actually seriously proposed: > Andrew's new hypercall, and Jan's additional argument (in the struct, > seems to be what Jan is mainly suggesting). > > The new hypercall is neater but more new code - and involves a > deprecation plan; the additional argument is more messy but less > duplication. > > I think either of these options is tolerable. I don't see the need to > look further. > > Frankly, I find the choice difficult. But the bikeshed has to be > painted /some/ colour and we should make these decisions in a sensible > way and that means I and George (who've been called on to help decide) > need to put forward an opinion. > > On balance I think I prefer Jan's suggestion, mostly on the grounds > that in case of dispute, disagreement, or uncertainty, it is (all > other things being equal) better to make smaller changes. And if this > hypercall becomes _too_ much of a mess we can always replace it later > along the lines that Andrew suggests. I'm a bit torn here. I think on the whole, I agree with Ian's approach that if there is disagreement, then the more conservative approach should be taken. And if we were discussing an uncommitted patch about which no consensus had been reached, I would second his suggestion. On the other hand, I think there's a bit of a faulty interpretation of the procedure here. Jan reviewed the patch thoroughly and then acked it; on the basis of that, Konrad legitimately checked it in. After it was checked in Jan said, "I've changed my mind and withdraw my Ack"; and the assumption of the subsequent conversation was that an ack *can* be withdrawn after it has been legitimately checked in, and that if no other Ack is supplied, then it must be reverted. I don't think that's a correct interpretation of the rules. Reviewers in general, and maintainers in particular, should make reasonably sure that they mean the Ack before they give it; and if they change their mind after it has been legitimately checked in, then it's now up to them to make the change they want to see according to the regular procedure. That is, if Jan wants it reverted, he needs to post a patch reverting it and get Acks from the appropriate maintainers; and the discussion needs to be around Jan's reversion being accepted, not about Konrad's original patch continuing to be accepted. (Obvious exceptions can be made in the case of emergencies like build breakages.) Normally it's best to try to come to agreement instead of falling back to rule lawyering; and in any case it's always easier to talk about the rules when we're not trying to sort out a specific situation. But since we've failed to reach a genuine agreement, in this case I think the rule lawyering is probably a legitimate way to resolve the issue. Thoughts? -George
>>> George Dunlap <george.dunlap@citrix.com> 04/14/16 5:16 PM >>> >On the other hand, I think there's a bit of a faulty interpretation of >the procedure here. Jan reviewed the patch thoroughly and then acked >it; on the basis of that, Konrad legitimately checked it in. After it >was checked in Jan said, "I've changed my mind and withdraw my Ack"; >and the assumption of the subsequent conversation was that an ack >*can* be withdrawn after it has been legitimately checked in, and that >if no other Ack is supplied, then it must be reverted. > >I don't think that's a correct interpretation of the rules. Reviewers >in general, and maintainers in particular, should make reasonably sure >that they mean the Ack before they give it; and if they change their >mind after it has been legitimately checked in, then it's now up to >them to make the change they want to see according to the regular >procedure. That is, if Jan wants it reverted, he needs to post a >patch reverting it and get Acks from the appropriate maintainers; and >the discussion needs to be around Jan's reversion being accepted, not >about Konrad's original patch continuing to be accepted. (Obvious >exceptions can be made in the case of emergencies like build >breakages.) Fundamentally I agree, but I think there's more to this than just following a set of rules. For example, please don't forget the time pressure due to the (at that time) rapidly approaching freeze date. And then, mistakes happen, and so I made a mistake here by sending the ack a few hours too early. What is really hard to understand to me is why it is so difficult to just get a refereeing opinion on the actual interface change. IMO we don't really need to discuss rules and processes, the question is as simple as "Do we want/need this new interface?" Jan
On Thu, Apr 14, 2016 at 4:59 PM, Jan Beulich <jbeulich@suse.com> wrote: >>>> George Dunlap <george.dunlap@citrix.com> 04/14/16 5:16 PM >>> >>On the other hand, I think there's a bit of a faulty interpretation of >>the procedure here. Jan reviewed the patch thoroughly and then acked >>it; on the basis of that, Konrad legitimately checked it in. After it >>was checked in Jan said, "I've changed my mind and withdraw my Ack"; >>and the assumption of the subsequent conversation was that an ack >>*can* be withdrawn after it has been legitimately checked in, and that >>if no other Ack is supplied, then it must be reverted. >> >>I don't think that's a correct interpretation of the rules. Reviewers >>in general, and maintainers in particular, should make reasonably sure >>that they mean the Ack before they give it; and if they change their >>mind after it has been legitimately checked in, then it's now up to >>them to make the change they want to see according to the regular >>procedure. That is, if Jan wants it reverted, he needs to post a >>patch reverting it and get Acks from the appropriate maintainers; and >>the discussion needs to be around Jan's reversion being accepted, not >>about Konrad's original patch continuing to be accepted. (Obvious >>exceptions can be made in the case of emergencies like build >>breakages.) > > Fundamentally I agree, but I think there's more to this than just following > a set of rules. For example, please don't forget the time pressure due to > the (at that time) rapidly approaching freeze date. And then, mistakes > happen, and so I made a mistake here by sending the ack a few hours too > early. Sure, mistakes happen; but I hope it's not being to controversial to say that in general, the procedure should be arranged such that the person who makes the mistake is the one who has to do deal with the most consequences from the mistake, not the people around him. I mean, if you asked a bartender for a bottle of beer, and after he'd already opened it you said, "Oh sorry, I actually wanted a cider", it would be fair enough for the bartender to ask you to pay for the beer, rather than eating it*, wouldn't it? :-) > What is really hard to understand to me is why it is so difficult to just > get a refereeing opinion on the actual interface change. IMO we don't > really need to discuss rules and processes, the question is as simple > as "Do we want/need this new interface?" Well Ian and I have already given our opinions -- Ian thinks moving to a clean interface and deprecating the old one is in general a good thing, and doesn't look too painful in this case. I don't really see a problem with using a large fixed size, but I don't fundamentally see a problem with moving to a new clean interface either. Given that Andy has a strong aversion to the way things are, if you had only a mild distaste rather than a strong objection to the new hypercall, it would probably make sense to go with the new hypercall. If you do have a strong objection, then maybe we could try to convince Andy to accept adding subops with different calling semantics to the existing hypercall. But I would still think the burden of persuasion is primarily on you. -George * In this context "eating" something means just accepting the loss of the thing yourself. For instance, if you bought two tickets to a concert but couldn't convince anyone to go with you, you'd say you had to "eat the other ticket". Here it means the bartender throws the beer away and accepts the loss himself.
>>> George Dunlap <george.dunlap@citrix.com> 04/14/16 6:20 PM >>> >On Thu, Apr 14, 2016 at 4:59 PM, Jan Beulich <jbeulich@suse.com> wrote: >>>>> George Dunlap <george.dunlap@citrix.com> 04/14/16 5:16 PM >>> >>>On the other hand, I think there's a bit of a faulty interpretation of >>>the procedure here. Jan reviewed the patch thoroughly and then acked >>>it; on the basis of that, Konrad legitimately checked it in. After it >>>was checked in Jan said, "I've changed my mind and withdraw my Ack"; >>>and the assumption of the subsequent conversation was that an ack >>>*can* be withdrawn after it has been legitimately checked in, and that >>>if no other Ack is supplied, then it must be reverted. >>> >>>I don't think that's a correct interpretation of the rules. Reviewers >>>in general, and maintainers in particular, should make reasonably sure >>>that they mean the Ack before they give it; and if they change their >>>mind after it has been legitimately checked in, then it's now up to >>>them to make the change they want to see according to the regular >>>procedure. That is, if Jan wants it reverted, he needs to post a >>>patch reverting it and get Acks from the appropriate maintainers; and >>>the discussion needs to be around Jan's reversion being accepted, not >>>about Konrad's original patch continuing to be accepted. (Obvious >>>exceptions can be made in the case of emergencies like build >>>breakages.) >> >> Fundamentally I agree, but I think there's more to this than just following >> a set of rules. For example, please don't forget the time pressure due to >> the (at that time) rapidly approaching freeze date. And then, mistakes >> happen, and so I made a mistake here by sending the ack a few hours too >> early. > >Sure, mistakes happen; but I hope it's not being to controversial to >say that in general, the procedure should be arranged such that the >person who makes the mistake is the one who has to do deal with the >most consequences from the mistake, not the people around him. I >mean, if you asked a bartender for a bottle of beer, and after he'd >already opened it you said, "Oh sorry, I actually wanted a cider", it >would be fair enough for the bartender to ask you to pay for the beer, >rather than eating it*, wouldn't it? :-) Sure. And I think that's what I've done. I suggested to revert the thing, collecting opinions either direction. I just didn't post a revert patch, as I think that makes little sense - a revert is a mechanical operation, which doesn't need people looking at the actual patch. >Well Ian and I have already given our opinions -- Ian thinks moving to >a clean interface and deprecating the old one is in general a good >thing, and doesn't look too painful in this case. I don't really see >a problem with using a large fixed size, but I don't fundamentally see >a problem with moving to a new clean interface either. Given that >Andy has a strong aversion to the way things are, if you had only a >mild distaste rather than a strong objection to the new hypercall, it >would probably make sense to go with the new hypercall. > >If you do have a strong objection, then maybe we could try to convince >Andy to accept adding subops with different calling semantics to the >existing hypercall. But I would still think the burden of persuasion >is primarily on you. I do not have a strong objection, or else I would have converted my ack into a nak instead of just withdrawing it. I just dislike the duplication, and hence I'm not happy with me now being the one having approved it to go in. Hence the request for a replacement ack (or whatever else referee decision). And btw., considering that Konrad has already posted a revert patch, and I have ack-ed that one, this could now go in right away (and the discussion could either be settled or start over). Jan
George Dunlap writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."):
> On the other hand, I think there's a bit of a faulty interpretation of
> the procedure here. Jan reviewed the patch thoroughly and then acked
> it; on the basis of that, Konrad legitimately checked it in. After it
> was checked in Jan said, "I've changed my mind and withdraw my Ack";
> and the assumption of the subsequent conversation was that an ack
> *can* be withdrawn after it has been legitimately checked in, and that
> if no other Ack is supplied, then it must be reverted.
>
> I don't think that's a correct interpretation of the rules. Reviewers
> in general, and maintainers in particular, should make reasonably sure
> that they mean the Ack before they give it; and if they change their
> mind after it has been legitimately checked in, then it's now up to
> them to make the change they want to see according to the regular
> procedure.
For the record, I agree completely with George here. I was expecting
that the next step would be to for Jan to post patches to revert the
extra hypercall and replace it with something else.
Jan Beulich writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested Was:
> And btw., considering that Konrad has already posted a revert patch,
> and I have ack-ed that one, this could now go in right away (and the
> discussion could either be settled or start over).
I don't see that patch you describe in my inbox, but maybe I have
missed it.
If that reversion is proposed, following a request for a 2nd/3rd
opinion from me and George, and given the discussion so far, I think
that patch ought to have been CC'd to me and George.
I don't think it would be appropriate to commit a revert except as
part of a series which introduces an replacement way of providing the
needed functionality - at least, enough functionality that in practice
a plausibly long build-id can be retrieved.
If you want the original reverted, I think it is up to you, Jan, to
provide (or procure) such a replacement.
Thanks,
Ian.
On Thu, Apr 14, 2016 at 07:11:46PM +0100, Ian Jackson wrote: > George Dunlap writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."): > > On the other hand, I think there's a bit of a faulty interpretation of > > the procedure here. Jan reviewed the patch thoroughly and then acked > > it; on the basis of that, Konrad legitimately checked it in. After it > > was checked in Jan said, "I've changed my mind and withdraw my Ack"; > > and the assumption of the subsequent conversation was that an ack > > *can* be withdrawn after it has been legitimately checked in, and that > > if no other Ack is supplied, then it must be reverted. > > > > I don't think that's a correct interpretation of the rules. Reviewers > > in general, and maintainers in particular, should make reasonably sure > > that they mean the Ack before they give it; and if they change their > > mind after it has been legitimately checked in, then it's now up to > > them to make the change they want to see according to the regular > > procedure. > > For the record, I agree completely with George here. I was expecting > that the next step would be to for Jan to post patches to revert the > extra hypercall and replace it with something else. > > Jan Beulich writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested Was: > > And btw., considering that Konrad has already posted a revert patch, > > and I have ack-ed that one, this could now go in right away (and the > > discussion could either be settled or start over). > > I don't see that patch you describe in my inbox, but maybe I have > missed it. It is part of my series. This is the revert (there are two of them) http://lists.xen.org/archives/html/xen-devel/2016-04/msg01913.html http://lists.xen.org/archives/html/xen-devel/2016-04/msg01926.html And then these two patches add build-id using the XENVER hypercall: http://lists.xen.org/archives/html/xen-devel/2016-04/msg01923.html http://lists.xen.org/archives/html/xen-devel/2016-04/msg01902.html > > If that reversion is proposed, following a request for a 2nd/3rd > opinion from me and George, and given the discussion so far, I think > that patch ought to have been CC'd to me and George. Argh, I probably missed you and George on them. My apologies! > > I don't think it would be appropriate to commit a revert except as > part of a series which introduces an replacement way of providing the > needed functionality - at least, enough functionality that in practice > a plausibly long build-id can be retrieved. > > If you want the original reverted, I think it is up to you, Jan, to > provide (or procure) such a replacement. > > Thanks, > Ian.
On Thu, Apr 14, 2016 at 6:01 PM, Jan Beulich <jbeulich@suse.com> wrote: >>Sure, mistakes happen; but I hope it's not being to controversial to >>say that in general, the procedure should be arranged such that the >>person who makes the mistake is the one who has to do deal with the >>most consequences from the mistake, not the people around him. I >>mean, if you asked a bartender for a bottle of beer, and after he'd >>already opened it you said, "Oh sorry, I actually wanted a cider", it >>would be fair enough for the bartender to ask you to pay for the beer, >>rather than eating it*, wouldn't it? :-) > > Sure. And I think that's what I've done. I suggested to revert the thing, > collecting opinions either direction. I just didn't post a revert patch, as I > think that makes little sense - a revert is a mechanical operation, which > doesn't need people looking at the actual patch. You don't need people to review the content of the actual patch, but it provides a standardized way to have a discussion about the patch, and it makes it clear what the procedure is for having it applied. You don't need any technical code review for adding or removing maintainers either, but we ask people to send patches to the MAINTAINERS file anyway, because it provides a structured way to have an open discussion and make a decision. When Konrad asked for further input on the patch and then didn't get any in a few days, you responded, "It looks like it will have to be reverted then." As I've said, I think that's the wrong way round -- it's not that the commit is reverted unless someone else acks it; it's that the commit stays unless someone acks your reversion. If you had posted a patch (probably RFC) requesting to revert the change in favor of a different one, then it would have been more obvious that the burden was on you to get the reversion Acked, rather than on Konrad to get the existing commit re-acked. >>Well Ian and I have already given our opinions -- Ian thinks moving to >>a clean interface and deprecating the old one is in general a good >>thing, and doesn't look too painful in this case. I don't really see >>a problem with using a large fixed size, but I don't fundamentally see >>a problem with moving to a new clean interface either. Given that >>Andy has a strong aversion to the way things are, if you had only a >>mild distaste rather than a strong objection to the new hypercall, it >>would probably make sense to go with the new hypercall. >> >>If you do have a strong objection, then maybe we could try to convince >>Andy to accept adding subops with different calling semantics to the >>existing hypercall. But I would still think the burden of persuasion >>is primarily on you. > > I do not have a strong objection, or else I would have converted my ack > into a nak instead of just withdrawing it. I just dislike the duplication, and > hence I'm not happy with me now being the one having approved it to go > in. Hence the request for a replacement ack (or whatever else referee > decision). Well this makes it sound like you're saying that you were asking us to save you from having to appear to have approved of a patch that you didn't like. Which doesn't sound very nice. :-) But I wonder if something slightly different is going on. Forgive me for trying to guess at motivations here, but I think it may be helpful. I'm often in the situation where my gut objects to a patch that other coders I respect think is fine; and often a few years down the road, I look back and agree that it's fine as well. In other words, I know that sometimes my own objections turn out to be unreasonable; and that in any case, working with other people you sometimes have to compromise. But on the other hand, I've also had the experience of giving in and accepting patches that later I regret, or that other people come back and say, "This was a terrible idea, you should have stood up for it more." So in the moment -- particularly, as you say, under time pressure -- how do you determine if objecting to the patch is being unreasonable and obstructive, or if assenting to the patch is failing your duty as a maintainer to prevent bad code, and is a decision you'll regret later? Was it perhaps actually the case that your internal reasoning was along the lines of, "Actually, adding a new hypercall seems like a bad idea. But since Andrew strongly disagrees, maybe I'm being unreasonable. Or, maybe it really is a bad idea and he's being unreasonable. Why don't I ask the REST maintainers to look at it; if they Ack it, then I'm probably being too conservative; if they don't, then I'm probably justified in objecting to it"? If so, we should probably find a better way for maintainers to ask for additional feedback in those situations. :-) I'd certainly appreciate that at times. If that's not the case, and you genuinely only have a mild dislike for the hypercall, then there are a couple of things to say about that. The first is that given the circumstances, it seems to me that giving the Ack was not only reasonable, but the right thing to do. An Ack doesn't mean, "I think this is a good idea", but it means "Given all considerations, I think this should be in the tree." And given that Andrew had strong objections to the other solutions, and you only had a distaste for this one, this is obviously the solution to choose. Acking a patch on the basis that you don't really like it but it's the best compromise with the other maintainers is perfectly reasonable. Secondly, I can certainly see that you might be a bit embarrased about having your name on a patch you dislike, but... well, you did in fact approve it going in. :-) Maybe that was because you were in haste, and you regret it, but that's an accurate record of what happened. Stand up and take responsibility for your actions. :-) > And btw., considering that Konrad has already posted a revert patch, > and I have ack-ed that one, this could now go in right away (and the > discussion could either be settled or start over). Yes, unless someone objects to that patch then we have a clear way forward, and we're just discussing principles for future reference at this point. -George
>>> Ian Jackson <Ian.Jackson@eu.citrix.com> 04/14/16 8:12 PM >>> >Jan Beulich writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested Was: >> And btw., considering that Konrad has already posted a revert patch, >> and I have ack-ed that one, this could now go in right away (and the >> discussion could either be settled or start over). > >I don't see that patch you describe in my inbox, but maybe I have >missed it. Patches 1 and 2 of the most recent v8.1 series. >If that reversion is proposed, following a request for a 2nd/3rd >opinion from me and George, and given the discussion so far, I think >that patch ought to have been CC'd to me and George. > >I don't think it would be appropriate to commit a revert except as >part of a series which introduces an replacement way of providing the >needed functionality - at least, enough functionality that in practice >a plausibly long build-id can be retrieved. For one, the revert wouldn't revert that functionality, as that didn't even go in yet. >If you want the original reverted, I think it is up to you, Jan, to >provide (or procure) such a replacement. And with that (albeit not just because of it not being in yet at all), I don't see why this would be the case. I think there's some general disagreement here, which with the hackathon around the corner we probably should just get sorted out in person there. Jan
>>> George Dunlap <george.dunlap@citrix.com> 04/15/16 1:23 PM >>> >On Thu, Apr 14, 2016 at 6:01 PM, Jan Beulich <jbeulich@suse.com> wrote: >>>Sure, mistakes happen; but I hope it's not being to controversial to >>>say that in general, the procedure should be arranged such that the >>>person who makes the mistake is the one who has to do deal with the >>>most consequences from the mistake, not the people around him. I >>>mean, if you asked a bartender for a bottle of beer, and after he'd >>>already opened it you said, "Oh sorry, I actually wanted a cider", it >>>would be fair enough for the bartender to ask you to pay for the beer, >>>rather than eating it*, wouldn't it? :-) >> >> Sure. And I think that's what I've done. To be fair, I think this was too broad a statement: I had indeed asked Konrad to collect the replacement ack. Yet on second thought I really can't see what's wrong with withdraing an ack - just like a patch can be withdrawn, that should be possible for an ack too. And if the patch has already gone in, no matter whether the patch itself or the ack got withdrawn, the patch imo should then be reverted. I agree that's not something spelled out anywhere, so my opinion here is based on solely general considerations of mine. >When Konrad asked for further input on the patch and then didn't get >any in a few days, you responded, "It looks like it will have to be >reverted then." As I've said, I think that's the wrong way round -- >it's not that the commit is reverted unless someone else acks it; it's >that the commit stays unless someone acks your reversion. If you had >posted a patch (probably RFC) requesting to revert the change in favor >of a different one, then it would have been more obvious that the >burden was on you to get the reversion Acked, rather than on Konrad to >get the existing commit re-acked. I agree on the process aspect (albeit among the few cases where things needed reverting, I think there was a non-negligible amount of such where the revert was requested quite informally), but that's relatively moot with me not agreeing on the premises it builds upon. >>>Well Ian and I have already given our opinions -- Ian thinks moving to >>>a clean interface and deprecating the old one is in general a good >>>thing, and doesn't look too painful in this case. I don't really see >>>a problem with using a large fixed size, but I don't fundamentally see >>>a problem with moving to a new clean interface either. Given that >>>Andy has a strong aversion to the way things are, if you had only a >>>mild distaste rather than a strong objection to the new hypercall, it >>>would probably make sense to go with the new hypercall. >>> >>>If you do have a strong objection, then maybe we could try to convince >>>Andy to accept adding subops with different calling semantics to the >>>existing hypercall. But I would still think the burden of persuasion >>>is primarily on you. >> >> I do not have a strong objection, or else I would have converted my ack >> into a nak instead of just withdrawing it. I just dislike the duplication, and >> hence I'm not happy with me now being the one having approved it to go >> in. Hence the request for a replacement ack (or whatever else referee >> decision). > >Well this makes it sound like you're saying that you were asking us to >save you from having to appear to have approved of a patch that you >didn't like. Which doesn't sound very nice. :-) I'm sorry if it's being understood that way. My intention really was to avoid the revert in case a replacement ack can be obtained. Otherwise, following my way of thinking outlined above, the patch should have been reverted right away. Yet I seem to be the only one here thinking this way... >But I wonder if something slightly different is going on. Forgive me >for trying to guess at motivations here, but I think it may be >helpful. I'm often in the situation where my gut objects to a patch >that other coders I respect think is fine; and often a few years down >the road, I look back and agree that it's fine as well. In other >words, I know that sometimes my own objections turn out to be >unreasonable; and that in any case, working with other people you >sometimes have to compromise. But on the other hand, I've also had >the experience of giving in and accepting patches that later I regret, >or that other people come back and say, "This was a terrible idea, you >should have stood up for it more." > >So in the moment -- particularly, as you say, under time pressure -- >how do you determine if objecting to the patch is being unreasonable >and obstructive, or if assenting to the patch is failing your duty as >a maintainer to prevent bad code, and is a decision you'll regret >later? > >Was it perhaps actually the case that your internal reasoning was >along the lines of, "Actually, adding a new hypercall seems like a bad >idea. But since Andrew strongly disagrees, maybe I'm being >unreasonable. Or, maybe it really is a bad idea and he's being >unreasonable. Why don't I ask the REST maintainers to look at it; if >they Ack it, then I'm probably being too conservative; if they don't, >then I'm probably justified in objecting to it"? Well, not really. My main problem here (and not just here) is that often I end up reviewing patches for being done correctly, but ignore the question of "Do we need this in the first place?" This has happened to me more often earlier on, but it continues to happen from time to time, like on this occasion. >If so, we should probably find a better way for maintainers to ask for >additional feedback in those situations. :-) I'd certainly appreciate >that at times. Well, the main problem I'm seeing here that the current set of REST maintainers makes it very hard to get _any_ kind of feedback without explicitly asking and pinging for it. I sincerely hope that this will change with the pending committership and maintainership adjustments. My general understanding here is that it shouldn't be really necessary to explicitly ask for a second opinion - after all maintainers get Cc-ed on patches for the reason of seeking their input. And the question of replacing an existing public interface with a slightly different new one, even more so with no obvious route of deprecating the old one, is something that any one of the REST maintainers should really have an opinion on imo. >If that's not the case, and you genuinely only have a mild dislike for >the hypercall, then there are a couple of things to say about that. >The first is that given the circumstances, it seems to me that giving >the Ack was not only reasonable, but the right thing to do. An Ack >doesn't mean, "I think this is a good idea", but it means "Given all >considerations, I think this should be in the tree." And given that >Andrew had strong objections to the other solutions, and you only had >a distaste for this one, this is obviously the solution to choose. >Acking a patch on the basis that you don't really like it but it's the >best compromise with the other maintainers is perfectly reasonable. Taking a strictly formal perspective, there was no disagreement between maintainers, since I've been the only one of those currently named under REST involved in the discussion. Which is part of the problem, as said above. And Andrew's strong objection was, in my view, based on not very convincing arguments. Hence I couldn't really see myself making a reasonable compromise here. >Secondly, I can certainly see that you might be a bit embarrased >about having your name on a patch you dislike, but... well, you did in >fact approve it going in. :-) Maybe that was because you were in >haste, and you regret it, but that's an accurate record of what >happened. Stand up and take responsibility for your actions. :-) Which is why - see above - I didn't revert it right away. >> And btw., considering that Konrad has already posted a revert patch, >> and I have ack-ed that one, this could now go in right away (and the >> discussion could either be settled or start over). > >Yes, unless someone objects to that patch then we have a clear way >forward, and we're just discussing principles for future reference at >this point. Agreed. As said in reply to Ian - perhaps worth taking some time at the hackathon. Jan
diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te index e174e48..7e69ce9 100644 --- a/tools/flask/policy/policy/modules/xen/xen.te +++ b/tools/flask/policy/policy/modules/xen/xen.te @@ -74,11 +74,12 @@ allow dom0_t xen_t:xen2 { get_symbol }; -# Allow dom0 to use all XENVER_ subops that have checks. +# Allow dom0 to use all XENVER_ subops and VERSION subops that have checks. # Note that dom0 is part of domain_type so this has duplicates. allow dom0_t xen_t:version { xen_extraversion xen_compile_info xen_capabilities xen_changeset xen_pagesize xen_guest_handle xen_commandline + extraversion capabilities changeset pagesize guest_handle commandline }; allow dom0_t xen_t:mmu memorymap; @@ -145,10 +146,12 @@ if (guest_writeconsole) { # pmu_ctrl is for) allow domain_type xen_t:xen2 pmu_use; -# For normal guests all possible except XENVER_commandline. +# For normal guests all possible except XENVER_commandline, VERSION_changeset, +# and VERSION_commandline allow domain_type xen_t:version { xen_extraversion xen_compile_info xen_capabilities xen_changeset xen_pagesize xen_guest_handle + extraversion capabilities pagesize guest_handle }; ############################################################################### diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 83744e8..31d2115 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1235,6 +1235,7 @@ static arm_hypercall_t arm_hypercall_table[] = { HYPERCALL(multicall, 2), HYPERCALL(platform_op, 1), HYPERCALL_ARM(vcpu_op, 3), + HYPERCALL(version_op, 3), }; #ifndef NDEBUG diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 80d59ff..f16b590 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -5322,6 +5322,7 @@ static const struct { COMPAT_CALL(platform_op), COMPAT_CALL(mmuext_op), HYPERCALL(xenpmu_op), + HYPERCALL(version_op), HYPERCALL(arch_1) }; diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S index 33e2c12..fd25e84 100644 --- a/xen/arch/x86/x86_64/compat/entry.S +++ b/xen/arch/x86/x86_64/compat/entry.S @@ -394,6 +394,7 @@ ENTRY(compat_hypercall_table) .quad do_tmem_op .quad do_ni_hypercall /* reserved for XenClient */ .quad do_xenpmu_op /* 40 */ + .quad do_version_op .rept __HYPERVISOR_arch_0-((.-compat_hypercall_table)/8) .quad compat_ni_hypercall .endr @@ -445,6 +446,7 @@ ENTRY(compat_hypercall_args_table) .byte 1 /* do_tmem_op */ .byte 0 /* reserved for XenClient */ .byte 2 /* do_xenpmu_op */ /* 40 */ + .byte 3 /* do_version_op */ .rept __HYPERVISOR_arch_0-(.-compat_hypercall_args_table) .byte 0 /* compat_ni_hypercall */ .endr diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S index 07ef096..b0e7257 100644 --- a/xen/arch/x86/x86_64/entry.S +++ b/xen/arch/x86/x86_64/entry.S @@ -730,6 +730,7 @@ ENTRY(hypercall_table) .quad do_tmem_op .quad do_ni_hypercall /* reserved for XenClient */ .quad do_xenpmu_op /* 40 */ + .quad do_version_op .rept __HYPERVISOR_arch_0-((.-hypercall_table)/8) .quad do_ni_hypercall .endr @@ -781,6 +782,7 @@ ENTRY(hypercall_args_table) .byte 1 /* do_tmem_op */ .byte 0 /* reserved for XenClient */ .byte 2 /* do_xenpmu_op */ /* 40 */ + .byte 3 /* do_version_op */ .rept __HYPERVISOR_arch_0-(.-hypercall_args_table) .byte 0 /* do_ni_hypercall */ .endr diff --git a/xen/common/compat/kernel.c b/xen/common/compat/kernel.c index df93fdd..7a7ca53 100644 --- a/xen/common/compat/kernel.c +++ b/xen/common/compat/kernel.c @@ -39,6 +39,8 @@ CHECK_TYPE(capabilities_info); CHECK_TYPE(domain_handle); +CHECK_TYPE(version_op_val); + #define xennmi_callback compat_nmi_callback #define xennmi_callback_t compat_nmi_callback_t diff --git a/xen/common/kernel.c b/xen/common/kernel.c index a4a3c36..f6d2676 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -221,6 +221,47 @@ void __init do_initcalls(void) #endif +static int get_features(struct domain *d, xen_feature_info_t *fi) +{ + switch ( fi->submap_idx ) + { + case 0: + fi->submap = (1U << XENFEAT_memory_op_vnode_supported); + if ( paging_mode_translate(d) ) + fi->submap |= + (1U << XENFEAT_writable_page_tables) | + (1U << XENFEAT_auto_translated_physmap); + if ( is_hardware_domain(d) ) + fi->submap |= 1U << XENFEAT_dom0; +#ifdef CONFIG_X86 + if ( VM_ASSIST(d, pae_extended_cr3) ) + fi->submap |= (1U << XENFEAT_pae_pgdir_above_4gb); + switch ( d->guest_type ) + { + case guest_type_pv: + fi->submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) | + (1U << XENFEAT_highmem_assist) | + (1U << XENFEAT_gnttab_map_avail_bits); + break; + case guest_type_pvh: + fi->submap |= (1U << XENFEAT_hvm_safe_pvclock) | + (1U << XENFEAT_supervisor_mode_kernel) | + (1U << XENFEAT_hvm_callback_vector); + break; + case guest_type_hvm: + fi->submap |= (1U << XENFEAT_hvm_safe_pvclock) | + (1U << XENFEAT_hvm_callback_vector) | + (1U << XENFEAT_hvm_pirqs); + break; + } +#endif + break; + default: + return -EINVAL; + } + return 0; +} + /* * Simple hypercalls. */ @@ -298,47 +339,14 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) case XENVER_get_features: { xen_feature_info_t fi; - struct domain *d = current->domain; + int rc; if ( copy_from_guest(&fi, arg, 1) ) return -EFAULT; - switch ( fi.submap_idx ) - { - case 0: - fi.submap = (1U << XENFEAT_memory_op_vnode_supported); - if ( VM_ASSIST(d, pae_extended_cr3) ) - fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb); - if ( paging_mode_translate(d) ) - fi.submap |= - (1U << XENFEAT_writable_page_tables) | - (1U << XENFEAT_auto_translated_physmap); - if ( is_hardware_domain(d) ) - fi.submap |= 1U << XENFEAT_dom0; -#ifdef CONFIG_X86 - switch ( d->guest_type ) - { - case guest_type_pv: - fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) | - (1U << XENFEAT_highmem_assist) | - (1U << XENFEAT_gnttab_map_avail_bits); - break; - case guest_type_pvh: - fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) | - (1U << XENFEAT_supervisor_mode_kernel) | - (1U << XENFEAT_hvm_callback_vector); - break; - case guest_type_hvm: - fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) | - (1U << XENFEAT_hvm_callback_vector) | - (1U << XENFEAT_hvm_pirqs); - break; - } -#endif - break; - default: - return -EINVAL; - } + rc = get_features(current->domain, &fi); + if ( rc ) + return rc; if ( __copy_to_guest(arg, &fi, 1) ) return -EFAULT; @@ -381,6 +389,123 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) return -ENOSYS; } +/* Computed by capabilities_cache_init. */ +static xen_capabilities_info_t __read_mostly cached_cap; +static unsigned int __read_mostly cached_cap_len; + +/* + * Similar to HYPERVISOR_xen_version but with a sane interface + * (has a length, one can probe for the length) and with one less sub-ops: + * missing XENVER_compile_info. + */ +DO(version_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg, + unsigned int len) +{ + union { + xen_version_op_val_t val; + xen_feature_info_t fi; + } u = {}; + unsigned int sz = 0; + const void *ptr = NULL; + int rc = xsm_version_op(XSM_OTHER, cmd); + + /* We can safely return -EPERM! */ + if ( rc ) + return rc; + + /* + * The HYPERVISOR_xen_version differs in that some return the value, + * and some copy it on back on argument. We follow the same rule for all + * sub-ops: return the number of bytes written, or negative errno on + * failure, and always copy the result in arg. Yeey sanity! + */ + switch ( cmd ) + { + case XEN_VERSION_version: + sz = sizeof(xen_version_op_val_t); + u.val = (xen_major_version() << 16) | xen_minor_version(); + break; + + case XEN_VERSION_extraversion: + sz = strlen(xen_extra_version()) + 1; + ptr = xen_extra_version(); + break; + + case XEN_VERSION_capabilities: + sz = cached_cap_len; + ptr = cached_cap; + break; + + case XEN_VERSION_changeset: + sz = strlen(xen_changeset()) + 1; + ptr = xen_changeset(); + break; + + case XEN_VERSION_platform_parameters: + sz = sizeof(xen_version_op_val_t); + u.val = HYPERVISOR_VIRT_START; + break; + + case XEN_VERSION_get_features: + sz = sizeof(xen_feature_info_t); + + if ( guest_handle_is_null(arg) ) + break; + + if ( copy_from_guest(&u.fi, arg, 1) ) + { + rc = -EFAULT; + break; + } + rc = get_features(current->domain, &u.fi); + break; + + case XEN_VERSION_pagesize: + sz = sizeof(xen_version_op_val_t); + u.val = PAGE_SIZE; + break; + + case XEN_VERSION_guest_handle: + sz = ARRAY_SIZE(current->domain->handle); + ptr = current->domain->handle; + break; + + case XEN_VERSION_commandline: + sz = strlen(saved_cmdline) + 1; + ptr = saved_cmdline; + break; + + default: + rc = -ENOSYS; + } + + if ( rc ) + return rc; + + /* + * This hypercall also allows the client to probe. If it provides + * a NULL arg we will return the size of the space it has to + * allocate for the specific sub-op. + */ + ASSERT(sz); + if ( guest_handle_is_null(arg) ) + return sz; + + if ( !rc ) + { + unsigned int bytes = min(sz, len); + + if ( copy_to_guest(arg, ptr ? : &u, bytes) ) + rc = -EFAULT; + + /* We return len (truncate) worth of data even if we fail. */ + if ( !rc && sz > len ) + rc = -ENOBUFS; + } + + return rc == 0 ? sz : rc; +} + DO(nmi_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct xennmi_callback cb; @@ -418,6 +543,20 @@ DO(ni_hypercall)(void) return -ENOSYS; } +static int __init capabilities_cache_init(void) +{ + /* + * Pre-allocate the cache so we do not have to worry about + * simultaneous invocations on safe_strcat by guests and the cache + * data becoming garbage. + */ + arch_get_xen_caps(&cached_cap); + cached_cap_len = strlen(cached_cap) + 1; + + return 0; +} +__initcall(capabilities_cache_init); + /* * Local variables: * mode: C diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index 870bc3b..5f90718 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -128,6 +128,8 @@ * * VCPUOP_register_vcpu_info * * VCPUOP_register_runstate_memory_area * + * HYPERVISOR_version_op + * All generic sub-operations * * Other notes on the ARM ABI: * diff --git a/xen/include/public/version.h b/xen/include/public/version.h index 24a582f..d71ec5b 100644 --- a/xen/include/public/version.h +++ b/xen/include/public/version.h @@ -30,7 +30,14 @@ #include "xen.h" -/* NB. All ops return zero on success, except XENVER_{version,pagesize} */ +/* + * There are two hypercalls mentioned in here. The XENVER_ are for + * HYPERCALL_xen_version (17), while VERSION_ are for the + * HYPERCALL_version_op (41). + * + * The subops are very similar except that the later hypercall has a + * sane interface. + */ /* arg == NULL; returns major:minor (16:16). */ #define XENVER_version 0 @@ -87,6 +94,67 @@ typedef struct xen_feature_info xen_feature_info_t; #define XENVER_commandline 9 typedef char xen_commandline_t[1024]; +/* + * The HYPERCALL_version_op has a set of sub-ops which mirror the + * sub-ops of HYPERCALL_xen_version. However this hypercall differs + * radically from the former: + * - It returns the amount of bytes returned. + * - It will return -XEN_EPERM if the guest is not permitted + * (Albeit XEN_VERSION_version, XEN_VERSION_platform_parameters, and + * XEN_VERSION_get_features will always return an value as guest cannot + * survive without this information). + * - It will return the requested data in arg. + * - It requires an third argument (len) for the length of the + * arg. Naturally the arg has to fit the requested data otherwise + * -XEN_ENOBUFS is returned. + * + * It also offers an mechanism to probe for the amount of bytes an + * sub-op will require. Having the arg have an NULL handle will + * return the number of bytes requested for the operation. Or an + * negative value if an error is encountered. + */ + +typedef uint64_t xen_version_op_val_t; +DEFINE_XEN_GUEST_HANDLE(xen_version_op_val_t); + +/* + * arg == xen_version_op_val_t. Encoded as major:minor (31..16:15..0), while + * 63..32 are zero. + */ +#define XEN_VERSION_version 0 + +/* arg == char[]. Contains NUL terminated utf-8 string. */ +#define XEN_VERSION_extraversion 1 + +/* arg == char[]. Contains NUL terminated utf-8 string. */ +#define XEN_VERSION_capabilities 3 + +/* arg == char[]. Contains NUL terminated utf-8 string. */ +#define XEN_VERSION_changeset 4 + +/* arg == xen_version_op_val_t. */ +#define XEN_VERSION_platform_parameters 5 + +/* + * arg = xen_feature_info_t - shares the same structure + * as the XENVER_get_features. + */ +#define XEN_VERSION_get_features 6 + +/* arg == xen_version_op_val_t. */ +#define XEN_VERSION_pagesize 7 + +/* + * arg == void. + * + * The toolstack fills it out for guest consumption. It is intended to hold + * the UUID of the guest. + */ +#define XEN_VERSION_guest_handle 8 + +/* arg = char[]. Contains NUL terminated utf-8 string. */ +#define XEN_VERSION_commandline 9 + #endif /* __XEN_PUBLIC_VERSION_H__ */ /* diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h index 64ba7ab..1a99929 100644 --- a/xen/include/public/xen.h +++ b/xen/include/public/xen.h @@ -115,6 +115,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t); #define __HYPERVISOR_tmem_op 38 #define __HYPERVISOR_xc_reserved_op 39 /* reserved for XenClient */ #define __HYPERVISOR_xenpmu_op 40 +#define __HYPERVISOR_version_op 41 /* supersedes xen_version (17) */ /* Architecture-specific hypercall definitions. */ #define __HYPERVISOR_arch_0 48 diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h index 0c8ae0e..e8d2b81 100644 --- a/xen/include/xen/hypercall.h +++ b/xen/include/xen/hypercall.h @@ -147,6 +147,10 @@ do_xenoprof_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg); extern long do_xenpmu_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg); +extern long +do_version_op(unsigned int cmd, + XEN_GUEST_HANDLE_PARAM(void) arg, unsigned int len); + #ifdef CONFIG_COMPAT extern int diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index abbe282..e5dad35 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -751,3 +751,24 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op) return xsm_default_action(XSM_PRIV, current->domain, NULL); } } + +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op) +{ + XSM_ASSERT_ACTION(XSM_OTHER); + switch ( op ) + { + case XEN_VERSION_version: + case XEN_VERSION_platform_parameters: + case XEN_VERSION_get_features: + /* These MUST always be accessible to any guest by default. */ + return 0; + case XEN_VERSION_extraversion: + case XEN_VERSION_capabilities: + case XEN_VERSION_pagesize: + case XEN_VERSION_guest_handle: + /* These can be accessible to a guest. */ + return xsm_default_action(XSM_HOOK, current->domain, NULL); + default: + return xsm_default_action(XSM_PRIV, current->domain, NULL); + } +} diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 5ecbee0..ac80472 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -194,6 +194,7 @@ struct xsm_operations { int (*pmu_op) (struct domain *d, unsigned int op); #endif int (*xen_version) (uint32_t cmd); + int (*version_op) (uint32_t cmd); }; #ifdef CONFIG_XSM @@ -737,6 +738,11 @@ static inline int xsm_xen_version (xsm_default_t def, uint32_t op) return xsm_ops->xen_version(op); } +static inline int xsm_version_op (xsm_default_t def, uint32_t op) +{ + return xsm_ops->version_op(op); +} + #endif /* XSM_NO_WRAPPERS */ #ifdef CONFIG_MULTIBOOT diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index 9791ad4..776dd09 100644 --- a/xen/xsm/dummy.c +++ b/xen/xsm/dummy.c @@ -163,4 +163,5 @@ void xsm_fixup_ops (struct xsm_operations *ops) set_to_dummy_if_null(ops, pmu_op); #endif set_to_dummy_if_null(ops, xen_version); + set_to_dummy_if_null(ops, version_op); } diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 2069cb3..1eaec58 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -1658,6 +1658,40 @@ static int flask_xen_version (uint32_t op) } } +static int flask_version_op (uint32_t op) +{ + u32 dsid = domain_sid(current->domain); + + switch ( op ) + { + case XEN_VERSION_version: + case XEN_VERSION_platform_parameters: + case XEN_VERSION_get_features: + /* These MUST always be accessible to any guest by default. */ + return 0; + case XEN_VERSION_extraversion: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__EXTRAVERSION, NULL); + case XEN_VERSION_capabilities: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__CAPABILITIES, NULL); + case XEN_VERSION_changeset: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__CHANGESET, NULL); + case XEN_VERSION_pagesize: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__PAGESIZE, NULL); + case XEN_VERSION_guest_handle: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__GUEST_HANDLE, NULL); + case XEN_VERSION_commandline: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__COMMANDLINE, NULL); + default: + return -EPERM; + } +} + long do_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op); int compat_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op); @@ -1797,6 +1831,7 @@ static struct xsm_operations flask_ops = { .pmu_op = flask_pmu_op, #endif .xen_version = flask_xen_version, + .version_op = flask_version_op, }; static __init void flask_init(void) diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors index badcf1c..56600bb 100644 --- a/xen/xsm/flask/policy/access_vectors +++ b/xen/xsm/flask/policy/access_vectors @@ -496,12 +496,14 @@ class security del_ocontext } -# Class version is used to describe the XENVER_ hypercall. +# Class version is used to describe the XENVER_ and VERSION hypercall. # Almost all sub-ops are described here - in the default case all of them should -# be allowed except the XENVER_commandline. +# be allowed except the XENVER_commandline, VERSION_commandline, and +# VERSION_changeset. # # The ones that are omitted are XENVER_version, XENVER_platform_parameters, -# and XENVER_get_features - as they MUST always be returned to a guest. +# XENVER_get_features, XEN_VERSION_version, XEN_VERSION_platform_parameters, +# and XEN_VERSION_get_features - as they MUST always be returned to a guest. # class version { @@ -519,4 +521,17 @@ class version xen_guest_handle # Xen command line. xen_commandline +# --- VERSION hypercall --- +# Extra informations (-unstable). + extraversion +# Such as "xen-3.0-x86_64 xen-3.0-x86_32p hvm-3.0-x86_32 hvm-3.0-x86_32p hvm-3.0-x86_64". + capabilities +# Source code changeset. + changeset +# Page size the hypervisor uses. + pagesize +# An value that the control stack can choose. + guest_handle +# Xen command line. + commandline }