From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sahita, Ravi" Subject: Re: [PATCH v7 11/15] x86/altp2m: define and implement alternate p2m HVMOP types. Date: Thu, 23 Jul 2015 15:16:21 +0000 Message-ID: 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> <55B11F970200007800094B83@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: <55B11F970200007800094B83@prv-mh.provo.novell.com> Content-Language: en-US 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 Cc: Tim Deegan , "Sahita, Ravi" , Wei Liu , George Dunlap , Andrew Cooper , Ian Jackson , "White, Edmund H" , "xen-devel@lists.xen.org" , "Nakajima, Jun" , "tlengyel@novetta.com" , Daniel De Graaf List-Id: xen-devel@lists.xenproject.org >From: Jan Beulich [mailto:JBeulich@suse.com] >Sent: Thursday, July 23, 2015 8:09 AM > >>>> 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? > Yes in a revision I would handle this with the default case. >> 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. Thanks for your flexibility and honesty on this - hope other maintainers are ok with v7 in the same way, with keeping the other comments you have logged already as pending for post 4.6. Ravi > >Jan