From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask Date: Mon, 4 Aug 2014 15:50:07 +0100 Message-ID: <53DF9D9F.40209@citrix.com> References: <1407157961-7239-1-git-send-email-paul.durrant@citrix.com> <1407157961-7239-2-git-send-email-paul.durrant@citrix.com> <53DF8B21.2040907@citrix.com> <53DFABAA02000078000290C4@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53DFABAA02000078000290C4@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Paul Durrant Cc: Ian Jackson , Stefano Stabellini , Keir Fraser , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 04/08/14 14:50, Jan Beulich wrote: >>>> On 04.08.14 at 15:31, wrote: >> On 04/08/14 14:12, Paul Durrant wrote: >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -5533,8 +5533,22 @@ long do_hvm_op(unsigned long op, >> XEN_GUEST_HANDLE_PARAM(void) arg) >>> rc = -EINVAL; >>> break; >>> case HVM_PARAM_VIRIDIAN: >>> - if ( a.value > 1 ) >>> - rc = -EINVAL; >>> + /* This should only ever be set once by the tools and read by the guest. */ >>> + rc = -EPERM; >>> + if ( curr_d == d ) >>> + break; >>> + >>> + rc = -EPERM; >>> + if ( d->arch.hvm_domain.params[a.index] && >>> + a.value != d->arch.hvm_domain.params[a.index] ) >>> + break; >> Setting it twice should be an error, even if it is set to the same value >> again. > I specifically asked for it to be done this way, such that redundant > calls wouldn't needlessly fail. Remember that we're altering an > existing interface, and hence should be careful about breaking > existing callers. The only valid users are domain builder parts of the toolstack, which necessarily needs to be in sync with Xen. All current in-tree callers are ok. While in general I would agree, we are already changing the interface quite substantially. A stricter interface is easier to augment later if the need arises, and here I feel there is sufficient change to warrant doing the interface properly rather than leaving this quirk around forevermore. In a more general sense, having worked on the migration code, I was considering that it would be a *very* good thing to move all of this logic into the toolstack, with Xen interacting with a pristine set of up-to-latest-interface state. This would reduce the amount of Xen code doing input sanitisation/manipulation, and moves all of the backwards compatibility cruft into a safer context to run. One frequently requested feature of XenServer (which has a number of large obstacles, but is sane in principle) is the ability to migrate backwards. The usecase is for this is being able to undo a half-upgrade which has gone very wrong. With all the compatibility code in the toolstack rather than Xen, this would be feasible to implement. ~Andrew