From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v7 11/15] x86/altp2m: define and implement alternate p2m HVMOP types. Date: Thu, 23 Jul 2015 09:08:39 -0600 Message-ID: <55B11F970200007800094B83@prv-mh.provo.novell.com> References: <1437606081-6964-1-git-send-email-edmund.h.white@intel.com> <1437606081-6964-12-git-send-email-edmund.h.white@intel.com> <55B0DC790200007800094762@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ravi Sahita Cc: Tim Deegan , Wei Liu , George Dunlap , Andrew Cooper , Ian Jackson , Edmund H White , "xen-devel@lists.xen.org" , Jun Nakajima , "tlengyel@novetta.com" , Daniel De Graaf List-Id: xen-devel@lists.xenproject.org >>> On 23.07.15 at 16:56, wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >>Sent: Thursday, July 23, 2015 3:22 AM >> >>>>> On 23.07.15 at 01:01, wrote: >>> Signed-off-by: Ed White >>> >>> Acked-by: Jan Beulich >> >>And I have to withdraw this ack pending clarification of (and perhaps >>adjustment to) the #VE info address interface. >> > > Could we have the ack back :-) I clarified the #VE info address interface in > the other email - repeating here: > > " If the "EPT-violation #VE" VM-execution control is 1, the > virtualization-exception information address must > satisfy the following checks: > - Bits 11:0 of the address must be 0. > - The address must not set any bits beyond the processor's physical-address > width." Yes, for this aspect. >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -6138,6 +6138,140 @@ static int hvmop_get_param( >>> return rc; >>> } >>> >>> +static int do_altp2m_op( >>> + XEN_GUEST_HANDLE_PARAM(void) arg) { >>> + struct xen_hvm_altp2m_op a; >>> + struct domain *d = NULL; >>> + int rc = 0; >>> + >>> + if ( !hvm_altp2m_supported() ) >>> + return -EOPNOTSUPP; >>> + >>> + if ( copy_from_guest(&a, arg, 1) ) >>> + return -EFAULT; >>> + >>> + if ( a.pad1 || a.pad2 || >>> + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || >>> + (a.cmd < HVMOP_altp2m_get_domain_state) || >>> + (a.cmd > HVMOP_altp2m_change_gfn) ) >> >>I'm afraid such a change invalidates any earlier ack, even if ti is correct. > Instead >>of this, why don't you start numbering of the sub-ops at zero? Or if you > really >>have a reason to start at 1, why not simply check a.cmd against zero (without >>using any particular sub-op value)? And then it escapes me why this can't be >>handled in a default case in the switch statement below anyway. > > Hmm - is that a requirement per se? we are consistently checking per the > sub-op definition we have. Well, in a way. But doing range checks like this means future additions of sub-ops would always need to touch this code. Quite different from doing it in the default case of a switch statement. Plus, can you see how the expression is going to look like if in interface version 2 you need to remove one or two of the current entries, replacing them with new, higher numbers? > Would like this to be considered as is. > > As I said in the cover letter we have constraints on how much more we can do > this week now - > so requesting the maintainers to accept v7 with the review comments you have > on those recorded as pending to be addressed by us. Yes, on that basis, albeit extremely hesitantly to be honest. If any other maintainer would be as hesitant as I am about this, I would likely put the two together to yield a NAK. Jan