From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 11/20] PVH xen: introduce pvh.c Date: Thu, 16 May 2013 09:00:44 +0100 Message-ID: <5194AE4C02000078000D6A2D@nat28.tlf.novell.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> <20130515184207.0f50b06d@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130515184207.0f50b06d@mantra.us.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: Mukesh Rathor Cc: xen-devel List-Id: xen-devel@lists.xenproject.org >>> On 16.05.13 at 03:42, Mukesh Rathor wrote: > --- 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; > + Assuming this is meant to be temporary - yes, this _might_ be acceptable (if accompanied by a proper comment). But then again registering vCPU info is a pretty basic interface (which recently got even moved into common code iirc), so I'm having a hard time seeing why you need to suppress it rather than make it work from the beginning. > 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; > + Similarly here - what's wrong with this for PVH? > rc = -EINVAL; > if ( !is_pinned_vcpu(v) ) > break; > --- 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; > + } I can see that this may indeed require some special cases to be taken care of. But adding a comment is then the minimum requirement. And the increasing amount of "PVH fixme"s is worrying in their own right - once again, I went through a similar exercise with 32-on-64 support, and didn't have a need to post patches for public review (with the underlying implication of them being in a mergeable state) with this many issues left open. > + > 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; > + } This one may even have to remain, assuming PVH would send NMIs via LAPIC ICR? But then using !is_pv_domain() would seem the right thing here. Otoh, allowing as many PV operations as possible along with the respective HVM counterparts may be desirable for easing the kernel side transition? Jan > if ( !guest_handle_is_null(arg) ) > return -EINVAL; >