From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v3 3/4] x86/pvh: Handle hypercalls for 32b PVH guests Date: Tue, 11 Aug 2015 03:32:58 -0600 Message-ID: <55C9DD6A020000780009970A@prv-mh.provo.novell.com> References: <1436566853-8444-1-git-send-email-boris.ostrovsky@oracle.com> <1436566853-8444-4-git-send-email-boris.ostrovsky@oracle.com> <55B1147D0200007800094A8C@prv-mh.provo.novell.com> <55B28558.4020906@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55B28558.4020906@oracle.com> 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: Boris Ostrovsky Cc: elena.ufimtseva@oracle.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, roger.pau@citrix.com List-Id: xen-devel@lists.xenproject.org >>> On 24.07.15 at 20:35, wrote: > On 07/23/2015 10:21 AM, Jan Beulich wrote: >>>>> On 11.07.15 at 00:20, wrote: >>> Signed-off-by: Boris Ostrovsky >>> --- >>> Changes in v3: >>> * Defined compat_mmuext_op(). (XEN_GUEST_HANDLE_PARAM(mmuext_op_compat_t) >>> is not defined in header files so I used 'void' type. >> How is it not? It's in compat/xen.h (which is a generated header). > > compat/xen.h has DEFINE_COMPAT_HANDLE(mmuext_op_compat_t) (which is > __compat_handle_mmuext_op_compat_t). > > We need XEN_GUEST_HANDLE(mmuext_op_compat_t), which is > __guest_handle_mmuext_op_compat_t. And I wasn't sure it's worth > explicitly adding it to a header file (like I think what we do for > vcpu_runstate_info_compat_t in sched.h); Hmm, indeed all other compat_..._op()-s use void handles (albeit in most if not all of the cases their native counterparts do too). So I guess using void here is fine then, or using COMPAT_HANDLE() instead. It's not really relevant anyway since COMPAT_CALL() casts the function pointer to the intended type anyway. >>> @@ -4981,7 +5003,7 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) >>> return viridian_hypercall(regs); >>> >>> if ( (eax >= NR_hypercalls) || >>> - (is_pvh_domain(currd) ? !pvh_hypercall64_table[eax] >>> + (is_pvh_domain(currd) ? !pvh_hypercall32_table[eax] >>> : !hvm_hypercall32_table[eax]) ) >> ... this will break (as we're assuming 32- and 64-bit tables to be fully >> in sync here; there's still the pending work item of constructing these >> tables so that this has a better chance of not getting broken). > > So you prefer to have full check --- explicitly for both 32- and 64-bit, > right? No. Just adding the missing operation to the table will deal with it. I wouldn't like to see more conditionals to be added to this code path when we can avoid doing so. What we could do is add a respective ASSERT() to the 64-bit path, albeit the NULL deref would be observable as a fault without the ASSERT() too (and adding one wouldn't help release builds [and their security]). Jan