From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [PATCH 11/20] PVH xen: introduce pvh.c Date: Wed, 15 May 2013 18:42:07 -0700 Message-ID: <20130515184207.0f50b06d@mantra.us.oracle.com> References: <1368579168-30829-1-git-send-email-mukesh.rathor@oracle.com> <1368579168-30829-12-git-send-email-mukesh.rathor@oracle.com> <519382BA02000078000D6658@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <519382BA02000078000D6658@nat28.tlf.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 Cc: xen-devel List-Id: xen-devel@lists.xenproject.org On Wed, 15 May 2013 11:42:34 +0100 "Jan Beulich" wrote: > >>> On 15.05.13 at 02:52, Mukesh Rathor > >>> wrote: > > --- /dev/null > > + > > +static int pvh_grant_table_op(unsigned int cmd, > > XEN_GUEST_HANDLE(void) uop, > > + unsigned int count) > > +{ > > +#ifndef NDEBUG > > The whole function should be in such a conditional (if it's really > needed, which I previously said I doubt). Without doing so, and Temporary code, can we leave it for initial stages while PVH is being tested and tried. It will help with debug a lot. Alternately, would you be ok with something like the patch way below. I could do same for grant and hypercalls too. > with the way you have pvh_hypercall64_table[], build will fail > for debug=n. I know. I left it for you to implment it the way you wanted with memcopy of the table, which I completely disagree with. > > + rc = do_vcpu_op(cmd, vcpuid, arg); > > + > > + /* pvh boot vcpu setting context for bringing up smp vcpu */ > > + if ( cmd == VCPUOP_initialise ) > > + vmx_vmcs_enter(current); > > This is wrong in three ways - for one, you can't call a vmx function > from here, then the operation also doesn't appear to belong here, Ooops, sorry! This was in vmx_pvh.c file originally and got moved. Yeah, I struggled with this, I can't remember why I needed this. Let me go back and investigate this. > and with pvh_hypercall64_table[] not even existing in non-debug > builds this won't happen there at all (which is making very clear that > these function overrides are plain wrong, as I had tried to tell you > from the beginning). I don't think they are *wrong*, they work just fine for HVM! They might be a different approach than you might like, and I appreciate that. But I am thinking of being able to easily catch and debug things when the feature goes thru infancy. thanks Mukesh -------------------------------------------------------------------------- diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 21382eb..b1a33b5 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1129,6 +1129,10 @@ arch_do_vcpu_op( struct domain *d = v->domain; struct vcpu_register_vcpu_info info; + rc = -ENOSYS; + if ( is_pvh_vcpu(current) ) + break; + rc = -EFAULT; if ( copy_from_guest(&info, arg, 1) ) break; @@ -1169,6 +1173,10 @@ arch_do_vcpu_op( { struct vcpu_get_physid cpu_id; + rc = -ENOSYS; + if ( is_pvh_vcpu(current) ) + break; + rc = -EINVAL; if ( !is_pinned_vcpu(v) ) break; diff --git a/xen/common/domain.c b/xen/common/domain.c index a734755..68de4bb 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -968,6 +968,12 @@ long do_vcpu_op(int cmd, int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) } case VCPUOP_down: + if ( is_pvh_vcpu(current) ) + { + rc = -ENOSYS; + break; + } + if ( !test_and_set_bit(_VPF_down, &v->pause_flags) ) vcpu_sleep_nosync(v); break; @@ -1039,6 +1045,11 @@ long do_vcpu_op(int cmd, int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) #ifdef VCPU_TRAP_NMI case VCPUOP_send_nmi: + if ( is_pvh_vcpu(current) ) + { + rc = -ENOSYS; + break; + } if ( !guest_handle_is_null(arg) ) return -EINVAL;